-
Notifications
You must be signed in to change notification settings - Fork 450
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
Add support for options startup parameter #878
Conversation
2fc6a3a
to
d746015
Compare
d746015
to
6cb1b0a
Compare
6cb1b0a
to
6119e8c
Compare
Looks good overall. |
psycopg.OperationalError, | ||
match="unsupported options startup parameter: parameter too long", | ||
): | ||
bouncer.test(options="-c timezone=" + too_long_param) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a test case for a variable that is neither tracked nor ignored in the config:
-c extra_float_digits=3
to exhaustively test the failure paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used enable_seqscan
for the test you were describing and extra_float_digits
for one that used ignore_startup_parameters.
* options startup paramater. Also all other postgres command line arguments | ||
* can be rewritten to this form: | ||
*/ | ||
static bool set_startup_options(PgSocket *client, const char *options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can reuse other functions from loader.c such as cstr_get_pair(), cstr_get_key() and cstr_get_value().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly the parsing works slightly differently for the options
string and the connections string. The options string uses \
to escape whitespace characters, while the connection string quotes values in single quotes.
src/client.c
Outdated
|
||
/* | ||
* set_startup_options takes the value of to the "options" startup parameter | ||
* and uses it to set the parameters that are embeded in this value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/of to the/of the/
s/embeded/embedded/
s/postgres/PostgreSQL/
s/paramater/parameter/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI for next time: You can do such suggestions using the Github interface, which allows easily applying them
* and uses it to set the parameters that are embeded in this value. | |
* and uses it to set the parameters that are embedded in this value. |
equals = strchr((char *) arg.data, '='); | ||
if (!equals) | ||
goto fail; | ||
*equals = '\0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a matter of style but I will suggest anyway.
*equals++ = '\0';
and remove + 1
2 lines below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this as is, I think it's clearer this way. Mostly because the variable is called equals
, thus logically pointing to the equals character. If it was the position
variable I'd have agreed with the suggestion.
The `-c config=value` pattern has been supported in PgBouncer since PR pgbouncer#878. As described in pgbouncer#952 Postgres also allows to use the `--config=value`, which honestly seems more readable, but for some reason is mentioned almost nowhere in the Postgres documentation. Since pgbouncer#878 this always throws an error, and putting `options` in `ignore_startup_parameters` wouldn't get rid of this error. So this should actually be considered a regression for the same reason as pgbouncer#907 was considered a regression (although a much less impactful one given we've only received two reports of this causing issues). Fixes pgbouncer#952
The `-c config=value` pattern has been supported in PgBouncer since PR #878. As described in #952 Postgres also allows to use the `--config=value`, which honestly seems more readable, but for some reason is mentioned almost nowhere in the Postgres documentation. Since #878 this always throws an error, and putting `options` in `ignore_startup_parameters` wouldn't get rid of this error. So this should actually be considered a regression for the same reason as #907 was considered a regression (although a much less impactful one given we've only received two reports of this causing issues). Fixes #952
It's allowed to specify any Postgres command line parameter in the
options
startup packet #867. This is commonly used to specify GUC values on connection
startup, using the
-c key=value
command line flag. I believe the main reasonthis is used instead of specifying the desired GUC as a startup parameter
directly is because libpq does not allow specifying arbitrary startup
parameters.
This PR adds support for configuring GUCs using the
-c
flag in the optionsstartup packet. While in theory people can specify any flag they want. In
practice the
-c
flag is the only one that I know is commonly used. Probablybecause it is the only one that's documented in the Postgres docs for
PGOPTIONS
. Any of the other command line flags are simply shorthands forspecific GUCs, and so even when those would be used they can be easily rewritten
using
-c
. So to keep parsing of the options startup parameter simple, thisonly supports the
-c
flag.Fixes #875