Thread: Forgive trailing semicolons inside of config files
This has been a long-standing annoyance of mine. Who hasn't done something like this?:
psql> SET random_page_cost = 2.5;
(do some stuff, realize that rpc was too high)
Let's put that inside of postgresql.conf:
#------------------------------------------------------------------------------
# CUSTOMIZED OPTIONS
#------------------------------------------------------------------------------
# Add settings for extensions here
random_page_cost = 2.5;
Boom! Server will not start. Surely, we can be a little more liberal in what we accept? Attached patch allows a single trailing semicolon to be silently discarded. As this parsing happens before the logging collector starts up, the error about the semicolon is often buried somewhere in a separate logfile or journald - so let's just allow postgres to start up since there is no ambiguity about what random_page_cost (or any other GUC) is meant to be set to.
I also considered doing an additional ereport(LOG) when we find one, but seemed better on reflection to simply ignore it.
Cheers,
Greg
Attachment
On Tue, 11 Jul 2023 at 10:43, Greg Sabino Mullane <htamfids@gmail.com> wrote:
This has been a long-standing annoyance of mine. Who hasn't done something like this?:psql> SET random_page_cost = 2.5;(do some stuff, realize that rpc was too high)Let's put that inside of postgresql.conf:#------------------------------------------------------------------------------# CUSTOMIZED OPTIONS#------------------------------------------------------------------------------
# Add settings for extensions hererandom_page_cost = 2.5;Boom! Server will not start. Surely, we can be a little more liberal in what we accept? Attached patch allows a single trailing semicolon to be silently discarded. As this parsing happens before the logging collector starts up, the error about the semicolon is often buried somewhere in a separate logfile or journald - so let's just allow postgres to start up since there is no ambiguity about what random_page_cost (or any other GUC) is meant to be set to.
Please, no!
There is no end to accepting sloppy syntax. What next, allow "SET random_page_cost = 2.5;" (with or without semicolon) in config files?
I'd be more interested in improvements in visibility of errors. For example, maybe if I try to start the server and there is a config file problem, I could somehow get a straightforward error message right in the terminal window complaining about the line of the configuration which is wrong.
Or maybe there could be a "check configuration" subcommand which checks the configuration. If it's fine, say so and set a flag saying the server is clear to be started/restarted. If not, give useful error messages and don't set the flag. Then make the start/restart commands only do their thing if the "config OK" flag is set. Make sure that editing the configuration clears the flag (or have 2 copies of the configuration, copied over by the "check" subcommand: one for editing, one for running with).
This might properly belong outside of Postgres itself, I don't know. But I think it would be way more useful than a potentially never-ending series of patches to liberalize the config parser.
Isaac Morland <isaac.morland@gmail.com> writes:
> On Tue, 11 Jul 2023 at 10:43, Greg Sabino Mullane <htamfids@gmail.com>
>> # Add settings for extensions here
>> random_page_cost = 2.5;
>>
>> Boom! Server will not start. Surely, we can be a little more liberal in
>> what we accept? Attached patch allows a single trailing semicolon to be
>> silently discarded.
> Please, no!
I agree. Allowing this would create huge confusion about whether it's
EOL or semicolon that ends a config file entry. If you can write a
semicolon, then why not spread an entry across lines, or write
multiple entries on one line?
It seems possible that someday we might want to convert over to
semicolon-is-end-of-entry precisely to allow such cases. But
I think that if/when we do that, it should be a flag day where you
*must* change to the new syntax. (We did exactly that in pgbench
scripts some years ago, and people didn't complain too much.)
> Or maybe there could be a "check configuration" subcommand which checks the
> configuration.
We have such a thing, see the pg_file_settings view.
regards, tom lane
On Tue, Jul 11, 2023 at 11:04 AM Isaac Morland <isaac.morland@gmail.com> wrote:
Please, no!There is no end to accepting sloppy syntax. What next, allow "SET random_page_cost = 2.5;" (with or without semicolon) in config files?
Well yes, there is an end. A single, trailing semicolon. Full stop. It's not a slippery slope in which we end up asking the AI parser to interpret our haikus to derive the actual value. The postgresql.conf file is not some finicky YAML/JSON beast - we already support some looseness in quoting or not quoting values, optional whitespace, etc. Think of the trailing semicolon as whitespace, if you like. You can see from the patch that this does not replace EOL/EOF.
I'd be more interested in improvements in visibility of errors. For example, maybe if I try to start the server and there is a config file problem, I could somehow get a straightforward error message right in the terminal window complaining about the line of the configuration which is wrong.
That ship has long since sailed. We already send a detailed error message with the line number, but in today's world of "service start", "systemctl start", and higher level of control such as Patroni and Kubernetes, getting things to show in a terminal window isn't happening. We can't work around 2>&1.
Or maybe there could be a "check configuration" subcommand which checks the configuration.
There are things inside of Postgres once it has started, but yeah, something akin to visudo would be nice for editing config files.
But I think it would be way more useful than a potentially never-ending series of patches to liberalize the config parser.
It's a single semicolon, not a sign of the parser apocalypse. I've no plans for future enhancements, but if they do no harm and make Postgres more user friendly, I will support them.
Cheers,
Greg
Hi, On 2023-07-11 12:21:46 -0400, Greg Sabino Mullane wrote: > On Tue, Jul 11, 2023 at 11:04 AM Isaac Morland <isaac.morland@gmail.com> > > Or maybe there could be a "check configuration" subcommand which checks > > the configuration. > > > > There are things inside of Postgres once it has started, but yeah, > something akin to visudo would be nice for editing config files. You can also do it kind-of-reasonably with the server binary, like: PGDATA=/srv/dev/pgdev-dev /path/to/postgres -C server_version; echo $? > > I'd be more interested in improvements in visibility of errors. For > > example, maybe if I try to start the server and there is a config file > > problem, I could somehow get a straightforward error message right in the > > terminal window complaining about the line of the configuration which is > > wrong. > > > > That ship has long since sailed. We already send a detailed error message > with the line number, but in today's world of "service start", "systemctl > start", and higher level of control such as Patroni and Kubernetes, getting > things to show in a terminal window isn't happening. We can't work around > 2>&1. At least with debian's infrastructure, both systemctl start and reload show errors reasonably well: start with broken config: Jul 11 19:13:40 awork3 systemd[1]: Starting postgresql@15-test.service - PostgreSQL Cluster 15-test... Jul 11 19:13:40 awork3 postgresql@15-test[3217452]: Error: invalid line 3 in /var/lib/postgresql/15/test/postgresql.auto.conf:dd Jul 11 19:13:40 awork3 systemd[1]: postgresql@15-test.service: Can't open PID file /run/postgresql/15-test.pid (yet?) afterstart: No such file or directory reload with broken config: Jul 11 19:10:38 awork3 systemd[1]: Reloading postgresql@15-test.service - PostgreSQL Cluster 15-test... Jul 11 19:10:38 awork3 postgresql@15-test[3217175]: Error: invalid line 3 in /var/lib/postgresql/15/test/postgresql.auto.conf:dd Jul 11 19:10:38 awork3 systemd[1]: postgresql@15-test.service: Control process exited, code=exited, status=1/FAILURE Jul 11 19:10:38 awork3 systemd[1]: Reload failed for postgresql@15-test.service - PostgreSQL Cluster 15-test. However: It looks like that's all implemented in debian specific tooling, rather than PG itself. Oops. Looks like we could make this easier in core postgres by adding one more sd_notify() call, with something like STATUS=reload failed due to syntax error in file "/srv/dev/pgdev-dev/postgresql.conf" line 821, near end of line Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Looks like we could make this easier in core postgres by adding one more
> sd_notify() call, with something like
> STATUS=reload failed due to syntax error in file "/srv/dev/pgdev-dev/postgresql.conf" line 821, near end of line
Seems reasonable to investigate. The systemd camel's nose is already
inside our tent, so we might as well consider more systemd-specific
hacks to improve the user experience. But it seems like we'd need
some careful thought about just which messages to report.
regards, tom lane