Skip to content

fix: Add support for postgres 9.2 and lower#1392

Closed
KijongHan wants to merge 1 commit intotestcontainers:developfrom
KijongHan:fix/add-support-for-postgres-9.3-and-lower
Closed

fix: Add support for postgres 9.2 and lower#1392
KijongHan wants to merge 1 commit intotestcontainers:developfrom
KijongHan:fix/add-support-for-postgres-9.3-and-lower

Conversation

@KijongHan
Copy link
Contributor

What does this PR do?

This PR adds support for running Postgres containers below version 9.3. Adds to the PostgreSqlBuilder.Build method to check for Postgres versions below 9.3 (which doesn't have support for pg_isready to check for database readiness) using Image.MatchVersion - for these versions check for message logged in container using UntilMessageIsLogged helper method for the message "PostgreSQL init process complete; ready for start up."

Other strategies tried

  • using psql to connect to container db and running select 1 or select version() - these were not reliable and the database was not in a state ready to accept connections
  • using "database system is ready to accept connections" - but couldn't reliably connect on the first instance of the message

related github discussion that helped determine above strategy: docker-library/postgres#146

Why is it important?

some older codebases are built on older versions of postgres, and being able to build tests using those older versions before migrating the version gives peace of mind for validating successful migration

@netlify
Copy link

netlify bot commented Mar 7, 2025

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 4947ff3
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/67d1d835525bf800086b8df2
😎 Deploy Preview https://deploy-preview-1392--testcontainers-dotnet.netlify.app
📱 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.

@KijongHan KijongHan force-pushed the fix/add-support-for-postgres-9.3-and-lower branch from 0f7d7b9 to 4947ff3 Compare March 12, 2025 18:53
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! The changes look good. However, TBH I'm not sure if I like adding support for a version that is EOL. Developers can simply override the default wait strategy or fall back to the generic container builder. But I agree that it is very difficult for developers to understand which versions are supported and which are not. This is something I've been thinking about a lot, but I haven't come up with a proper solution yet.

I don't know how to proceed from here. It's probably best to merge it so that other developers don't run into the same issue 🤔.

Note: I believe it was necessary to wait until the log messages were logged twice (for a reliable indication). I need to double-check the other language implementations.

@0xced
Copy link
Contributor

0xced commented Mar 15, 2025

I have a UntilDatabaseIsAvailable branch on my fork which would solve this issue. The UntilDatabaseIsAvailable wait strategy works with any ADO.NET provider. It could even have been used to workaround #1383! I actually copy/pasted this wait strategy in two projects as a workaround while waiting for Testcontainers 4.4.0 to be released.

I think it would be a perfect fit for this use case too (supporting PostgreSQL containers below version 9.3) and for any other potentially flaky wait strategy for database containers.

Edit: I just opened #1401. Then it could be used like this:

new PostgreSqlBuilder()
    .WithImage("postgres:9.2")
    .WithWaitStrategy(Wait.ForUnixContainer().UntilDatabaseIsAvailable(NpgsqlFactory.Instance));

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I had a quick discussion in our Slack workspace and created this discussion this morning: #1407, LMKWYT. I am heading out for a few days off now, I will review your (@0xced) PR when I get back. After that, we can decide how to proceed with this PR. Thanks again 🙏.

@KijongHan
Copy link
Contributor Author

Thanks for the PR. I had a quick discussion in our Slack workspace and created this discussion this morning: #1407, LMKWYT. I am heading out for a few days off now, I will review your (@0xced) PR when I get back. After that, we can decide how to proceed with this PR. Thanks again 🙏.

Yeah I think your proposal there is quite reasonable, given this image is outside the LTS Im happy to close off this PR until the need arises. Appreciate your efforts!

@KijongHan KijongHan closed this Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants