Обсуждение: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

The following bug has been logged on the website:

Bug reference:      18964
Logged by:          Mert Alev
Email address:      mert@futo.org
PostgreSQL version: 17.5
Operating system:   Debian 12
Description:

My project uses PostgreSQL (default 14, but later versions are supported as
well) with an extension where one of its parameters was removed in a later
version. I had applied a non-default value for this parameter using `ALTER
DATABASE immich SET vchordrq.prewarm_dim = '512,640,768,1024,1152,1536';`.
After upgrading the extension to a version that no longer includes this
parameter, admins reported that each connection emitted the following
warning:
```
2025-06-18 23:32:46.765 CEST [49] WARNING:  invalid configuration parameter
name "vchordrq.prewarm_dim"
2025-06-18 23:32:46.765 CEST [49] DETAIL:  "vchordrq" is a reserved prefix.
```
As a solution, I added a migration that removes the now-redundant parameter
with `ALTER DATABASE immich RESET vchordrq.prewarm_dim;`. However, while
this worked for our CI that uses Postgres 14 and for admins using 14, we
began getting reports from admins using 15, 16 and 17 that the migration
fails with the following error:
```
PostgresError: invalid configuration parameter name "vchordrq.prewarm_dim"
{
  severity_local: 'ERROR',
  severity: 'ERROR',
  code: '42602',
  detail: '"vchordrq" is a reserved prefix.',
  file: 'guc.c',
  line: '1153',
  routine: 'assignable_custom_variable_name'
}
```
While not a PG developer, I think this is likely due to the change in commit
[88103567c](https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=88103567c)
that was part of the 15 release. It makes for a tricky situation where it
seems the options to remove this parameter are:
1. Write directly to `pg_catalog.pg_db_role_setting`, removing this
parameter from the `setconfig` array
2. Run `ALTER DATABASE immich RESET ALL;`, purging all database-level
parameter configuration along with it
3. Make a backup, then remove the relevant line that applies this parameter
in the backup file before restoring
There may be other approaches as well, but all of them are rather cumbersome
and error-prone. My solution was to write a transaction that first queries
the `setconfig` parameters, then runs `RESET ALL`, then re-applies all
parameters except `vchordrq.prewarm_dim` using the queried data.
It would be great if `RESET`ing the parameter still worked in this
situation, even if `SET`ing it does not.

Thanks for the report.

On Fri, Jun 20, 2025 at 12:24:20PM +0000, PG Bug reporting form wrote:
> My project uses PostgreSQL (default 14, but later versions are supported as
> well) with an extension where one of its parameters was removed in a later
> version. I had applied a non-default value for this parameter using `ALTER
> DATABASE immich SET vchordrq.prewarm_dim = '512,640,768,1024,1152,1536';`.
> After upgrading the extension to a version that no longer includes this
> parameter, admins reported that each connection emitted the following
> warning:
> ```
> 2025-06-18 23:32:46.765 CEST [49] WARNING:  invalid configuration parameter
> name "vchordrq.prewarm_dim"
> 2025-06-18 23:32:46.765 CEST [49] DETAIL:  "vchordrq" is a reserved prefix.
> ```
> As a solution, I added a migration that removes the now-redundant parameter
> with `ALTER DATABASE immich RESET vchordrq.prewarm_dim;`. However, while
> this worked for our CI that uses Postgres 14 and for admins using 14, we
> began getting reports from admins using 15, 16 and 17 that the migration
> fails with the following error:
> ```
> PostgresError: invalid configuration parameter name "vchordrq.prewarm_dim"
> {
>   severity_local: 'ERROR',
>   severity: 'ERROR',
>   code: '42602',
>   detail: '"vchordrq" is a reserved prefix.',
>   file: 'guc.c',
>   line: '1153',
>   routine: 'assignable_custom_variable_name'
> }
> ```
>
> [...]
>
> It would be great if `RESET`ing the parameter still worked in this
> situation, even if `SET`ing it does not.

It seems like we could at least allow superusers and folks with existing
privileges on removed parameters to RESET in this case.  IIUC we'd need to
teach validate_option_array_item() to treat unknown custom parameters like
they're known (for calls from GUCArrayDelete()).

On the extension side, another option could be to leave the parameter
defined as a placeholder so that Postgres knows about it.

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Jun 20, 2025 at 12:24:20PM +0000, PG Bug reporting form wrote:
>> As a solution, I added a migration that removes the now-redundant parameter
>> with `ALTER DATABASE immich RESET vchordrq.prewarm_dim;`. However, while
>> this worked for our CI that uses Postgres 14 and for admins using 14, we
>> began getting reports from admins using 15, 16 and 17 that the migration
>> fails with the following error:
>> ```
>> PostgresError: invalid configuration parameter name "vchordrq.prewarm_dim"
>> {
>> severity_local: 'ERROR',
>> severity: 'ERROR',
>> code: '42602',
>> detail: '"vchordrq" is a reserved prefix.',
>> 
>> It would be great if `RESET`ing the parameter still worked in this
>> situation, even if `SET`ing it does not.

> It seems like we could at least allow superusers and folks with existing
> privileges on removed parameters to RESET in this case.

The hazard we need to pay attention to here is someone removing a
parameter setting that they shouldn't have had privilege to do.
In particular, if a non-superuser can RESET a SUSET-or-above
parameter, that's bad.

In the case at hand, where the prefix is known but the parameter
isn't, I think we could safely assume that the setting is either
a mistake (probably installed while the extension wasn't loaded)
or the described case of a parameter the extension no longer
uses.  Either way it seems safe to allow RESET with suitable
privileges on the target DB and/or role.

If the prefix is not known, then we're really flying blind.
It looks like we assume the parameter might be SUSET and therefore
allow both ALTER DB/ROLE SET and RESET only to superusers.
I'm hesitant to relax that case.

            regards, tom lane

On Fri, Jun 20, 2025 at 04:24:41PM -0400, Tom Lane wrote:
> In the case at hand, where the prefix is known but the parameter
> isn't, I think we could safely assume that the setting is either
> a mistake (probably installed while the extension wasn't loaded)
> or the described case of a parameter the extension no longer
> uses.  Either way it seems safe to allow RESET with suitable
> privileges on the target DB and/or role.

I'm a little hesitant to consider opening this up too much.  For example,
what if someone accidentally downgrades the library temporarily and a GUC
definition disappears, or one version of the library removes a parameter
and a future one adds it back (due to user backlash)?  Maybe those
situations are too contrived/unlikely to worry about, though.

> If the prefix is not known, then we're really flying blind.
> It looks like we assume the parameter might be SUSET and therefore
> allow both ALTER DB/ROLE SET and RESET only to superusers.
> I'm hesitant to relax that case.

IIUC we also check pg_parameter_acl in this case, but I agree that we don't
want to relax this any further.

-- 
nathan

Here is a fragile proof-of-concept patch that demonstrates roughly what I
had in mind.

-- 
nathan

Here is a tidied-up version of the patch.  It's still quite fragile, but
AFAICT to do any better we'd need to return more context from
find_option(), and I don't think we can change that one on the
back-branches since it's exported.  I assume we don't want find_option() to
create a placeholder in this case, either.  I'll continue to look around
for a better solution...

If we wanted to open up RESET in this case to everyone with suitable
privileges on the target DB and/or role (as proposed by Tom upthread [0]),
I think we'd just need to replace the "return false;" under find_option()
to "return reset_custom;".

[0] https://postgr.es/m/2474668.1750451081%40sss.pgh.pa.us

-- 
nathan

On Wed, 2025-06-25 at 11:06 -0500, Nathan Bossart wrote:
> Here is a tidied-up version of the patch.  It's still quite fragile, but
> AFAICT to do any better we'd need to return more context from
> find_option(), and I don't think we can change that one on the
> back-branches since it's exported.  I assume we don't want find_option() to
> create a placeholder in this case, either.  I'll continue to look around
> for a better solution...

I tested your patch, and it works as expected for

  ALTER DATABASE ... RESET
  ALTER ROLE ... RESET
  ALTER ROLE ... IN DATABASE ... RESET

There is still one piece missing in my opinion:

  ALTER SYSTEM RESET testext.swap_limit;
  ERROR:  invalid configuration parameter name "testext.swap_limit"
  DETAIL:  "testext" is a reserved prefix.

I think that this case should work like the others.

I'd like to see regression tests for this, but I am not sure how
to best devise them.
One idea would be to stick them into the regression tests of some
contrib module, even though it is not the perfect place.
Perhaps a sequence like this:

  DO $$BEGIN EXECUTE format(
    'ALTER DATABASE %I SET pg_trgm.somevar = 42',
    current_catalog); END$$;

  LOAD 'pg_trgm';
  WARNING:  invalid configuration parameter name "pg_trgm.somevar", removing it
  DETAIL:  "pg_trgm" is now a reserved prefix.

  DO $$BEGIN EXECUTE format(
    'ALTER DATABASE %I RESET pg_trgm.somevar',
    current_catalog); END$$;

  \drds

> If we wanted to open up RESET in this case to everyone with suitable
> privileges on the target DB and/or role (as proposed by Tom upthread [0]),
> I think we'd just need to replace the "return false;" under find_option()
> to "return reset_custom;".
>
> [0] https://postgr.es/m/2474668.1750451081%40sss.pgh.pa.us

I have no strong opinion about that.
I'd say it is good enough to allow superusers to reset such parameters.

Yours,
Laurenz Albe

On Tue, Jul 22, 2025 at 04:27:24AM +0200, Laurenz Albe wrote:
> I tested your patch, and it works as expected for

Thanks for reviewing.

> There is still one piece missing in my opinion:
> 
>   ALTER SYSTEM RESET testext.swap_limit;
>   ERROR:  invalid configuration parameter name "testext.swap_limit"
>   DETAIL:  "testext" is a reserved prefix.
> 
> I think that this case should work like the others.

Good catch.  This should be fixed in v3.

> I'd like to see regression tests for this, but I am not sure how
> to best devise them.
> One idea would be to stick them into the regression tests of some
> contrib module, even though it is not the perfect place.

Added in v3.

-- 
nathan

On Thu, 2025-07-24 at 11:30 -0500, Nathan Bossart wrote:
> On Tue, Jul 22, 2025 at 04:27:24AM +0200, Laurenz Albe wrote:
>
> > There is still one piece missing in my opinion:
> >
> >   ALTER SYSTEM RESET testext.swap_limit;
> >   ERROR:  invalid configuration parameter name "testext.swap_limit"
> >   DETAIL:  "testext" is a reserved prefix.
> >
> > I think that this case should work like the others.
>
> Good catch.  This should be fixed in v3.

Check.

> > I'd like to see regression tests for this, but I am not sure how
> > to best devise them.
> > One idea would be to stick them into the regression tests of some
> > contrib module, even though it is not the perfect place.
>
> Added in v3.

Great!  The regression test works fine on my Linux machine.

> --- /dev/null
> +++ b/contrib/auto_explain/sql/alter_reset.sql
> [...]
> +SELECT current_database() AS datname \gset
> +CREATE ROLE regress_ae_role;
> +
> +ALTER DATABASE :datname SET auto_explain.bogus = 1;
> +ALTER ROLE regress_ae_role SET auto_explain.bogus = 1;
> +ALTER ROLE regress_ae_role IN DATABASE :datname SET auto_explain.bogus = 1;
> +ALTER SYSTEM SET auto_explain.bogus = 1;
> +
> +LOAD 'auto_explain';
> +
> +ALTER DATABASE :datname RESET auto_explain.bogus;
> +ALTER ROLE regress_ae_role RESET auto_explain.bogus;
> +ALTER ROLE regress_ae_role IN DATABASE :datname RESET auto_explain.bogus;

That is perhaps a rediculous quibble, but shouldn't that be :"datname"?
I guess the regression test database will always have a proper name...

Anyway, I'll mark the patch as "ready for committer".

Yours,
Laurenz Albe

On Thu, Jul 24, 2025 at 11:06:57PM +0200, Laurenz Albe wrote:
> On Thu, 2025-07-24 at 11:30 -0500, Nathan Bossart wrote:
>> +ALTER ROLE regress_ae_role IN DATABASE :datname RESET auto_explain.bogus;
> 
> That is perhaps a rediculous quibble, but shouldn't that be :"datname"?
> I guess the regression test database will always have a proper name...

You're right, it probably should be quoted.

> Anyway, I'll mark the patch as "ready for committer".

Thanks.  I'd like to get this fixed in the August releases, so I'm planning
to commit this sometime late next week, barring additional feedback or
objections.

-- 
nathan

On Fri, Jul 25, 2025 at 09:15:17AM -0500, Nathan Bossart wrote:
> On Thu, Jul 24, 2025 at 11:06:57PM +0200, Laurenz Albe wrote:
>> Anyway, I'll mark the patch as "ready for committer".
> 
> Thanks.  I'd like to get this fixed in the August releases, so I'm planning
> to commit this sometime late next week, barring additional feedback or
> objections.

I looked into how easily this back-patched to v15 (where commit 88103567c
was added), and I noticed that the ALTER SYSTEM code looks a bit different
on v15 and v16.  Furthermore, I didn't see a simple way to fix it on those
versions without first back-patching commit 2d870b4.  From the commit
message, it looks like Tom didn't back-patch it at the time due to a lack
of complaints.  I'm currently thinking we should first back-patch that one
to at least v15, if not all supported versions, before applying my proposed
patch.  Thoughts?

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> I looked into how easily this back-patched to v15 (where commit 88103567c
> was added), and I noticed that the ALTER SYSTEM code looks a bit different
> on v15 and v16.  Furthermore, I didn't see a simple way to fix it on those
> versions without first back-patching commit 2d870b4.  From the commit
> message, it looks like Tom didn't back-patch it at the time due to a lack
> of complaints.  I'm currently thinking we should first back-patch that one
> to at least v15, if not all supported versions, before applying my proposed
> patch.  Thoughts?

No objection here.  Now that that's a couple years old, it should have
had enough time to bake.

            regards, tom lane

On Mon, Jul 28, 2025 at 05:27:50PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> I looked into how easily this back-patched to v15 (where commit 88103567c
>> was added), and I noticed that the ALTER SYSTEM code looks a bit different
>> on v15 and v16.  Furthermore, I didn't see a simple way to fix it on those
>> versions without first back-patching commit 2d870b4.  From the commit
>> message, it looks like Tom didn't back-patch it at the time due to a lack
>> of complaints.  I'm currently thinking we should first back-patch that one
>> to at least v15, if not all supported versions, before applying my proposed
>> patch.  Thoughts?
> 
> No objection here.  Now that that's a couple years old, it should have
> had enough time to bake.

It applies relatively cleanly down to v15.  The only thing I had to change
was to replace guc_free() with free() on v15.  I was a little worried about
changing the signature of check_GUC_name_for_parameter_acl(), but
codesearch.debian.net doesn't show any outside uses.

-- 
nathan

Committed.

On Wed, Jul 30, 2025 at 02:13:16PM -0500, Nathan Bossart wrote:
> It applies relatively cleanly down to v15.  The only thing I had to change
> was to replace guc_free() with free() on v15.  I was a little worried about
> changing the signature of check_GUC_name_for_parameter_acl(), but
> codesearch.debian.net doesn't show any outside uses.

I ended up skipping the back-patch of commit 2d870b4 to avoid the ABI
breakage.  That means ALTER SYSTEM remains broken for this use-case on v15
and v16, but IIUC it's already pretty broken in this area, so I'm not sure
we care too much.

-- 
nathan