Skip to content

Conversation

@vonzshik
Copy link
Contributor

@vonzshik vonzshik commented Mar 3, 2025

Closes #6035

@vonzshik vonzshik requested a review from roji as a code owner March 3, 2025 13:30
@Brar
Copy link
Member

Brar commented Mar 4, 2025

I actually liked it better before you added the explicit RequireAuthMode.All, because we share PGREQUIREAUTH with libpq and before adding it we were 100% syntax compatible whereas now we have the All mode in addition.

It's not a big deal because our default for the unset option is the same as libpq's (All) but if users explicitly set PGREQIREAUTH to All because they think that it's a good idea or required, they break libpq connections.

@vonzshik
Copy link
Contributor Author

vonzshik commented Mar 4, 2025

I actually liked it better before you added the explicit RequireAuthMode.All, because we share PGREQUIREAUTH with libpq and before adding it we were 100% syntax compatible whereas now we have the All mode in addition.

It's not a big deal because our default for the unset option is the same as libpq's (All) but if users explicitly set PGREQIREAUTH to All because they think that it's a good idea or required, they break libpq connections.

I think we can just disallow parsing ALL, it's an internal enum after all, so as long as we do not document it publicly...

@vonzshik
Copy link
Contributor Author

vonzshik commented Mar 4, 2025

OK, seems no one has an any other objections. Merging.

@vonzshik vonzshik merged commit a4a7f60 into main Mar 4, 2025
14 checks passed
@vonzshik vonzshik deleted the 6035-add-require-auth branch March 4, 2025 12:12
@meteortent
Copy link

want to know when this commit could be release? will it be in version 9.0.4?

@vonzshik
Copy link
Contributor Author

want to know when this commit could be release? will it be in version 9.0.4?

We usually do not backport features (and this is a feature) to previous releases, so most likely 10.0 in November.

@meteortent

This comment was marked as off-topic.

@vonzshik

This comment was marked as off-topic.

@meteortent

This comment was marked as off-topic.

@vonzshik

This comment was marked as off-topic.

@meteortent

This comment was marked as off-topic.

@vonzshik

This comment was marked as off-topic.

@meteortent

This comment was marked as off-topic.

@roji

This comment was marked as off-topic.

@meteortent

This comment was marked as off-topic.

@roji

This comment was marked as off-topic.

@meteortent

This comment was marked as off-topic.

@NinoFloris

This comment was marked as off-topic.

@meteortent

This comment was marked as off-topic.

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.

Add RequireAuth connection string option

6 participants