Skip to content

Support "options" in ignore_startup_parameters again #908

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

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

JelteF
Copy link
Member

@JelteF JelteF commented Jul 27, 2023

By supporting parsing the options startup parameter, we accidentally
stopped supporting using it in ignore_startup_parameters.

Users have put options in ignore_startup_parameters and so upgrading
to 1.20.0 might break their existing connection strings, if the actual
options that are contained in the options startup parameter are
unsupported.

This changes the parsing of options to allow any unsupported parameter
if options itself is in ignore_startup_parameters.

NOTE: There's other failure paths in the options startup parameter
parsing that could still trigger now even though options is in
ignore_startup_parameters:

  • too long parameter
  • non -c parameter
  • options would have been unparsable by PostgreSQL too

But honestly those seem unlikely enough that I think it's better to
error for them than to ignore those errors.

Fixes #907

By supporting parsing the `options` startup parameter, we accidentally
stopped supporting using it in `ignore_startup_parameters`.

Users have put `options` in `ignore_startup_parameters` and so upgrading
to 1.20.0 might break their existing connection strings, if the actual
options that are contained in the `options` startup parameter are
unsupported.

This changes the parsing of `options` to allow any unsupported parameter
if `options` itself is in `ignore_startup_parameters`.

NOTE: There's other failure paths in the `options` startup parameter
parsing that could still trigger now even though `options` is in
`ignore_startup_parameters`:

- too long parameter
- non `-c` parameter
- options would have been unparsable by PostgreSQL too

But honestly those seem unlikely enough that I think it's better to
error for them than to ignore those errors.

Fixes pgbouncer#907
@emelsimsek
Copy link
Contributor

Changes look good in restoring the compatibility with previous versions.

As a separate note, ignore_startup_parameters documentation is confusing to me. What does it mean that ignored parameters are handled by admin? I expect them not being cached but passed to postgres as they are.

ignore_startup_parameters
By default, PgBouncer allows only parameters it can keep track of in startup packets: client_encoding, datestyle, timezone and standard_conforming_strings. All others parameters will raise an error. To allow others parameters, they can be specified here, so that PgBouncer knows that they are handled by the admin and it can ignore them.

If you need to specify multiple values, use a comma-separated list (e.g. options,extra_float_digits)

@eulerto
Copy link
Member

eulerto commented Jul 28, 2023

@JelteF The patch looks good to me. However, I have 2 suggestions:

  • add a comment in the set_startup_options function describing the behavior of options value in ignore_startup_parameters.
  • we should better document the options parameter. The commit dff974f does not add any documentation. I'm not sure exactly where to add it. Maybe in the ignore_startup_parameters as a new paragraph.

@JelteF
Copy link
Member Author

JelteF commented Aug 8, 2023

Added the docs. Merging this now.

@JelteF JelteF merged commit ff27483 into pgbouncer:master Aug 8, 2023
@JelteF JelteF deleted the fix-907 branch August 8, 2023 12:59
@torstenfohrer
Copy link

Hi, i trying to get pgbouncer work with some software that send following

image

And nothing set in ignore_startup_parameters=options,client_encoding,standard_conforming_strings will get around it.

Looking at code you try to find first -c option=value and then look if options is set in ignore_startup_parameters

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.

Unsupported startup parameter in options: statement_timeout
4 participants