Expose PostgreSQL on default port in PostgreSqlQueryRunner.main

[vlad-lyutenko]

Description

Previously ports were randomly bound, which made it harder to work on local environment. Now when running outside of the CI default ports will be used.

For now its initial change for Postgressql only, but it could be extended to others (like Mysql, Sqlserver e.t.c.)

Non-technical explanation

Release notes

(x) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[cla-bot]

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

[hashhar]

Useful change for local development. Thanks.

Some nits about comments.

[hashhar]

cc: @ebyhr you might find this useful when running query runners locally.

@vlad-lyutenko Please update the PR description to also mention if you plan to make similar change to other query runners as well? I assume this PR is to gather feedback?

[findepi]

@vlad-lyutenko i am not super fond of the change.
it will make it impossible to run multiple instances of integration tests concurrently, something that i do from time to time.
(It will also make it impossible to run integration tests when you have a product env running)

It would make more sense to limit the change to PostgreSqlQueryRunner.main.

Or not do it at all. For local development purpose I leverage the cheat sheet https://gist.github.com/findepi/04c96f0f60dcc95329f569bb0c44a0cd :

#  run PostgreSQL
bin/ptl env up --environment singlenode-postgresql --without-trino

# Connect to PostreSQL with CLI
docker exec -itu postgres ptl-postgresql psql -U test -d test

# connect to PostgreSQL with Trino
open https://github.com/trinodb/trino/blob/master/README.md#running-the-full-server

[findepi]

per

it will make it impossible to run multiple instances of integration tests concurrently

[hashhar]

Limiting to main makes sense. Good point about concurrent tests (which can also happen if you run tests for entire module from CLI since by default we run tests in parallel).

[ebyhr]

+1 to limit the usage to query runner main class.

[wendigo]

The whole point of this change is to make it easier to work with QueryRunners and connect to a containerized database on the default port. Limiting usage to main fulfill this need.

Addressed

[ebyhr]

Adding a new argument to TestingPostgreSqlServer looked fine, but TestingMySqlServer already has two arguments in the constructor. If we will apply this change to other connectors too, we may want to consider different approach. e.g. Introduce a new environment variable likes TESTCONTAINERS_USE_FIXED_PORT.

(Please ignore this comment if it's already considered)

[ssheikin]

LGTM, please squash corresponding commits.

[vlad-lyutenko]

Applied comments. Please take a look again @hashhar @findepi @wendigo @ssheikin

[hashhar]

Adding a new argument to TestingPostgreSqlServer looked fine, but TestingMySqlServer already has two arguments in the constructor. If we will apply this change to other connectors too, we may want to consider different approach. e.g. Introduce a new environment variable likes TESTCONTAINERS_USE_FIXED_PORT.

(Please ignore this comment if it's already considered)

@ebyhr Do you mean checking for the env var directly in the query runner instead?
I do like the simplicity of setting a well-named self-explanatory env var like USE_FIXED_PORT.

We can revisit this when implementing this for other query runners.
I'm fine with current approach for now.

[ebyhr]

Do you mean checking for the env var directly in the query runner instead?

Yes, setting environment variables in Intellij's Run/Debug Configurations' looks enough to me.

We can revisit this when implementing this for other query runners.
I'm fine with current approach for now.

👍

[ssheikin]

Adding a new argument to TestingPostgreSqlServer looked fine, but TestingMySqlServer already has two arguments in the constructor. If we will apply this change to other connectors too, we may want to consider different approach. e.g. Introduce a new environment variable likes TESTCONTAINERS_USE_FIXED_PORT.

Do you mean checking for the env var directly in the query runner instead?

Yes, setting environment variables in Intellij's Run/Debug Configurations' looks enough to me.

I have another usecase which I poorly described here: #14037 (comment) and for me such environment variable would perfectly match to NOT limit the usage to query runner main class., but use in all factory methods, as with env var we can easily steer behaviour.

however

I'm fine with current approach for now.

because it's much concise and straight-forward as it is now.