Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Allow resetting unknown custom GUCs with reserved prefixes.

  2. Allow ALTER SYSTEM to set unrecognized custom GUCs.

  3. Disallow setting bogus GUCs within an extension's reserved namespace.

  1. BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    PG Bug reporting form <noreply@postgresql.org> — 2025-06-20T12:24:20Z

    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.

  2. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Nathan Bossart <nathandbossart@gmail.com> — 2025-06-20T19:53:43Z

    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

  3. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-20T20:24:41Z

    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

  4. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Nathan Bossart <nathandbossart@gmail.com> — 2025-06-20T21:20:25Z

    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

  5. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Nathan Bossart <nathandbossart@gmail.com> — 2025-06-20T22:48:37Z

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

-- 
nathan

  6. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Nathan Bossart <nathandbossart@gmail.com> — 2025-06-25T16:06:05Z

    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

  7. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-07-22T02:27:24Z

    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

  8. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Nathan Bossart <nathandbossart@gmail.com> — 2025-07-24T16:30:49Z

    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

  9. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-07-24T21:06:57Z

    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

  10. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Nathan Bossart <nathandbossart@gmail.com> — 2025-07-25T14:15:17Z

    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

  11. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Nathan Bossart <nathandbossart@gmail.com> — 2025-07-28T21:15:04Z

    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

  12. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-28T21:27:50Z

    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

  13. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Nathan Bossart <nathandbossart@gmail.com> — 2025-07-30T19:13:16Z

    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

  14. Re: BUG #18964: `ALTER DATABASE ... RESET ...` fails to reset extension parameters that no longer exist

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-01T22:01:47Z

    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