Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(postgres): recommend ForSQL strategy #2208

Closed
wants to merge 1 commit into from

Conversation

greg0ire
Copy link

While trying to use a postgres containers and using ForLog(), we had a tests that worked locally, but failed on the CI, for some reason. Searching the web, one can stumble upon https://www.xavierbouclet.com/2022/08/08/TestContainer-Wait-DB-SQL.html If somebody went through the trouble of writing a whole blog post about this, chances are this issue is not uncommon.
Also, the SQL strategy was contributed after the log strategy, which might explain why it wasn't default recommendation.

@greg0ire greg0ire requested a review from a team as a code owner February 14, 2024 17:04
Copy link

netlify bot commented Feb 14, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 0daaffc
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/65cf3784b8279d000845bd2a
😎 Deploy Preview https://deploy-preview-2208--testcontainers-go.netlify.app/modules/postgres
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdelapenya
Copy link
Collaborator

Thanks for this PR! I'm interested in how you discovered it: is it that you copied the example from pkg.go.dev?

@mdelapenya mdelapenya self-assigned this Feb 14, 2024
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Feb 14, 2024
@greg0ire
Copy link
Author

greg0ire commented Feb 14, 2024

No, there is a dedicated documentation page for that strategy (I would link it if I wasn't on my phone).

EDIT: I'm back home: https://golang.testcontainers.org/features/wait/sql/

While trying to use a postgres containers and using ForLog(), we had a
tests that worked locally, but failed on the CI, for some reason.
Searching the web, one can stumble upon https://www.xavierbouclet.com/2022/08/08/TestContainer-Wait-DB-SQL.html
If somebody went through the trouble of writing a whole blog post about
this, chances are this issue is not uncommon.
Also, the SQL strategy was contributed after the log strategy, which
might explain why it wasn't default recommendation.
@eddumelendez
Copy link
Member

Hi, one missing thing about the shared blogpost is related to how the postgres container is started:

  1. Container start
  2. Postgres service start
  3. Postgres service stop
  4. Postgres service start

After that, the container and service is ready to use. The blogpost uses the default strategy, which is wait for an open port but this could be available in step 2 causing flaky tests. SQL wait strategy could cause the same. The PostgreSQLContainer implementation relies in the wait strategy instead due to an empty container logs twice the database system is ready to accept connections but a data volume is mounted then it is logged once. But, that's a different issue.

@greg0ire
Copy link
Author

@eddumelendez it's unclear to me what makes you say that the postgres container is started in 4 steps rather than just 2. Would you care to elaborate?

@eddumelendez
Copy link
Member

@greg0ire start a postgres container and see the logs to check what I mention

@greg0ire
Copy link
Author

greg0ire commented Feb 17, 2024

Ok, I've tried it, so let me explain this a bit more to everyone else:

  1. The container starts
  2. It detects if the database already exists or not
  3. Based on that, it either creates the database or skips initialization
  4. It starts

In case there is no database, there seems indeed to be a start and stop of the server: https://github.com/docker-library/postgres/blob/1424abf76f421d6f7bf933d9e42bbbed866fae3a/16/bookworm/docker-ensure-initdb.sh#L46C2-L51

That start is "socket only", and "does not listen on external TCP/IP". Maybe we could somehow find a way to leverage that?

@greg0ire greg0ire marked this pull request as draft February 17, 2024 19:03
@JordanP
Copy link
Contributor

JordanP commented Mar 6, 2024

True that if the DATA_DIR is empty, PG will start "2 times". But that's why the WithOccurrence(2) line is here. So I am not sure this PR is a big improvement .... Is there a way to reproduce (ideally "consistently") what you observed ?

@greg0ire
Copy link
Author

greg0ire commented Mar 7, 2024

I didn't experience it first hand, @joseboretto did. Let's close this so as not to waste more maintainer time on this, and open an issue if we come up with a reproducer instead.

@greg0ire greg0ire closed this Mar 7, 2024
@greg0ire greg0ire deleted the recommend-sql-wait branch March 7, 2024 07:51
@mdelapenya
Copy link
Collaborator

Thanks @greg0ire for coming back to the issue and close it 🙇 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants