Horton

Thread

Commits

  1. Avoid throwing away the error message in syncrep_yyerror.

  2. Return yyparse() result not via global variable

  1. synchronous_standby_names parser discards errors

    Robert Haas <robertmhaas@gmail.com> — 2025-07-24T17:58:14Z

    Since Peter Eisentraut's commit
473a575e05979b4dbb28b3f2544f4ec8f184ce65 on January 25 of this year,
attempting to set synchronous_standby_names to a value that doesn't
parse results in a generic and uninformative error message:

[robert.haas ~]$ postgres -c synchronous_standby_names='"foo'
2025-07-24 17:16:05.769 GMT [35558] FATAL:  invalid value for
parameter "synchronous_standby_names": ""foo"
2025-07-24 17:16:05.769 GMT [35558] DETAIL:
"synchronous_standby_names" parser failed.

This happens because syncrep_yyerror() claims to stuff the error
message into *syncrep_parse_error_msg_p but does not actually do so.
The attached patch fixes it:

[robert.haas ~]$ postgres -c synchronous_standby_names='"foo'
2025-07-24 17:16:22.527 GMT [35962] FATAL:  invalid value for
parameter "synchronous_standby_names": ""foo"
2025-07-24 17:16:22.527 GMT [35962] DETAIL:  unterminated quoted
identifier at end of input

Apparently, our tests do not exercise this parser's error cases. I
looked into adding a test but I don't see a real easy place to add
one. If we had testing for messages like "WAL archival cannot be
enabled when \"wal_level\" is \"minimal\"" then it would make sense to
test this kind of thing in the same place, but we don't. Because the
setting is PGC_SIGHUP, we can't just include a SET command in the main
regression tests, either. I don't want to spend the time to upgrade
our testing infrastructure to catch future instances of this problem
right now, and therefore propose to just commit this fix and move on.

Thanks to my colleague Jacob Champion for helping track this down.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  2. Re: synchronous_standby_names parser discards errors

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-24T18:14:18Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Since Peter Eisentraut's commit
> 473a575e05979b4dbb28b3f2544f4ec8f184ce65 on January 25 of this year,
> attempting to set synchronous_standby_names to a value that doesn't
> parse results in a generic and uninformative error message:

Oops.

> Apparently, our tests do not exercise this parser's error cases. I
> looked into adding a test but I don't see a real easy place to add
> one. If we had testing for messages like "WAL archival cannot be
> enabled when \"wal_level\" is \"minimal\"" then it would make sense to
> test this kind of thing in the same place, but we don't. Because the
> setting is PGC_SIGHUP, we can't just include a SET command in the main
> regression tests, either. I don't want to spend the time to upgrade
> our testing infrastructure to catch future instances of this problem
> right now, and therefore propose to just commit this fix and move on.

Agreed, the cost/benefit ratio for creating a test case doesn't
seem attractive.

			regards, tom lane