Horton

Thread

Commits

  1. Fix MarkGUCPrefixReserved() to check all options.

  1. MarkGUCPrefixReserved() doesn't check all options

    Karina Litskevich <litskevichkarina@gmail.com> — 2023-07-06T09:17:44Z

    Hi hackers,

Ekaterina Sokolova and I have found a bug in PG 15. Since 88103567cb
MarkGUCPrefixReserved() is supposed not only reporting GUCs that haven't
been
defined by the extension, but also removing them. However, it removes them
in
a wrong way, so that a GUC that goes after the removed GUC is never checked.

To reproduce the bug add the following to the postgresql.conf

shared_preload_libraries = 'pg_stat_statements'
pg_stat_statements.nonexisting_option_1 = on
pg_stat_statements.nonexisting_option_2 = on
pg_stat_statements.nonexisting_option_3 = on
pg_stat_statements.nonexisting_option_4 = on

and start the server. In the logfile you'll see only first and third options
reported invalid and removed.

In master MarkGUCPrefixReserved() iterates a hash table, not an array as in
PG 15. I'm not sure whether it is safe to remove an entry from this hash
table
while iterating it, but at least I can't reproduce the bug on master.

I attached a bugfix for PG 15.

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/

  2. Re: MarkGUCPrefixReserved() doesn't check all options

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-07-06T10:07:02Z

    On 06/07/2023 12:17, Karina Litskevich wrote:
> Hi hackers,
> 
> Ekaterina Sokolova and I have found a bug in PG 15. Since 88103567cb 
> MarkGUCPrefixReserved() is supposed not only reporting GUCs that
> haven't been defined by the extension, but also removing them.
> However, it removes them in a wrong way, so that a GUC that goes
> after the removed GUC is never checked.
> 
> To reproduce the bug add the following to the postgresql.conf
> 
> shared_preload_libraries = 'pg_stat_statements'
> pg_stat_statements.nonexisting_option_1 = on
> pg_stat_statements.nonexisting_option_2 = on
> pg_stat_statements.nonexisting_option_3 = on
> pg_stat_statements.nonexisting_option_4 = on
> 
> and start the server. In the logfile you'll see only first and third
> options reported invalid and removed.

Good catch!

> In master MarkGUCPrefixReserved() iterates a hash table, not an array
> as in PG 15. I'm not sure whether it is safe to remove an entry from
> this hash table while iterating it, but at least I can't reproduce
> the bug on master.
Yes, it's safe to remove the current element, while scanning a hash 
table with hash_seq_init/search. See comment on hash_seq_init.

> I attached a bugfix for PG 15.

Applied, thanks!

-- 
Heikki Linnakangas
Neon (https://neon.tech)