ALTER DATABASE RESET with unexistent guc doesn't report an error

Started by Vitaly Davydov4 months ago6 messages
#1Vitaly Davydov
v.davydov@postgrespro.ru
1 attachment(s)

Dear Hackers,

I've found that ALTER DATABASE RESET with an unexistent guc does nothing without
error reporting.

ALTER DATABASE SET reports an error if guc doesn't exist:

alter database mydb set myparam to 10;

ERROR: unrecognized configuration parameter "myparam"

ALTER DATABASE RESET doesn't report an error at all:

alter database mydb reset myparam;

ALTER DATABASE

I think it is a wrong behaviour. I believe, ALTER DATABASE RESET should report
an error in this case. I've also think that ALTER DATABASE RESET TABLESPACE does
nothing without any error reporting. I've prepared a simple patch to handle this
case (master branch). It adds a check for guc existence with error reporting.

With best regards,
Vitaly

Attachments:

0001-Add-check-for-unexistent-guc-in-ALTER-DATABASE-RESET.patchtext/x-patchDownload
From dbe0897886144771d622d2aaff3c47ffec4f894e Mon Sep 17 00:00:00 2001
From: Vitaly Davydov <v.davydov@postgrespro.ru>
Date: Thu, 11 Sep 2025 11:13:04 +0300
Subject: [PATCH] Add check for unexistent guc in ALTER DATABASE RESET

Command ALTER DATABASE RESET doesn't not check that the guc exists. It
does nothing and completes without an error if the guc doesn't exist.
The patch adds the check with error reporting.
---
 src/backend/catalog/pg_db_role_setting.c | 5 +++++
 src/test/regress/expected/database.out   | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/src/backend/catalog/pg_db_role_setting.c b/src/backend/catalog/pg_db_role_setting.c
index 090fc07c28a..ca6fcac4625 100644
--- a/src/backend/catalog/pg_db_role_setting.c
+++ b/src/backend/catalog/pg_db_role_setting.c
@@ -151,6 +151,11 @@ AlterSetting(Oid databaseid, Oid roleid, VariableSetStmt *setstmt)
 
 		CatalogTupleInsert(rel, newtuple);
 	}
+	else
+	{
+		/* check guc existence and report an error if the guc doesn't exist */
+		GUCArrayDelete(NULL, setstmt->name);
+	}
 
 	InvokeObjectPostAlterHookArg(DbRoleSettingRelationId,
 								 databaseid, 0, roleid, false);
diff --git a/src/test/regress/expected/database.out b/src/test/regress/expected/database.out
index 4cbdbdf84d0..20a258eddd6 100644
--- a/src/test/regress/expected/database.out
+++ b/src/test/regress/expected/database.out
@@ -3,6 +3,7 @@ CREATE DATABASE regression_tbd
 ALTER DATABASE regression_tbd RENAME TO regression_utf8;
 ALTER DATABASE regression_utf8 SET TABLESPACE regress_tblspace;
 ALTER DATABASE regression_utf8 RESET TABLESPACE;
+ERROR:  unrecognized configuration parameter "tablespace"
 ALTER DATABASE regression_utf8 CONNECTION_LIMIT 123;
 -- Test PgDatabaseToastTable.  Doing this with GRANT would be slow.
 BEGIN;
@@ -11,6 +12,7 @@ SET datacl = array_fill(makeaclitem(10, 10, 'USAGE', false), ARRAY[5e5::int])
 WHERE datname = 'regression_utf8';
 -- load catcache entry, if nothing else does
 ALTER DATABASE regression_utf8 RESET TABLESPACE;
+ERROR:  unrecognized configuration parameter "tablespace"
 ROLLBACK;
 CREATE ROLE regress_datdba_before;
 CREATE ROLE regress_datdba_after;
-- 
2.34.1

#2Kirill Reshke
reshkekirill@gmail.com
In reply to: Vitaly Davydov (#1)
Re: ALTER DATABASE RESET with unexistent guc doesn't report an error

Hi!

On Thu, 11 Sept 2025 at 13:35, Vitaly Davydov <v.davydov@postgrespro.ru> wrote:

I've also think that ALTER DATABASE RESET TABLESPACE does
nothing without any error reporting.

I can see that ALTER DATABASE RESET TABLESPACE indeed does not change
dattablespace.
Documentation also lacks any information about support of something
like this. [0]https://www.postgresql.org/docs/current/sql-alterdatabase.html

This test case looks like just an oversight of 0844b3968985
I think we can remove "support" for ALTER DATABASE RESET TABLESPACE.

LGTM

[0]: https://www.postgresql.org/docs/current/sql-alterdatabase.html

--
Best regards,
Kirill Reshke

#3Álvaro Herrera
alvherre@kurilemu.de
In reply to: Kirill Reshke (#2)
Re: ALTER DATABASE RESET with unexistent guc doesn't report an error

On 2025-Sep-11, Kirill Reshke wrote:

I think we can remove "support" for ALTER DATABASE RESET TABLESPACE.

What about ALTER USER RESET TABLESPACE?

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Computing is too important to be left to men." (Karen Spärck Jones)

#4Kirill Reshke
reshkekirill@gmail.com
In reply to: Álvaro Herrera (#3)
Re: ALTER DATABASE RESET with unexistent guc doesn't report an error

On Thu, 11 Sept 2025 at 19:27, Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2025-Sep-11, Kirill Reshke wrote:

I think we can remove "support" for ALTER DATABASE RESET TABLESPACE.

What about ALTER USER RESET TABLESPACE?

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Computing is too important to be left to men." (Karen Spärck Jones)

Does this feature work?

```
reshke=# alter user u1 set tablespace to tb2;
ERROR: unrecognized configuration parameter "tablespace"
```

Also this:

```
reshke=# alter table ss reset tablespace ;
ERROR: syntax error at or near "tablespace"
LINE 1: alter table ss reset tablespace ;
^
```

"tablespace" is tab-completed after 'alter table ss reset <TAB>' which
is a least strange

--
Best regards,
Kirill Reshke

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Álvaro Herrera (#3)
Re: ALTER DATABASE RESET with unexistent guc doesn't report an error

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:

On 2025-Sep-11, Kirill Reshke wrote:

I think we can remove "support" for ALTER DATABASE RESET TABLESPACE.

What about ALTER USER RESET TABLESPACE?

Yeah, I think you're right. The complaint is fundamentally that
these two cases behave differently:

regression=# ALTER DATABASE regression RESET bogus;
ERROR: unrecognized configuration parameter "bogus"
regression=# ALTER DATABASE postgres RESET bogus;
ALTER DATABASE

the unobvious-to-the-user reason being that "regression" has a
pg_db_role_setting entry and "postgres" does not. But there's
also no error for

regression=# ALTER USER postgres RESET bogus;
ALTER ROLE

and by the same logic there should be. I think though that
the proposed patch addresses both cases.

Looking at the patch, the delta in database.out raises
another question:

ALTER DATABASE regression_tbd RENAME TO regression_utf8;
ALTER DATABASE regression_utf8 SET TABLESPACE regress_tblspace;
ALTER DATABASE regression_utf8 RESET TABLESPACE;
+ERROR: unrecognized configuration parameter "tablespace"
ALTER DATABASE regression_utf8 CONNECTION_LIMIT 123;

The author of this bit of test script evidently thought that
ALTER ... RESET TABLESPACE is the inverse of ALTER ... SET TABLESPACE,
and what we are seeing is that it is not. That may be a bug in
itself, but it's not what Vitaly is on about, IIUC.

regards, tom lane

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#5)
Re: ALTER DATABASE RESET with unexistent guc doesn't report an error

I wrote:

Looking at the patch, the delta in database.out raises
another question:
ALTER DATABASE regression_tbd RENAME TO regression_utf8;
ALTER DATABASE regression_utf8 SET TABLESPACE regress_tblspace;
ALTER DATABASE regression_utf8 RESET TABLESPACE;
+ERROR: unrecognized configuration parameter "tablespace"
ALTER DATABASE regression_utf8 CONNECTION_LIMIT 123;
The author of this bit of test script evidently thought that
ALTER ... RESET TABLESPACE is the inverse of ALTER ... SET TABLESPACE,
and what we are seeing is that it is not. That may be a bug in
itself, but it's not what Vitaly is on about, IIUC.

That was in fact a test bug, now corrected at 4adb0380b.
I've substituted a more on-point test case, wordsmithed the
comment a little bit, and pushed it.

Thanks for the report and patch!

regards, tom lane