Skip to content

Fix assertion failures introduced by psycopg 3.2.4#1250

Merged
JelteF merged 6 commits into
pgbouncer:masterfrom
cosgroveb:upgrade_psycopg
Jan 30, 2025
Merged

Fix assertion failures introduced by psycopg 3.2.4#1250
JelteF merged 6 commits into
pgbouncer:masterfrom
cosgroveb:upgrade_psycopg

Conversation

@cosgroveb

@cosgroveb cosgroveb commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

These failures were previously addressed by @AndrewJackson2020
in #1249 by pinning psycopg to 3.2.3. I became aware of this because I
just re-enabled the python test suite in Debian1 2.

This is a good change in psycopg3. Let's update the assertions and bump
the version.

Open question around whether or not we should pin here? It would be good if
we can sort these issues out before they block the upload of new
pgbouncer packages to downstream distros (although most folks probably
care more about pgdg?) No worries either way.

These failures were previously addressed by @AndrewJackson2020
in pgbouncer#1249 by pinning psycopg to 3.2.3. I became aware of this because I
just re-enabled the python test suite in Debian[1][2].

This is a good change in psycopg. Let's update the assertions and bump
the version.

Open question around whether or not we should pin here? It would be good if
we can sort these issues out before they block the upload of new
pgbouncer packages to downstream distros (although most folks probably
care more about pgdg?) No worries either way.

[1]: https://www.postgresql.org/message-id/CAOMoQbROhu7WG9LnyjHb1RXJrax_HKKMBbwaou%2Bjps0uZgqHFw%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAGKCzNAQKMKJ_BQcF%3DuzcfcM%3Dvx8QYnT%2Bjtzuv_yp%3D76zbhn7g%40mail.gmail.com
@AndrewJackson2020

Copy link
Copy Markdown
Contributor

Thank you for doing this.

I am on team pin. Further I think we should pin every version of every package in the requirements.txt file as long as it passes CI. I think that in an ideal world 2 runs of CI should produce the exact same results every single time, and pinning gets us closer to that.

@df7cb

df7cb commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

Fwiw, pinning won't help with Debian, we will be using the package that is present in the release targeted, ignoring any pins.

Comment thread test/test_misc.py
@cosgroveb

Copy link
Copy Markdown
Contributor Author

@df7cb

Fwiw, pinning won't help with Debian, we will be using the package that is present in the release targeted, ignoring any pins.

Yeah, that's a more explicit way of stating what I was trying to say here:

Open question around whether or not we should pin here? It would be good if
we can sort these issues out before they block the upload of new
pgbouncer packages to downstream distros (although most folks probably
care more about pgdg?) No worries either way.

Since Debian will always build against the latest version of
dependencies (such as psycopg) available in the distro this will
keep tests passing on stable and oldstable distros.
@cosgroveb

Copy link
Copy Markdown
Contributor Author

I am on team pin. Further I think we should pin every version of every package in the requirements.txt file as long as it passes CI.

This approach works well for internal platform teams that have mirrors and aren't disrupted e.g. when a package is pulled but IMHO it's less realistic for an OSS project being distributed across several OSes and packaging systems. I agree at least that there's a time and place for it.

Co-authored-by: Brian Cosgrove <bcosgrove@paypal.com>
@df7cb

df7cb commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

Afaict requirements.txt is just ignored by most parts of the Debian toolchain. If you insist on pinning to a version that behaves differently, we will poke you with a stick until the regression tests are working again for Debian and apt.postgresql.org. 😁

@df7cb

df7cb commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

That said, https://jengus.postgresql.org/job/pgbouncer-binaries/102/architecture=amd64,distribution=sid/console is unhappy and we'd appreciate a new release with this PR merged. 😸

@AndrewJackson2020

Copy link
Copy Markdown
Contributor

Is the idea with removing pins that any upstream packager will always pull the most recent versions of any python packages so the pgbouncer requirements.txt should always pull the most recent version even if that means a successful build yesterday could mean a failed build today?

@cosgroveb

cosgroveb commented Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

Yes. We should aim for our test suite to be compatible with all of the versions of our dependencies that downstream packagers will use and a build failure tomorrow is precisely how that is accomplished.

We definitely want our tests passing in Debian. To get the signal that we need we must not pin.

@JelteF

JelteF commented Jan 30, 2025

Copy link
Copy Markdown
Member

Is the idea with removing pins that any upstream packager will always pull the most recent versions of any python packages so the pgbouncer requirements.txt should always pull the most recent version even if that means a successful build yesterday could mean a failed build today?

Yeah, I think that's a fair assesment. And I think that's a reasonable tradeoff. Packaging is definitely a case where we cannot force a specific python dependency version on people, so we should just make sure that we work with all. And then we'll just have to live with the fact that we need to fix our build sometimes (for which a temporary workaround can be to pin the version).

@JelteF JelteF merged commit 7820686 into pgbouncer:master Jan 30, 2025
@cosgroveb cosgroveb deleted the upgrade_psycopg branch January 30, 2025 17:17
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.

4 participants