allow_system_table_mods stuff

Started by Peter Eisentrautover 6 years ago25 messages

peter.eisentraut@2ndquadrant.com

over 6 years ago

3 attachment(s)

After the earlier thread [0]/messages/by-id/e49f825b-fb25-0bc8-8afc-d5ad895c7975@2ndquadrant.com that dealt with ALTER TABLE on system
catalogs, I took a closer look at the allow_system_table_mods setting.
I found a few oddities, and it seems there is some room for improvement.

Attached are some patches to get the discussion rolling: One patch makes
allow_system_table_mods settable at run time by superuser, the second
one is a test suite that documents the current behavior that I gathered
after analyzing the source code, the third one removes some code that
was found useless by the tests. (The first patch might be useful on its
own, but right now it's just to facilitate the test suite.)

Some observations:

- For the most part, a_s_t_m establishes an additional level of access
control on top of superuserdom for doing DDL on system catalogs. That
seems like a useful definition.

- But enabling a_s_t_m also allows a non-superuser to do DML on system
catalogs. That seems like an entirely unrelated and surprising behavior.

- Some checks are redundant with the pinning concept of the dependency
system. For example, you can't drop a system catalog even with a_s_t_m
on. That seems useful, of course, but as a result there is a bit of
dead or useless code around. (The dependency system is newer than a_s_t_m.)

- The source code comments indicate that SET STATISTICS on system
catalogs is supposed to be allowed without a_s_t_m, but it actually
doesn't work.

Proposals and discussion points:

- Having a test suite like this seems useful.

- The behavior that a_s_t_m allows non-superusers to do DML on system
catalogs should be removed. (Regular permissions can be used for that.)

- Things that are useful in normal use, for example SET STATISTICS, some
or all reloptions, should always be allowed (subject to other access
control).

- There is currently no support in pg_dump to preserve any of those
changes. Maybe that's not a big problem.

- Dead code or code that is redundant with pinning should be removed.

Any other thoughts?

[0]: /messages/by-id/e49f825b-fb25-0bc8-8afc-d5ad895c7975@2ndquadrant.com
/messages/by-id/e49f825b-fb25-0bc8-8afc-d5ad895c7975@2ndquadrant.com

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

v1-0001-Change-allow_system_table_mods-to-SUSET.patchtext/plain; charset=UTF-8; name=v1-0001-Change-allow_system_table_mods-to-SUSET.patch; x-mac-creator=0; x-mac-type=0Download

From d9143b1ca4d00ac90b1cd8cd6eefba86cb4685b6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 21 Jun 2019 10:24:03 +0200
Subject: [PATCH v1 1/3] Change allow_system_table_mods to SUSET

---
 doc/src/sgml/config.sgml     | 2 +-
 src/backend/utils/misc/guc.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 84341a30e5..de19519b20 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9298,7 +9298,7 @@ <title>Developer Options</title>
        <para>
         Allows modification of the structure of system tables.
         This is used by <command>initdb</command>.
-        This parameter can only be set at server start.
+        Only superusers can change this setting.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1208eb9a68..025dcc1d87 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1772,7 +1772,7 @@ static struct config_bool ConfigureNamesBool[] =
 	},
 
 	{
-		{"allow_system_table_mods", PGC_POSTMASTER, DEVELOPER_OPTIONS,
+		{"allow_system_table_mods", PGC_SUSET, DEVELOPER_OPTIONS,
 			gettext_noop("Allows modifications of the structure of system tables."),
 			NULL,
 			GUC_NOT_IN_SAMPLE
-- 
2.22.0

v1-0002-Add-tests-for-allow_system_table_mods.patchtext/plain; charset=UTF-8; name=v1-0002-Add-tests-for-allow_system_table_mods.patch; x-mac-creator=0; x-mac-type=0Download

From 97515b52944b663c068a8dee763e41855739fd56 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 21 Jun 2019 10:24:03 +0200
Subject: [PATCH v1 2/3] Add tests for allow_system_table_mods

---
 .../regress/expected/alter_system_table.out   | 147 ++++++++++++++++
 src/test/regress/parallel_schedule            |   3 +
 src/test/regress/sql/alter_system_table.sql   | 165 ++++++++++++++++++
 3 files changed, 315 insertions(+)
 create mode 100644 src/test/regress/expected/alter_system_table.out
 create mode 100644 src/test/regress/sql/alter_system_table.sql

diff --git a/src/test/regress/expected/alter_system_table.out b/src/test/regress/expected/alter_system_table.out
new file mode 100644
index 0000000000..2550e75b2c
--- /dev/null
+++ b/src/test/regress/expected/alter_system_table.out
@@ -0,0 +1,147 @@
+CREATE USER regress_user_ast;
+SET allow_system_table_mods = off;
+-- create new table in pg_catalog
+CREATE TABLE pg_catalog.test (a int);
+ERROR:  permission denied to create "pg_catalog.test"
+DETAIL:  System catalog modifications are currently disallowed.
+-- anyarray column
+CREATE TABLE t1x (a int, b anyarray);
+ERROR:  column "b" has pseudo-type anyarray
+-- index on system catalog
+CREATE INDEX ON pg_class (relnamespace);
+ERROR:  permission denied: "pg_class" is a system catalog
+-- index on system catalog
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+ERROR:  permission denied: "pg_namespace" is a system catalog
+-- write to system catalog table as superuser
+-- (does not need allow_system_table_mods)
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 0, 'foo');
+-- write to system catalog table as normal user
+GRANT INSERT ON pg_description TO regress_user_ast;
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 1, 'foo');
+ERROR:  permission denied for table pg_description
+RESET ROLE;
+-- policy on system catalog
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+ERROR:  permission denied: "pg_description" is a system catalog
+-- reserved schema name
+CREATE SCHEMA pg_foo;
+ERROR:  unacceptable schema name "pg_foo"
+DETAIL:  The prefix "pg_" is reserved for system schemas.
+-- drop system table
+DROP TABLE pg_description;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- truncate of system table
+TRUNCATE pg_description;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- rename column of system table
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- ATSimplePermissions()
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- SET STATISTICS
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- foreign key referencing catalog
+BEGIN;
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ERROR:  permission denied: "pg_description" is a system catalog
+ROLLBACK;
+-- RangeVarCallbackOwnsRelation()
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+ERROR:  permission denied: "pg_description" is a system catalog
+-- RangeVarCallbackForAlterRelation()
+ALTER TABLE pg_description RENAME TO pg_comment;
+ERROR:  permission denied: "pg_description" is a system catalog
+ALTER TABLE pg_description SET SCHEMA public;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+ERROR:  unacceptable tablespace name "pg_foo"
+DETAIL:  The prefix "pg_" is reserved for system tablespaces.
+-- triggers
+CREATE FUNCTION tf1() RETURNS trigger
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RETURN NULL;
+END $$;
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ERROR:  permission denied: "pg_description" is a system catalog
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+ERROR:  permission denied: "pg_description" is a system catalog
+--DROP TRIGGER t2 ON pg_description;
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ERROR:  permission denied: "pg_description" is a system catalog
+ALTER RULE r1 ON pg_description RENAME TO r2;
+ERROR:  permission denied: "pg_description" is a system catalog
+--DROP RULE r2 ON pg_description;
+SET allow_system_table_mods = on;
+-- create new table in pg_catalog
+CREATE TABLE pg_catalog.test (a int);
+-- anyarray column
+CREATE TABLE t1 (a int, b anyarray);
+-- index on system catalog
+CREATE INDEX ON pg_class (relnamespace);
+-- index on system catalog
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+-- write to system catalog table as superuser
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 2, 'foo');
+-- write to system catalog table as normal user
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 3, 'foo');
+RESET ROLE;
+-- policy on system catalog
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+-- reserved schema name
+CREATE SCHEMA pg_foo;
+-- drop system table
+BEGIN;
+DROP TABLE pg_description;
+ERROR:  cannot drop table pg_description because it is required by the database system
+ROLLBACK;
+-- truncate of system table
+BEGIN;
+TRUNCATE pg_description;
+ROLLBACK;
+-- rename column of system table
+BEGIN;
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+ROLLBACK;
+-- ATSimplePermissions()
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+-- SET STATISTICS
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+-- foreign key referencing catalog
+BEGIN;
+ALTER TABLE pg_description ADD PRIMARY KEY USING INDEX pg_description_o_c_o_index;
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ROLLBACK;
+-- RangeVarCallbackOwnsRelation()
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+-- RangeVarCallbackForAlterRelation()
+BEGIN;
+ALTER TABLE pg_description RENAME TO pg_comment;
+ROLLBACK;
+BEGIN;
+ALTER TABLE pg_description SET SCHEMA public;
+ROLLBACK;
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+ERROR:  directory "/no/such/location" does not exist
+-- triggers
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+DROP TRIGGER t2 ON pg_description;
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ALTER RULE r1 ON pg_description RENAME TO r2;
+DROP RULE r2 ON pg_description;
+-- cleanup
+REVOKE ALL ON pg_description FROM regress_user_ast;
+DELETE FROM pg_description WHERE objoid = 0 AND classoid = 0;
+DROP USER regress_user_ast;
+DROP FUNCTION tf1;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index f23fe8d870..6291a9c279 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -120,3 +120,6 @@ test: fast_default
 
 # run stats by itself because its delay may be insufficient under heavy load
 test: stats
+
+# TODO
+test: alter_system_table
diff --git a/src/test/regress/sql/alter_system_table.sql b/src/test/regress/sql/alter_system_table.sql
new file mode 100644
index 0000000000..83a0abe4b9
--- /dev/null
+++ b/src/test/regress/sql/alter_system_table.sql
@@ -0,0 +1,165 @@
+CREATE USER regress_user_ast;
+
+SET allow_system_table_mods = off;
+
+-- create new table in pg_catalog
+CREATE TABLE pg_catalog.test (a int);
+
+-- anyarray column
+CREATE TABLE t1x (a int, b anyarray);
+
+-- index on system catalog
+CREATE INDEX ON pg_class (relnamespace);
+
+-- index on system catalog
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+
+-- write to system catalog table as superuser
+-- (does not need allow_system_table_mods)
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 0, 'foo');
+
+-- write to system catalog table as normal user
+GRANT INSERT ON pg_description TO regress_user_ast;
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 1, 'foo');
+RESET ROLE;
+
+-- policy on system catalog
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+
+-- reserved schema name
+CREATE SCHEMA pg_foo;
+
+-- drop system table
+DROP TABLE pg_description;
+
+-- truncate of system table
+TRUNCATE pg_description;
+
+-- rename column of system table
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+
+-- ATSimplePermissions()
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+
+-- SET STATISTICS
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+
+-- foreign key referencing catalog
+BEGIN;
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ROLLBACK;
+
+-- RangeVarCallbackOwnsRelation()
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+
+-- RangeVarCallbackForAlterRelation()
+ALTER TABLE pg_description RENAME TO pg_comment;
+ALTER TABLE pg_description SET SCHEMA public;
+
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+
+-- triggers
+CREATE FUNCTION tf1() RETURNS trigger
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RETURN NULL;
+END $$;
+
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+--DROP TRIGGER t2 ON pg_description;
+
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ALTER RULE r1 ON pg_description RENAME TO r2;
+--DROP RULE r2 ON pg_description;
+
+
+SET allow_system_table_mods = on;
+
+-- create new table in pg_catalog
+CREATE TABLE pg_catalog.test (a int);
+
+-- anyarray column
+CREATE TABLE t1 (a int, b anyarray);
+
+-- index on system catalog
+CREATE INDEX ON pg_class (relnamespace);
+
+-- index on system catalog
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+
+-- write to system catalog table as superuser
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 2, 'foo');
+
+-- write to system catalog table as normal user
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 3, 'foo');
+RESET ROLE;
+
+-- policy on system catalog
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+
+-- reserved schema name
+CREATE SCHEMA pg_foo;
+
+-- drop system table
+BEGIN;
+DROP TABLE pg_description;
+ROLLBACK;
+
+-- truncate of system table
+BEGIN;
+TRUNCATE pg_description;
+ROLLBACK;
+
+-- rename column of system table
+BEGIN;
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+ROLLBACK;
+
+-- ATSimplePermissions()
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+
+-- SET STATISTICS
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+
+-- foreign key referencing catalog
+BEGIN;
+ALTER TABLE pg_description ADD PRIMARY KEY USING INDEX pg_description_o_c_o_index;
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ROLLBACK;
+
+-- RangeVarCallbackOwnsRelation()
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+
+-- RangeVarCallbackForAlterRelation()
+BEGIN;
+ALTER TABLE pg_description RENAME TO pg_comment;
+ROLLBACK;
+BEGIN;
+ALTER TABLE pg_description SET SCHEMA public;
+ROLLBACK;
+
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+
+-- triggers
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+DROP TRIGGER t2 ON pg_description;
+
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ALTER RULE r1 ON pg_description RENAME TO r2;
+DROP RULE r2 ON pg_description;
+
+
+-- cleanup
+REVOKE ALL ON pg_description FROM regress_user_ast;
+DELETE FROM pg_description WHERE objoid = 0 AND classoid = 0;
+DROP USER regress_user_ast;
+DROP FUNCTION tf1;
-- 
2.22.0

v1-0003-Disable-dead-code.patchtext/plain; charset=UTF-8; name=v1-0003-Disable-dead-code.patch; x-mac-creator=0; x-mac-type=0Download

From e0008c8e1504af1e8c5de25c2991423adb8ea30f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 21 Jun 2019 10:24:03 +0200
Subject: [PATCH v1 3/3] Disable dead code

---
 src/backend/catalog/index.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 587b717242..71ed9ea639 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -755,12 +755,14 @@ index_create(Relation heapRelation,
 	if (indexInfo->ii_NumIndexAttrs < 1)
 		elog(ERROR, "must index at least one column");
 
+#ifdef NOT_USED
 	if (!allow_system_table_mods &&
 		IsSystemRelation(heapRelation) &&
 		IsNormalProcessingMode())
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("user-defined indexes on system catalog tables are not supported")));
+#endif
 
 	/*
 	 * Concurrent index build on a system catalog is unsafe because we tend to
@@ -1700,6 +1702,7 @@ index_constraint_create(Relation heapRelation,
 	/* constraint creation support doesn't work while bootstrapping */
 	Assert(!IsBootstrapProcessingMode());
 
+#ifdef NOT_USED
 	/* enforce system-table restriction */
 	if (!allow_system_table_mods &&
 		IsSystemRelation(heapRelation) &&
@@ -1707,6 +1710,7 @@ index_constraint_create(Relation heapRelation,
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("user-defined indexes on system catalog tables are not supported")));
+#endif
 
 	/* primary/unique constraints shouldn't have any expressions */
 	if (indexInfo->ii_Expressions &&
-- 
2.22.0

Robert Haas

robertmhaas@gmail.com

over 6 years ago

In reply to: Peter Eisentraut (#1)

Re: allow_system_table_mods stuff

On Fri, Jun 21, 2019 at 5:12 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Attached are some patches to get the discussion rolling: One patch makes
allow_system_table_mods settable at run time by superuser, the second
one is a test suite that documents the current behavior that I gathered
after analyzing the source code, the third one removes some code that
was found useless by the tests. (The first patch might be useful on its
own, but right now it's just to facilitate the test suite.)

Sounds generally sensible (but I didn't read the code). I
particularly like the first idea.

Any other thoughts?

I kinda feel like we should prohibit DML on system catalogs, even by
superusers, unless you press the big red button that says "I am
definitely sure that I know what I'm doing." Linking that with
allow_system_table_mods is some way seems natural, but I'm not totally
sure it's the right thing to do. I guess we could have
alter_table_system_mods={no,yes,yesyesyes}, the former allowing DML
and not-too-scary things and the latter allowing anything at all.

A related issue is that alter_system_table_mods prohibits both stuff
that's probably not going to cause any big problem and stuff that is
almost guaranteed to make the system permanently unusable - e.g. you
could 'SET STORAGE' on a system catalog column, which is really pretty
innocuous, or you could change the oid column of pg_database to a
varlena type, which is guaranteed to destroy the universe. Here
again, maybe some operations should be more protected than others, or
maybe the relatively safe things just shouldn't be subject to
allow_system_table_mods at all.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Stephen Frost

sfrost@snowman.net

over 6 years ago

In reply to: Robert Haas (#2)

Re: allow_system_table_mods stuff

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Fri, Jun 21, 2019 at 5:12 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Any other thoughts?

I kinda feel like we should prohibit DML on system catalogs, even by
superusers, unless you press the big red button that says "I am
definitely sure that I know what I'm doing." Linking that with
allow_system_table_mods is some way seems natural, but I'm not totally
sure it's the right thing to do. I guess we could have
alter_table_system_mods={no,yes,yesyesyes}, the former allowing DML
and not-too-scary things and the latter allowing anything at all.

I agree that we should be strongly discouraging even superusers from
doing DML or DDL on system catalogs, and making them jump through hoops
to make it happen at all.

A related issue is that alter_system_table_mods prohibits both stuff
that's probably not going to cause any big problem and stuff that is
almost guaranteed to make the system permanently unusable - e.g. you
could 'SET STORAGE' on a system catalog column, which is really pretty
innocuous, or you could change the oid column of pg_database to a
varlena type, which is guaranteed to destroy the universe. Here
again, maybe some operations should be more protected than others, or
maybe the relatively safe things just shouldn't be subject to
allow_system_table_mods at all.

If there are things which are through proper grammar (ALTER TABLE or
such) and which will actually usefully work when done against a system
catalog table (eg: GRANT), then I'm all for just allowing that, provided
the regular security checks are done. I don't think we should ever be
allowed DML though, or any DDL which we know will break the system,
without making them go through hoops. Personally, I'd rather disallow
all DDL on system catalogs and then explicitly add support for specific
DDL when someone complains and has done a sufficient review to show that
allowing that DDL is a good thing and will actually work as intended.

Thanks,

Stephen

Tom Lane

tgl@sss.pgh.pa.us

over 6 years ago

In reply to: Robert Haas (#2)

Re: allow_system_table_mods stuff

Robert Haas <robertmhaas@gmail.com> writes:

I kinda feel like we should prohibit DML on system catalogs, even by
superusers, unless you press the big red button that says "I am
definitely sure that I know what I'm doing."

Keep in mind that DML-on-system-catalogs is unfortunately a really
standard hack in extension upgrade scripts. (If memory serves,
some of our contrib scripts do that, and we've certainly told third
parties that it's the only way out of some box or other.) I don't
think we can just shut it off. What you seem to be proposing is to
allow it only after

SET allow_system_table_mods = on;

which would be all right except that an extension script containing
such a command will fail outright in existing releases. I think we
need to be friendlier than that to extension authors who are, for the
most part, trying to work around some deficiency of ours not theirs.

I'm not saying that DML-off-by-default is a bad goal to work toward;
I'm just saying "mind the collateral damage".

A related issue is that alter_system_table_mods prohibits both stuff
that's probably not going to cause any big problem and stuff that is
almost guaranteed to make the system permanently unusable - e.g. you
could 'SET STORAGE' on a system catalog column, which is really pretty
innocuous, or you could change the oid column of pg_database to a
varlena type, which is guaranteed to destroy the universe. Here
again, maybe some operations should be more protected than others, or
maybe the relatively safe things just shouldn't be subject to
allow_system_table_mods at all.

Meh. It doesn't really seem to me that distinguishing these cases
is a productive use of code space or maintenance effort. Superusers
are assumed to know what they're doing, and most especially so if
they've hit the big red button.

regards, tom lane

Stephen Frost

sfrost@snowman.net

over 6 years ago

In reply to: Tom Lane (#4)

Re: allow_system_table_mods stuff

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I kinda feel like we should prohibit DML on system catalogs, even by
superusers, unless you press the big red button that says "I am
definitely sure that I know what I'm doing."

Keep in mind that DML-on-system-catalogs is unfortunately a really
standard hack in extension upgrade scripts. (If memory serves,
some of our contrib scripts do that, and we've certainly told third
parties that it's the only way out of some box or other.) I don't
think we can just shut it off. What you seem to be proposing is to
allow it only after

SET allow_system_table_mods = on;

That's basically what my feeling is, yes.

which would be all right except that an extension script containing
such a command will fail outright in existing releases. I think we
need to be friendlier than that to extension authors who are, for the
most part, trying to work around some deficiency of ours not theirs.

As with other cases where someone needs to do DML against the catalog
for some reason or another- we should fix that. If there's example
cases, great! Let's look at those and come up with a proper solution.

Other options include- letting an extension set that GUC (seems likely
that any case where this is needed is a case where the extension is
installing C functions and therefore is being run by a superuser
anyway...), or implicitly setting that GUC when we're running an
extension's script (urrggghhhh... I don't care for that one bit, but I
like it better than letting any superuser who wishes UPDATE random bits
in the catalog).

I'm not saying that DML-off-by-default is a bad goal to work toward;
I'm just saying "mind the collateral damage".

Sure, makes sense.

A related issue is that alter_system_table_mods prohibits both stuff
that's probably not going to cause any big problem and stuff that is
almost guaranteed to make the system permanently unusable - e.g. you
could 'SET STORAGE' on a system catalog column, which is really pretty
innocuous, or you could change the oid column of pg_database to a
varlena type, which is guaranteed to destroy the universe. Here
again, maybe some operations should be more protected than others, or
maybe the relatively safe things just shouldn't be subject to
allow_system_table_mods at all.

Meh. It doesn't really seem to me that distinguishing these cases
is a productive use of code space or maintenance effort. Superusers
are assumed to know what they're doing, and most especially so if
they've hit the big red button.

The direction I took the above was that we should actually be thinking
about if there are acceptable cases to be running DDL against the
catalog and, if so, specifically allow those. I'm not convinced at the
moment that any such exist and therefore I'd rather have it denied
(unless you push the big red button) and then tell people to show us
their use case and then we can decide if it's an 'ok' thing to allow, or
what.

I'd really like to stop the cases like stackoverflow articles that
describe how to "remove" an enum value by simply modifying the catalog,
or at least make them have to add a "well, push this big red button
first that the PG people tell you never to push, and then.." to the
start.

Thanks,

Stephen

Andres Freund

andres@anarazel.de

over 6 years ago

In reply to: Peter Eisentraut (#1)

Re: allow_system_table_mods stuff

On 2019-06-21 11:12:38 +0200, Peter Eisentraut wrote:

After the earlier thread [0] that dealt with ALTER TABLE on system
catalogs, I took a closer look at the allow_system_table_mods setting.
I found a few oddities, and it seems there is some room for improvement.

I complained about this recently again, and unfortunately the reaction
wasn't that welcoming:
/messages/by-id/20190509145054.byiwa255xvdbfh3a@alap3.anarazel.de

Attached are some patches to get the discussion rolling: One patch makes
allow_system_table_mods settable at run time by superuser

+1 - this seems to have agreement.

- For the most part, a_s_t_m establishes an additional level of access
control on top of superuserdom for doing DDL on system catalogs. That
seems like a useful definition.

- But enabling a_s_t_m also allows a non-superuser to do DML on system
catalogs. That seems like an entirely unrelated and surprising behavior.

Indeed.

- Some checks are redundant with the pinning concept of the dependency
system. For example, you can't drop a system catalog even with a_s_t_m
on. That seems useful, of course, but as a result there is a bit of
dead or useless code around. (The dependency system is newer than a_s_t_m.)

I'm not fond of deduplicating things around this. This seems like a
separate layers of defense to me.

- Having a test suite like this seems useful.

- The behavior that a_s_t_m allows non-superusers to do DML on system
catalogs should be removed. (Regular permissions can be used for that.)

- Dead code or code that is redundant with pinning should be removed.

-1

Any other thoughts?

* a_s_t_m=off should forbid modifying catalog tables, even for
superusers.

Greetings,

Andres Freund

andres@anarazel.de

over 6 years ago

In reply to: Tom Lane (#4)

Re: allow_system_table_mods stuff

Hi,

On 2019-06-21 12:28:43 -0400, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I kinda feel like we should prohibit DML on system catalogs, even by
superusers, unless you press the big red button that says "I am
definitely sure that I know what I'm doing."

Keep in mind that DML-on-system-catalogs is unfortunately a really
standard hack in extension upgrade scripts. (If memory serves,
some of our contrib scripts do that, and we've certainly told third
parties that it's the only way out of some box or other.) I don't
think we can just shut it off. What you seem to be proposing is to
allow it only after

SET allow_system_table_mods = on;

which would be all right except that an extension script containing
such a command will fail outright in existing releases. I think we
need to be friendlier than that to extension authors who are, for the
most part, trying to work around some deficiency of ours not theirs.

I'm not quite convinced we need to go very far with compatibility here -
pretty much by definition scripts that do this are tied a lot more to
the internals than ones using DDL. But if we want to, we could just -
for now at least - set allow_system_table_mods to a new 'warn' - when
processing extension scripts as superusers.

A related issue is that alter_system_table_mods prohibits both stuff
that's probably not going to cause any big problem and stuff that is
almost guaranteed to make the system permanently unusable - e.g. you
could 'SET STORAGE' on a system catalog column, which is really pretty
innocuous, or you could change the oid column of pg_database to a
varlena type, which is guaranteed to destroy the universe. Here
again, maybe some operations should be more protected than others, or
maybe the relatively safe things just shouldn't be subject to
allow_system_table_mods at all.

Meh. It doesn't really seem to me that distinguishing these cases
is a productive use of code space or maintenance effort. Superusers
are assumed to know what they're doing, and most especially so if
they've hit the big red button.

I really don't buy this. You need superuser for nearly all CREATE
EXTENSION invocations, and for a lot of other routine tasks. Making the
non-routine crazy stuff slightly harder is worthwhile. I don't think we
can really separate those two into fully separate roles unfortunately,
because the routine CREATE EXTENSION stuff obviously can be used to
elevate privs.

Greetings,

Andres Freund

Stephen Frost

sfrost@snowman.net

over 6 years ago

In reply to: Andres Freund (#7)

Re: allow_system_table_mods stuff

Greetings,

* Andres Freund (andres@anarazel.de) wrote:

On 2019-06-21 12:28:43 -0400, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

A related issue is that alter_system_table_mods prohibits both stuff
that's probably not going to cause any big problem and stuff that is
almost guaranteed to make the system permanently unusable - e.g. you
could 'SET STORAGE' on a system catalog column, which is really pretty
innocuous, or you could change the oid column of pg_database to a
varlena type, which is guaranteed to destroy the universe. Here
again, maybe some operations should be more protected than others, or
maybe the relatively safe things just shouldn't be subject to
allow_system_table_mods at all.

Meh. It doesn't really seem to me that distinguishing these cases
is a productive use of code space or maintenance effort. Superusers
are assumed to know what they're doing, and most especially so if
they've hit the big red button.

I really don't buy this. You need superuser for nearly all CREATE
EXTENSION invocations, and for a lot of other routine tasks. Making the
non-routine crazy stuff slightly harder is worthwhile. I don't think we
can really separate those two into fully separate roles unfortunately,
because the routine CREATE EXTENSION stuff obviously can be used to
elevate privs.

I'm not sure what you're intending to respond to here, but I don't think
it's what was being discussed.

The question went something like this- if we decide that setting the
default for relpages made sense in some use-case, should a superuser
have to hit the big red button to do:

ALTER TABLE pg_class SET DEFAULT relpages = 100;

or should we just allow it?

Tom's opinion, if I followed it correctly, was 'no, that is rare enough
that it just is not worth the extra code to allow that without the big
red button, but deny everything else.' My opinion was 'if they bring us
a legitimate use-case for such, then, sure, maybe we allow specficially
that without hitting the big red button.' Robert was suggesting that we
could have a tri-state for a_s_t_m, where you could hit the big red
button only half-way, and certain things would then be allowed.

At least, I think that's about how it went. None of it was about doing
typical CREATE EXTENSION and similar routine tasks that don't involve
running ALTER TABLE or DML against catalog tables.

Thanks,

Stephen

Tom Lane

tgl@sss.pgh.pa.us

over 6 years ago

In reply to: Stephen Frost (#5)

Re: allow_system_table_mods stuff

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Keep in mind that DML-on-system-catalogs is unfortunately a really
standard hack in extension upgrade scripts. (If memory serves,
some of our contrib scripts do that, and we've certainly told third
parties that it's the only way out of some box or other.)

As with other cases where someone needs to do DML against the catalog
for some reason or another- we should fix that. If there's example
cases, great! Let's look at those and come up with a proper solution.

As I said, we've got examples. I traced the existing UPDATEs-on-catalogs
in contrib scripts back to their origin commits, and found these:

commit a89b4b1be0d3550a7860250ff74dc69730555a1f
Update citext extension for parallel query.

This could have been done cleaner if we had ALTER AGGREGATE variants
that would let us add an aggcombine function after the fact and mark
the aggregate PARALLEL SAFE. Seems like a reasonable feature, but
it still doesn't exist today, three years later.

commit 94be9e3f0ca9e7ced66168397eb586565bced9ca
Fix citext's upgrade-from-unpackaged script to set its collation correctly.
commit 9b97b7f8356c63ea0b6704718d75ea01ec3035bf
Fix citext upgrade script to update derived copies of pg_type.typcollation.

The difficulty here was lack of ALTER TYPE to change a type's
collation. We'd have to rethink the denormalized storage of
typcollation in a lot of other places if we wanted to support that,
but possibly it'd be worth it.

commit 749a787c5b25ae33b3d4da0ef12aa05214aa73c7
Handle contrib's GIN/GIST support function signature changes honestly.

This needed to change the declared argument types of some support
functions, without having their OIDs change. No, I *don't* think
it'd be a good idea to provide a DDL command to do that.

commit de1d042f5979bc1388e9a6d52a4d445342b04932
Support index-only scans in contrib/cube and contrib/seg GiST indexes.

"The only exciting part of this is that ALTER OPERATOR FAMILY lacks
a way to drop a support function that was declared as being part of
an opclass rather than being loose in the family. For the moment,
we'll hack our way to a solution with a manual update of the pg_depend
entry type, which is what distinguishes the two cases. Perhaps
someday it'll be worth providing a cleaner way to do that, but for
now it seems like a very niche problem."

commit 0024e348989254d48dc4afe9beab98a6994a791e
Fix upgrade of contrib/intarray and contrib/unaccent from 9.0.
commit 4eb49db7ae634fab9af7437b2e7b6388dfd83bd3
Fix contrib/pg_trgm to have smoother updates from 9.0.

More cases where we had to change the proargtypes of a pg_proc
entry without letting its OID change.

commit 472f608e436a41865b795c999bda3369725fa097
One more hack to make contrib upgrades from 9.0 match fresh 9.1 installs.

Lack of a way to replace a support function in an existing opclass.
It's possible this could be done better, but on the other hand, it'd
be *really* hard to have an ALTER OPCLASS feature for that that would
be even a little bit concurrency-safe.

So there's certainly some fraction of these cases where we could have
avoided doing manual catalog updates by expending work on some ALTER
command instead. But I don't see much reason to think that we could,
or should try to, insist that every such case be done that way. The
cost/benefit ratio is not there in some cases, and in others, exposing
a DDL command to do it would just be providing easier access to
something that's fundamentally unsafe anyway.

The change-proargtypes example actually brings up a larger point:
exactly how is, say, screwing with the contents of the pg_class
row for a system catalog any safer than doing "DDL" on the catalog?
I don't think we should fool ourselves that the one thing is
inherently safer than the other.

In none of these cases are we ever going to be able to say "that's
generically safe", or at least if we try, we're going to find that
distinguishing safe cases from unsafe requires unreasonable amounts
of effort. I don't think it's a productive thing to spend time on.
I don't mind having two separate "allow_system_table_ddl" and
"allow_system_table_dml" flags, because it's easy to tell what each
of those is supposed to enforce. But I'm going to run away screaming
from any proposal to invent "allow_safe_system_table_dml". It's a
recipe for infinite security bugs and it's just not worth it.

regards, tom lane

#10

Stephen Frost

sfrost@snowman.net

over 6 years ago

In reply to: Tom Lane (#9)

Re: allow_system_table_mods stuff

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

So there's certainly some fraction of these cases where we could have
avoided doing manual catalog updates by expending work on some ALTER
command instead. But I don't see much reason to think that we could,
or should try to, insist that every such case be done that way. The
cost/benefit ratio is not there in some cases, and in others, exposing
a DDL command to do it would just be providing easier access to
something that's fundamentally unsafe anyway.

In the cases where we can do better by providing a DDL command, it's
certainly my opinion that we should go that route. I don't think we
should allow something that's fundamentally unsafe in that way- for
those cases though, how is the extension script making it 'safe'? If it
simply is hoping, well, that smells like a bug, and we probably should
try to avoid having that in our extensions as folks do like to copy
them.

When it comes to cases that fundamentally are one-off's and that we
don't think really deserve a proper DDL command, then I'd say we make
the extensions set the flag. At least then it's clear "hey, we had to
do something really grotty here, maybe don't copy this into your new
extension, or don't use this method." We should also un-set the flag
after.

The change-proargtypes example actually brings up a larger point:
exactly how is, say, screwing with the contents of the pg_class
row for a system catalog any safer than doing "DDL" on the catalog?
I don't think we should fool ourselves that the one thing is
inherently safer than the other.

I don't believe one to be safer than the other...

In none of these cases are we ever going to be able to say "that's
generically safe", or at least if we try, we're going to find that
distinguishing safe cases from unsafe requires unreasonable amounts
of effort. I don't think it's a productive thing to spend time on.
I don't mind having two separate "allow_system_table_ddl" and
"allow_system_table_dml" flags, because it's easy to tell what each
of those is supposed to enforce.

Which implies that it doesn't make sense to have two different flags
for it.

But I'm going to run away screaming
from any proposal to invent "allow_safe_system_table_dml". It's a
recipe for infinite security bugs and it's just not worth it.

Yeah, I'm not really a fan of that either.

Thanks,

Stephen

#11

Chapman Flack

chap@anastigmatix.net

over 6 years ago

In reply to: Stephen Frost (#10)

Re: allow_system_table_mods stuff

On 6/21/19 3:07 PM, Stephen Frost wrote:

When it comes to cases that fundamentally are one-off's and that we
don't think really deserve a proper DDL command, then I'd say we make
the extensions set the flag. At least then it's clear "hey, we had to
do something really grotty here, maybe don't copy this into your new
extension, or don't use this method." We should also un-set the flag
after.

I'd be leery of collateral damage from that to extension update scripts
in extension releases currently in the wild.

Maybe there should be a new extension control file setting

needs_system_table_mods = (boolean)

which means what it says if it's there, but if an ALTER EXTENSION
UPDATE sees a control file that lacks the setting, assume true
(with a warning?).

Regards,
-Chap

#12

Tom Lane

tgl@sss.pgh.pa.us

over 6 years ago

In reply to: Chapman Flack (#11)

Re: allow_system_table_mods stuff

Chapman Flack <chap@anastigmatix.net> writes:

I'd be leery of collateral damage from that to extension update scripts
in extension releases currently in the wild.

Yeah, that's my primary concern here.

Maybe there should be a new extension control file setting
needs_system_table_mods = (boolean)
which means what it says if it's there, but if an ALTER EXTENSION
UPDATE sees a control file that lacks the setting, assume true
(with a warning?).

I think that having a SET command in the update script is the way to go;
for one thing it simplifies testing the script by just sourcing it,
and for another it defaults to no-special-privileges which is the
right default. Also, non-backward-compatible control files aren't
any nicer than non-backward-compatible script files.

We do have to get past the compatibility issue though. My thought was
that for a period of N years we could force allow_system_table_dml = on
while running extension scripts, and then cease doing so. This would
give extension authors a reasonable window in which their scripts would
work in either old or new backends. At some point in that time they'd
probably have occasion to make other changes that render their scripts
not backwards compatible, at which point they can insert "SET
allow_system_table_dml = on" so that the script keeps working when we
remove the compatibility hack.

(Of course, we have an awful track record about actually doing things
N years after we said we would, but doesn't anybody around here have
a calendar app?)

This line of thought leads to the conclusion that we do want
separate "allow_system_table_dml" and "allow_system_table_ddl"
bools. Otherwise, the backwards-compatibility hack would need
to turn on a level of unsafety that extension scripts have *not*
had before and surely shouldn't have by default.

regards, tom lane

#13

Chapman Flack

chap@anastigmatix.net

over 6 years ago

In reply to: Tom Lane (#12)

Re: allow_system_table_mods stuff

On 6/21/19 4:37 PM, Tom Lane wrote:

We do have to get past the compatibility issue though. My thought was
that for a period of N years we could force allow_system_table_dml = on
while running extension scripts, and then cease doing so. This would
give extension authors a reasonable window in which their scripts would
work in either old or new backends. At some point in that time they'd
probably have occasion to make other changes that render their scripts
not backwards compatible, at which point they can insert "SET
allow_system_table_dml = on" so that the script keeps working when we
remove the compatibility hack.

I was having second thoughts too, like maybe to tweak ALTER EXTENSION
UPDATE to unconditionally force the flag on before the update script and
reset it after, but warn if it is actually still set at the reset-after
point.

Extension maintainers could then make the warning go away by releasing
versions where the update scripts contain an explicit RESET (at the very
top, if they do nothing fancy), or a(n initially redundant) SET at the
top and RESET at the bottom. No new control file syntax.

Regards,
-Chap

#14

Andres Freund

andres@anarazel.de

over 6 years ago

In reply to: Tom Lane (#12)

Re: allow_system_table_mods stuff

Hi,

On 2019-06-21 16:37:16 -0400, Tom Lane wrote:

We do have to get past the compatibility issue though. My thought was
that for a period of N years we could force allow_system_table_dml = on
while running extension scripts, and then cease doing so. This would
give extension authors a reasonable window in which their scripts would
work in either old or new backends. At some point in that time they'd
probably have occasion to make other changes that render their scripts
not backwards compatible, at which point they can insert "SET
allow_system_table_dml = on" so that the script keeps working when we
remove the compatibility hack.

I'd modify this approach by having a allow_system_table_dml level that
warns when DDL to system tables is performed, and then set
allow_system_table_dml to that when processing extension scripts (and
perhaps modify the warning message when creating_extension ==
true). That way it'd be easier to spot such extension scripts.

And I'd personally probably just set allow_system_table_dml to warn when
working interactively, just to improve logging etc.

This line of thought leads to the conclusion that we do want
separate "allow_system_table_dml" and "allow_system_table_ddl"
bools. Otherwise, the backwards-compatibility hack would need
to turn on a level of unsafety that extension scripts have *not*
had before and surely shouldn't have by default.

Greetings,

Andres Freund

#15

Robert Haas

robertmhaas@gmail.com

over 6 years ago

In reply to: Tom Lane (#12)

Re: allow_system_table_mods stuff

On Fri, Jun 21, 2019 at 4:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

This line of thought leads to the conclusion that we do want
separate "allow_system_table_dml" and "allow_system_table_ddl"
bools. Otherwise, the backwards-compatibility hack would need
to turn on a level of unsafety that extension scripts have *not*
had before and surely shouldn't have by default.

Right, exactly.

I'm repeating myself, but I still think it's super-useful to
distinguish things which are "for expert use only" from things which
are "totally bonkers." You can argue that if you're an expert, you
should know enough to avoid the totally bonkers things, but PostgreSQL
is pretty popular these days [citation needed] and there are a lot of
people administering databases who know what they are doing to a
pretty reasonable degree but don't have anywhere near the level of
understanding of someone who spends their days hacking core. Putting
up some kind of a stop sign that lets you know when you're about to go
from adventurous to lethal will help those people.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#16

Tom Lane

tgl@sss.pgh.pa.us

over 6 years ago

In reply to: Robert Haas (#15)

Re: allow_system_table_mods stuff

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Jun 21, 2019 at 4:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

This line of thought leads to the conclusion that we do want
separate "allow_system_table_dml" and "allow_system_table_ddl"
bools. Otherwise, the backwards-compatibility hack would need
to turn on a level of unsafety that extension scripts have *not*
had before and surely shouldn't have by default.

Right, exactly.

I'm repeating myself, but I still think it's super-useful to
distinguish things which are "for expert use only" from things which
are "totally bonkers."

Agreed, although "DML vs DDL" is a pretty poor approximation of that
boundary. As shown in examples upthread, you can find reasonable things
to do and totally-catastrophic things to do in both categories.

The position I'm maintaining is that it's not worth our trouble to try to
mechanically distinguish which things are which. Once you've broken the
glass and flipped either the big red switch or the slightly smaller orange
switch, it's entirely on you to not screw up your database beyond
recovery.

I do see value in two switches not one, but it's what I said above,
to not need to give people *more* chance-to-break-things than they
had before when doing manual catalog fixes. That is, we need a
setting that corresponds more or less to current default behavior.

There's an aesthetic argument to be had about whether to have two
bools or one three-way switch, but I prefer the former; there's
no backward-compatibility issue here since allow_system_table_mods
couldn't be set by applications anyway.

regards, tom lane

#17

Robert Haas

robertmhaas@gmail.com

over 6 years ago

In reply to: Tom Lane (#16)

Re: allow_system_table_mods stuff

On Mon, Jun 24, 2019 at 11:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm repeating myself, but I still think it's super-useful to
distinguish things which are "for expert use only" from things which
are "totally bonkers."

Agreed, although "DML vs DDL" is a pretty poor approximation of that
boundary. As shown in examples upthread, you can find reasonable things
to do and totally-catastrophic things to do in both categories.

I agree. I would like it if there were a way to do better, but I'm
not sure that there is, at least for a reasonable level of effort.

There's an aesthetic argument to be had about whether to have two
bools or one three-way switch, but I prefer the former; there's
no backward-compatibility issue here since allow_system_table_mods
couldn't be set by applications anyway.

I'm happy to defer on that point.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#18

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 6 years ago

In reply to: Peter Eisentraut (#1)

1 attachment(s)

Re: allow_system_table_mods stuff

Here is a new patch after the discussion.

- Rename allow_system_table_mods to allow_system_table_ddl.

(This makes room for a new allow_system_table_dml, but it's not
implemented here.)

- Make allow_system_table_ddl SUSET.

- Add regression test.

- Remove the behavior that allow_system_table_mods allowed
non-superusers to do DML on catalog tables without further access checking.

I think there was general agreement on all these points.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

v2-0001-New-and-improved-allow_system_table_ddl-setting.patchtext/plain; charset=UTF-8; name=v2-0001-New-and-improved-allow_system_table_ddl-setting.patch; x-mac-creator=0; x-mac-type=0Download

From b44c7c7b7177a87e777b3b2a7fe10ea739bfaa72 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 28 Jun 2019 11:36:04 +0200
Subject: [PATCH v2] New and improved allow_system_table_ddl setting

Rename allow_system_table_mods to allow_system_table_ddl to clarify
its meaning.  There is also some discussion about introducing a
parallel allow_system_table_dml setting, so this makes room for that.

Make allow_system_table_ddl settable at run time by superusers.  It
was previously postmaster start only.  We don't necessarily want to
make system catalog DDL wide-open, but there are occasionally useful
things to do like setting reloptions or statistics on a busy system
table, and blocking those doesn't help anyone.  Also, this enables
writing a test suite.

Add a regression test file that exercises the kinds of commands that
allow_system_table_ddl allows.

Previously, allow_system_table_mods allowed a non-superuser to do DML
on a system table without further permission checks.  This has been
removed, as it was quite inconsistent with the rest of the meaning of
this setting.  (Since allow_system_table_mods was previously only
accessible with a server restart, it is unlikely that anyone was using
this possibility.)

Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com
---
 doc/src/sgml/config.sgml                      |  16 +-
 doc/src/sgml/ref/show.sgml                    |  10 +-
 src/backend/catalog/aclchk.c                  |   5 +-
 src/backend/catalog/catalog.c                 |   2 +-
 src/backend/catalog/heap.c                    |  16 +-
 src/backend/catalog/index.c                   |  16 +-
 src/backend/commands/indexcmds.c              |   2 +-
 src/backend/commands/policy.c                 |   6 +-
 src/backend/commands/schemacmds.c             |   4 +-
 src/backend/commands/tablecmds.c              |  20 +-
 src/backend/commands/tablespace.c             |   4 +-
 src/backend/commands/trigger.c                |   6 +-
 src/backend/postmaster/postmaster.c           |   2 +-
 src/backend/rewrite/rewriteDefine.c           |   4 +-
 src/backend/tcop/postgres.c                   |   2 +-
 src/backend/utils/cache/relmapper.c           |   2 +-
 src/backend/utils/init/globals.c              |   2 +-
 src/backend/utils/misc/guc.c                  |   6 +-
 src/include/catalog/heap.h                    |   4 +-
 src/include/catalog/index.h                   |   4 +-
 src/include/miscadmin.h                       |   2 +-
 .../regress/expected/alter_system_table.out   | 170 ++++++++++++++++
 src/test/regress/expected/alter_table.out     |  10 +-
 src/test/regress/parallel_schedule            |   2 +-
 src/test/regress/serial_schedule              |   1 +
 src/test/regress/sql/alter_system_table.sql   | 187 ++++++++++++++++++
 src/test/regress/sql/alter_table.sql          |   6 +-
 27 files changed, 431 insertions(+), 80 deletions(-)
 create mode 100644 src/test/regress/expected/alter_system_table.out
 create mode 100644 src/test/regress/sql/alter_system_table.sql

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 84341a30e5..7a4e141a2d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9288,17 +9288,19 @@ <title>Developer Options</title>
     </para>
 
     <variablelist>
-     <varlistentry id="guc-allow-system-table-mods" xreflabel="allow_system_table_mods">
-      <term><varname>allow_system_table_mods</varname> (<type>boolean</type>)
+     <varlistentry id="guc-allow-system-table-ddl" xreflabel="allow_system_table_ddl">
+      <term><varname>allow_system_table_ddl</varname> (<type>boolean</type>)
       <indexterm>
-        <primary><varname>allow_system_table_mods</varname> configuration parameter</primary>
+        <primary><varname>allow_system_table_ddl</varname> configuration parameter</primary>
       </indexterm>
       </term>
       <listitem>
        <para>
-        Allows modification of the structure of system tables.
-        This is used by <command>initdb</command>.
-        This parameter can only be set at server start.
+        Allows DDL commands on system tables.  This is otherwise not allowed
+        even for superusers.  This is used by <command>initdb</command>.
+        Inconsiderate use of this setting can cause irretrievable data loss or
+        seriously corrupt the database system.  Only superusers can change
+        this setting.
        </para>
       </listitem>
      </varlistentry>
@@ -9831,7 +9833,7 @@ <title>Short Option Key</title>
        </row>
        <row>
         <entry><option>-O</option></entry>
-        <entry><literal>allow_system_table_mods = on</literal></entry>
+        <entry><literal>allow_system_table_ddl = on</literal></entry>
        </row>
        <row>
         <entry><option>-p <replaceable>x</replaceable></option></entry>
diff --git a/doc/src/sgml/ref/show.sgml b/doc/src/sgml/ref/show.sgml
index 945b0491b1..c98dc02444 100644
--- a/doc/src/sgml/ref/show.sgml
+++ b/doc/src/sgml/ref/show.sgml
@@ -167,14 +167,14 @@ <title>Examples</title>
    Show all settings:
 <programlisting>
 SHOW ALL;
-            name         | setting |                description                                                          
--------------------------+---------+-------------------------------------------------
- allow_system_table_mods | off     | Allows modifications of the structure of ...
+            name        | setting |                description                                                          
+------------------------+---------+-------------------------------------------------
+ allow_system_table_ddl | off     | Allows DDL on system tables.
     .
     .
     .
- xmloption               | content | Sets whether XML data in implicit parsing ...
- zero_damaged_pages      | off     | Continues processing past damaged page headers.
+ xmloption              | content | Sets whether XML data in implicit parsing ...
+ zero_damaged_pages     | off     | Continues processing past damaged page headers.
 (196 rows)
 </programlisting></para>
  </refsect1>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 2797af35c3..945f799c55 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3852,7 +3852,7 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
 
 	/*
 	 * Deny anyone permission to update a system catalog unless
-	 * pg_authid.rolsuper is set.  Also allow it if allowSystemTableMods.
+	 * pg_authid.rolsuper is set.
 	 *
 	 * As of 7.4 we have some updatable system views; those shouldn't be
 	 * protected in this way.  Assume the view rules can take care of
@@ -3861,8 +3861,7 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
 	if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) &&
 		IsSystemClass(table_oid, classForm) &&
 		classForm->relkind != RELKIND_VIEW &&
-		!superuser_arg(roleid) &&
-		!allowSystemTableMods)
+		!superuser_arg(roleid))
 	{
 #ifdef ACLDEBUG
 		elog(DEBUG2, "permission denied for system catalog update");
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index a065419cdb..77d8d54526 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -59,7 +59,7 @@
  *		We treat toast tables of user relations as "system relations" for
  *		protection purposes, e.g. you can't change their schemas without
  *		special permissions.  Therefore, most uses of this function are
- *		checking whether allow_system_table_mods restrictions apply.
+ *		checking whether allow_system_table_ddl restrictions apply.
  *		For other purposes, consider whether you shouldn't be using
  *		IsCatalogRelation instead.
  *
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 86820eecfc..e6960e543d 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -304,7 +304,7 @@ heap_create(const char *relname,
 			char relpersistence,
 			bool shared_relation,
 			bool mapped_relation,
-			bool allow_system_table_mods,
+			bool allow_system_table_ddl,
 			TransactionId *relfrozenxid,
 			MultiXactId *relminmxid)
 {
@@ -320,10 +320,10 @@ heap_create(const char *relname,
 	 * paths including pg_catalog are too confusing for now.
 	 *
 	 * But allow creating indexes on relations in pg_catalog even if
-	 * allow_system_table_mods = off, upper layers already guarantee it's on a
+	 * allow_system_table_ddl = off, upper layers already guarantee it's on a
 	 * user defined relation, not a system one.
 	 */
-	if (!allow_system_table_mods &&
+	if (!allow_system_table_ddl &&
 		((IsCatalogNamespace(relnamespace) && relkind != RELKIND_INDEX) ||
 		 IsToastNamespace(relnamespace)) &&
 		IsNormalProcessingMode())
@@ -1053,7 +1053,7 @@ AddNewRelationType(const char *typeName,
  *	reloptions: reloptions in Datum form, or (Datum) 0 if none
  *	use_user_acl: true if should look for user-defined default permissions;
  *		if false, relacl is always set NULL
- *	allow_system_table_mods: true to allow creation in system namespaces
+ *	allow_system_table_ddl: true to allow creation in system namespaces
  *	is_internal: is this a system-generated catalog?
  *
  * Output parameters:
@@ -1080,7 +1080,7 @@ heap_create_with_catalog(const char *relname,
 						 OnCommitAction oncommit,
 						 Datum reloptions,
 						 bool use_user_acl,
-						 bool allow_system_table_mods,
+						 bool allow_system_table_ddl,
 						 bool is_internal,
 						 Oid relrewrite,
 						 ObjectAddress *typaddress)
@@ -1105,11 +1105,11 @@ heap_create_with_catalog(const char *relname,
 
 	/*
 	 * Validate proposed tupdesc for the desired relkind.  If
-	 * allow_system_table_mods is on, allow ANYARRAY to be used; this is a
+	 * allow_system_table_ddl is on, allow ANYARRAY to be used; this is a
 	 * hack to allow creating pg_statistic and cloning it during VACUUM FULL.
 	 */
 	CheckAttributeNamesTypes(tupdesc, relkind,
-							 allow_system_table_mods ? CHKATYPE_ANYARRAY : 0);
+							 allow_system_table_ddl ? CHKATYPE_ANYARRAY : 0);
 
 	/*
 	 * This would fail later on anyway, if the relation already exists.  But
@@ -1226,7 +1226,7 @@ heap_create_with_catalog(const char *relname,
 							   relpersistence,
 							   shared_relation,
 							   mapped_relation,
-							   allow_system_table_mods,
+							   allow_system_table_ddl,
 							   &relfrozenxid,
 							   &relminmxid);
 
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index bb60b23093..b7719c3077 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -682,7 +682,7 @@ UpdateIndexRelation(Oid indexoid,
  *			create a partitioned index (table must be partitioned)
  * constr_flags: flags passed to index_constraint_create
  *		(only if INDEX_CREATE_ADD_CONSTRAINT is set)
- * allow_system_table_mods: allow table to be a system catalog
+ * allow_system_table_ddl: allow table to be a system catalog
  * is_internal: if true, post creation hook for new index
  * constraintId: if not NULL, receives OID of created constraint
  *
@@ -705,7 +705,7 @@ index_create(Relation heapRelation,
 			 Datum reloptions,
 			 bits16 flags,
 			 bits16 constr_flags,
-			 bool allow_system_table_mods,
+			 bool allow_system_table_ddl,
 			 bool is_internal,
 			 Oid *constraintId)
 {
@@ -755,7 +755,7 @@ index_create(Relation heapRelation,
 	if (indexInfo->ii_NumIndexAttrs < 1)
 		elog(ERROR, "must index at least one column");
 
-	if (!allow_system_table_mods &&
+	if (!allow_system_table_ddl &&
 		IsSystemRelation(heapRelation) &&
 		IsNormalProcessingMode())
 		ereport(ERROR,
@@ -886,7 +886,7 @@ index_create(Relation heapRelation,
 								relpersistence,
 								shared_relation,
 								mapped_relation,
-								allow_system_table_mods,
+								allow_system_table_ddl,
 								&relfrozenxid,
 								&relminmxid);
 
@@ -1011,7 +1011,7 @@ index_create(Relation heapRelation,
 												indexRelationName,
 												constraintType,
 												constr_flags,
-												allow_system_table_mods,
+												allow_system_table_ddl,
 												is_internal);
 			if (constraintId)
 				*constraintId = localaddr.objectId;
@@ -1668,7 +1668,7 @@ index_concurrently_set_dead(Oid heapId, Oid indexId)
  *		INDEX_CONSTR_CREATE_UPDATE_INDEX: update the pg_index row
  *		INDEX_CONSTR_CREATE_REMOVE_OLD_DEPS: remove existing dependencies
  *			of index on table's columns
- * allow_system_table_mods: allow table to be a system catalog
+ * allow_system_table_ddl: allow table to be a system catalog
  * is_internal: index is constructed due to internal process
  */
 ObjectAddress
@@ -1679,7 +1679,7 @@ index_constraint_create(Relation heapRelation,
 						const char *constraintName,
 						char constraintType,
 						bits16 constr_flags,
-						bool allow_system_table_mods,
+						bool allow_system_table_ddl,
 						bool is_internal)
 {
 	Oid			namespaceId = RelationGetNamespace(heapRelation);
@@ -1701,7 +1701,7 @@ index_constraint_create(Relation heapRelation,
 	Assert(!IsBootstrapProcessingMode());
 
 	/* enforce system-table restriction */
-	if (!allow_system_table_mods &&
+	if (!allow_system_table_ddl &&
 		IsSystemRelation(heapRelation) &&
 		IsNormalProcessingMode())
 		ereport(ERROR,
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 579b998954..b6bc6285be 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1009,7 +1009,7 @@ DefineIndex(Oid relationId,
 					 collationObjectId, classObjectId,
 					 coloptions, reloptions,
 					 flags, constr_flags,
-					 allowSystemTableMods, !check_rights,
+					 allowSystemTableDDL, !check_rights,
 					 &createdConstraintId);
 
 	ObjectAddressSet(address, RelationRelationId, indexRelationId);
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 1df76623ad..f400cbceaf 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -82,7 +82,7 @@ RangeVarCallbackForPolicy(const RangeVar *rv, Oid relid, Oid oldrelid,
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
 
 	/* No system table modifications unless explicitly allowed. */
-	if (!allowSystemTableMods && IsSystemClass(relid, classform))
+	if (!allowSystemTableDDL && IsSystemClass(relid, classform))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -395,7 +395,7 @@ RemovePolicyById(Oid policy_id)
 				 errmsg("\"%s\" is not a table",
 						RelationGetRelationName(rel))));
 
-	if (!allowSystemTableMods && IsSystemRelation(rel))
+	if (!allowSystemTableDDL && IsSystemRelation(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -485,7 +485,7 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
 				 errmsg("\"%s\" is not a table",
 						RelationGetRelationName(rel))));
 
-	if (!allowSystemTableMods && IsSystemRelation(rel))
+	if (!allowSystemTableDDL && IsSystemRelation(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index 6cf94a3140..6e7956706b 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -100,7 +100,7 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString,
 	check_is_member_of_role(saved_uid, owner_uid);
 
 	/* Additional check to protect reserved schema names */
-	if (!allowSystemTableMods && IsReservedName(schemaName))
+	if (!allowSystemTableDDL && IsReservedName(schemaName))
 		ereport(ERROR,
 				(errcode(ERRCODE_RESERVED_NAME),
 				 errmsg("unacceptable schema name \"%s\"", schemaName),
@@ -276,7 +276,7 @@ RenameSchema(const char *oldname, const char *newname)
 		aclcheck_error(aclresult, OBJECT_DATABASE,
 					   get_database_name(MyDatabaseId));
 
-	if (!allowSystemTableMods && IsReservedName(newname))
+	if (!allowSystemTableDDL && IsReservedName(newname))
 		ereport(ERROR,
 				(errcode(ERRCODE_RESERVED_NAME),
 				 errmsg("unacceptable schema name \"%s\"", newname),
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7dcd634a1a..d633442834 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -864,7 +864,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 										  stmt->oncommit,
 										  reloptions,
 										  true,
-										  allowSystemTableMods,
+										  allowSystemTableDDL,
 										  false,
 										  InvalidOid,
 										  typaddress);
@@ -1444,7 +1444,7 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
 	}
 
 	/* In the case of an invalid index, it is fine to bypass this check */
-	if (!invalid_system_index && !allowSystemTableMods && IsSystemClass(relOid, classform))
+	if (!invalid_system_index && !allowSystemTableDDL && IsSystemClass(relOid, classform))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -1926,7 +1926,7 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
 		aclcheck_error(aclresult, get_relkind_objtype(reltuple->relkind),
 					   relname);
 
-	if (!allowSystemTableMods && IsSystemClass(relid, reltuple))
+	if (!allowSystemTableDDL && IsSystemClass(relid, reltuple))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -2899,7 +2899,7 @@ renameatt_check(Oid myrelid, Form_pg_class classform, bool recursing)
 	if (!pg_class_ownercheck(myrelid, GetUserId()))
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(myrelid)),
 					   NameStr(classform->relname));
-	if (!allowSystemTableMods && IsSystemClass(myrelid, classform))
+	if (!allowSystemTableDDL && IsSystemClass(myrelid, classform))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -5146,7 +5146,7 @@ ATSimplePermissions(Relation rel, int allowed_targets)
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(rel->rd_rel->relkind),
 					   RelationGetRelationName(rel));
 
-	if (!allowSystemTableMods && IsSystemRelation(rel))
+	if (!allowSystemTableDDL && IsSystemRelation(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -6659,7 +6659,7 @@ ATPrepSetStatistics(Relation rel, const char *colName, int16 colNum, Node *newVa
 	 * We do our own permission checking because (a) we want to allow SET
 	 * STATISTICS on indexes (for expressional index columns), and (b) we want
 	 * to allow SET STATISTICS on system catalogs without requiring
-	 * allowSystemTableMods to be turned on.
+	 * allowSystemTableDDL to be turned on.
 	 */
 	if (rel->rd_rel->relkind != RELKIND_RELATION &&
 		rel->rd_rel->relkind != RELKIND_MATVIEW &&
@@ -7292,7 +7292,7 @@ ATExecAddIndexConstraint(AlteredTableInfo *tab, Relation rel,
 									  constraintName,
 									  constraintType,
 									  flags,
-									  allowSystemTableMods,
+									  allowSystemTableDDL,
 									  false);	/* is_internal */
 
 	index_close(indexRel, NoLock);
@@ -7616,7 +7616,7 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 				 errmsg("referenced relation \"%s\" is not a table",
 						RelationGetRelationName(pkrel))));
 
-	if (!allowSystemTableMods && IsSystemRelation(pkrel))
+	if (!allowSystemTableDDL && IsSystemRelation(pkrel))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -14775,7 +14775,7 @@ RangeVarCallbackOwnsRelation(const RangeVar *relation,
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relId)),
 					   relation->relname);
 
-	if (!allowSystemTableMods &&
+	if (!allowSystemTableDDL &&
 		IsSystemClass(relId, (Form_pg_class) GETSTRUCT(tuple)))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -14811,7 +14811,7 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid,
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
 
 	/* No system table modifications unless explicitly allowed. */
-	if (!allowSystemTableMods && IsSystemClass(relid, classform))
+	if (!allowSystemTableDDL && IsSystemClass(relid, classform))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 5e43867e6f..5c278b9c2a 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -300,7 +300,7 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
 	 * Disallow creation of tablespaces named "pg_xxx"; we reserve this
 	 * namespace for system purposes.
 	 */
-	if (!allowSystemTableMods && IsReservedName(stmt->tablespacename))
+	if (!allowSystemTableDDL && IsReservedName(stmt->tablespacename))
 		ereport(ERROR,
 				(errcode(ERRCODE_RESERVED_NAME),
 				 errmsg("unacceptable tablespace name \"%s\"",
@@ -951,7 +951,7 @@ RenameTableSpace(const char *oldname, const char *newname)
 		aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_TABLESPACE, oldname);
 
 	/* Validate new name */
-	if (!allowSystemTableMods && IsReservedName(newname))
+	if (!allowSystemTableDDL && IsReservedName(newname))
 		ereport(ERROR,
 				(errcode(ERRCODE_RESERVED_NAME),
 				 errmsg("unacceptable tablespace name \"%s\"", newname),
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 316692b7c2..0c129bb853 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -313,7 +313,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
 				 errmsg("\"%s\" is not a table or view",
 						RelationGetRelationName(rel))));
 
-	if (!allowSystemTableMods && IsSystemRelation(rel))
+	if (!allowSystemTableDDL && IsSystemRelation(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -1539,7 +1539,7 @@ RemoveTriggerById(Oid trigOid)
 				 errmsg("\"%s\" is not a table, view, or foreign table",
 						RelationGetRelationName(rel))));
 
-	if (!allowSystemTableMods && IsSystemRelation(rel))
+	if (!allowSystemTableDDL && IsSystemRelation(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -1648,7 +1648,7 @@ RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid,
 	/* you must own the table to rename one of its triggers */
 	if (!pg_class_ownercheck(relid, GetUserId()))
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
-	if (!allowSystemTableMods && IsSystemClass(relid, form))
+	if (!allowSystemTableDDL && IsSystemClass(relid, form))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 688ad439ed..c336f1081b 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -745,7 +745,7 @@ PostmasterMain(int argc, char *argv[])
 				break;
 
 			case 'O':
-				SetConfigOption("allow_system_table_mods", "true", PGC_POSTMASTER, PGC_S_ARGV);
+				SetConfigOption("allow_system_table_ddl", "true", PGC_POSTMASTER, PGC_S_ARGV);
 				break;
 
 			case 'o':
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index 7df2b6154c..7a2df2cd4e 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -271,7 +271,7 @@ DefineQueryRewrite(const char *rulename,
 				 errmsg("\"%s\" is not a table or view",
 						RelationGetRelationName(event_relation))));
 
-	if (!allowSystemTableMods && IsSystemRelation(event_relation))
+	if (!allowSystemTableDDL && IsSystemRelation(event_relation))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
@@ -927,7 +927,7 @@ RangeVarCallbackForRenameRule(const RangeVar *rv, Oid relid, Oid oldrelid,
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("\"%s\" is not a table or view", rv->relname)));
 
-	if (!allowSystemTableMods && IsSystemClass(relid, form))
+	if (!allowSystemTableDDL && IsSystemClass(relid, form))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied: \"%s\" is a system catalog",
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 44a59e1d4f..d4fa62f227 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3572,7 +3572,7 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx,
 				break;
 
 			case 'O':
-				SetConfigOption("allow_system_table_mods", "true", ctx, gucsource);
+				SetConfigOption("allow_system_table_ddl", "true", ctx, gucsource);
 				break;
 
 			case 'o':
diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 3b4f21bc54..baefb94209 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -974,7 +974,7 @@ perform_relmap_update(bool shared, const RelMapFile *updates)
 	 * Apply the updates to newmap.  No new mappings should appear, unless
 	 * somebody is adding indexes to system catalogs.
 	 */
-	merge_map_updates(&newmap, updates, allowSystemTableMods);
+	merge_map_updates(&newmap, updates, allowSystemTableDDL);
 
 	/* Write out the updated map and do other necessary tasks */
 	write_relmap_file(shared, &newmap, true, true, true,
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 3bf96de256..50304f070a 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -117,7 +117,7 @@ int			DateOrder = DATEORDER_MDY;
 int			IntervalStyle = INTSTYLE_POSTGRES;
 
 bool		enableFsync = true;
-bool		allowSystemTableMods = false;
+bool		allowSystemTableDDL = false;
 int			work_mem = 1024;
 int			maintenance_work_mem = 16384;
 int			max_parallel_maintenance_workers = 2;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1208eb9a68..941a0e2d24 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1772,12 +1772,12 @@ static struct config_bool ConfigureNamesBool[] =
 	},
 
 	{
-		{"allow_system_table_mods", PGC_POSTMASTER, DEVELOPER_OPTIONS,
-			gettext_noop("Allows modifications of the structure of system tables."),
+		{"allow_system_table_ddl", PGC_SUSET, DEVELOPER_OPTIONS,
+			gettext_noop("Allows DDL on system tables."),
 			NULL,
 			GUC_NOT_IN_SAMPLE
 		},
-		&allowSystemTableMods,
+		&allowSystemTableDDL,
 		false,
 		NULL, NULL, NULL
 	},
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index eec71c29d5..342337c3fa 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -56,7 +56,7 @@ extern Relation heap_create(const char *relname,
 							char relpersistence,
 							bool shared_relation,
 							bool mapped_relation,
-							bool allow_system_table_mods,
+							bool allow_system_table_ddl,
 							TransactionId *relfrozenxid,
 							MultiXactId *relminmxid);
 
@@ -77,7 +77,7 @@ extern Oid	heap_create_with_catalog(const char *relname,
 									 OnCommitAction oncommit,
 									 Datum reloptions,
 									 bool use_user_acl,
-									 bool allow_system_table_mods,
+									 bool allow_system_table_ddl,
 									 bool is_internal,
 									 Oid relrewrite,
 									 ObjectAddress *typaddress);
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 1113d25b2d..0a9280c8f6 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -68,7 +68,7 @@ extern Oid	index_create(Relation heapRelation,
 						 Datum reloptions,
 						 bits16 flags,
 						 bits16 constr_flags,
-						 bool allow_system_table_mods,
+						 bool allow_system_table_ddl,
 						 bool is_internal,
 						 Oid *constraintId);
 
@@ -99,7 +99,7 @@ extern ObjectAddress index_constraint_create(Relation heapRelation,
 											 const char *constraintName,
 											 char constraintType,
 											 bits16 constr_flags,
-											 bool allow_system_table_mods,
+											 bool allow_system_table_ddl,
 											 bool is_internal);
 
 extern void index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode);
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 61a24c2e3c..ce5d27ca33 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -241,7 +241,7 @@ extern PGDLLIMPORT int IntervalStyle;
 #define MAXTZLEN		10		/* max TZ name len, not counting tr. null */
 
 extern bool enableFsync;
-extern PGDLLIMPORT bool allowSystemTableMods;
+extern PGDLLIMPORT bool allowSystemTableDDL;
 extern PGDLLIMPORT int work_mem;
 extern PGDLLIMPORT int maintenance_work_mem;
 extern PGDLLIMPORT int max_parallel_maintenance_workers;
diff --git a/src/test/regress/expected/alter_system_table.out b/src/test/regress/expected/alter_system_table.out
new file mode 100644
index 0000000000..29dc7f05c1
--- /dev/null
+++ b/src/test/regress/expected/alter_system_table.out
@@ -0,0 +1,170 @@
+--
+-- Tests for things affected by allow_system_table_ddl
+--
+-- We run the same set of commands once with allow_system_table_ddl
+-- off and then again with on.
+--
+-- The "on" tests should where possible be wrapped in BEGIN/ROLLBACK
+-- blocks so as to not leave a mess around.  Also, be sure to clean up
+-- to not leave objects that cannot be dumped or restored by the
+-- pg_upgrade test suite.
+CREATE USER regress_user_ast;
+SET allow_system_table_ddl = off;
+-- create new table in pg_catalog
+CREATE TABLE pg_catalog.test (a int);
+ERROR:  permission denied to create "pg_catalog.test"
+DETAIL:  System catalog modifications are currently disallowed.
+-- anyarray column
+CREATE TABLE t1x (a int, b anyarray);
+ERROR:  column "b" has pseudo-type anyarray
+-- index on system catalog
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+ERROR:  permission denied: "pg_namespace" is a system catalog
+-- write to system catalog table as superuser
+-- (allowed even without allow_system_table_ddl)
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 0, 'foo');
+-- write to system catalog table as normal user
+GRANT INSERT ON pg_description TO regress_user_ast;
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 1, 'foo');
+ERROR:  permission denied for table pg_description
+RESET ROLE;
+-- policy on system catalog
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+ERROR:  permission denied: "pg_description" is a system catalog
+-- reserved schema name
+CREATE SCHEMA pg_foo;
+ERROR:  unacceptable schema name "pg_foo"
+DETAIL:  The prefix "pg_" is reserved for system schemas.
+-- drop system table
+DROP TABLE pg_description;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- truncate of system table
+TRUNCATE pg_description;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- rename column of system table
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- ATSimplePermissions()
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- SET STATISTICS
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- foreign key referencing catalog
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ERROR:  permission denied: "pg_description" is a system catalog
+-- RangeVarCallbackOwnsRelation()
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+ERROR:  permission denied: "pg_description" is a system catalog
+-- RangeVarCallbackForAlterRelation()
+ALTER TABLE pg_description RENAME TO pg_comment;
+ERROR:  permission denied: "pg_description" is a system catalog
+ALTER TABLE pg_description SET SCHEMA public;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+ERROR:  unacceptable tablespace name "pg_foo"
+DETAIL:  The prefix "pg_" is reserved for system tablespaces.
+-- triggers
+CREATE FUNCTION tf1() RETURNS trigger
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RETURN NULL;
+END $$;
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ERROR:  permission denied: "pg_description" is a system catalog
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+ERROR:  permission denied: "pg_description" is a system catalog
+--DROP TRIGGER t2 ON pg_description;
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ERROR:  permission denied: "pg_description" is a system catalog
+ALTER RULE r1 ON pg_description RENAME TO r2;
+ERROR:  permission denied: "pg_description" is a system catalog
+--DROP RULE r2 ON pg_description;
+SET allow_system_table_ddl = on;
+-- create new table in pg_catalog
+BEGIN;
+CREATE TABLE pg_catalog.test (a int);
+ROLLBACK;
+-- anyarray column
+BEGIN;
+CREATE TABLE t1 (a int, b anyarray);
+ROLLBACK;
+-- index on system catalog
+BEGIN;
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+ROLLBACK;
+-- write to system catalog table as superuser
+BEGIN;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 2, 'foo');
+ROLLBACK;
+-- write to system catalog table as normal user
+-- (not allowed)
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 3, 'foo');
+ERROR:  permission denied for table pg_description
+RESET ROLE;
+-- policy on system catalog
+BEGIN;
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+ROLLBACK;
+-- reserved schema name
+BEGIN;
+CREATE SCHEMA pg_foo;
+ROLLBACK;
+-- drop system table
+-- (This will fail anyway because it's pinned.)
+BEGIN;
+DROP TABLE pg_description;
+ERROR:  cannot drop table pg_description because it is required by the database system
+ROLLBACK;
+-- truncate of system table
+BEGIN;
+TRUNCATE pg_description;
+ROLLBACK;
+-- rename column of system table
+BEGIN;
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+ROLLBACK;
+-- ATSimplePermissions()
+BEGIN;
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+ROLLBACK;
+-- SET STATISTICS
+BEGIN;
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+ROLLBACK;
+-- foreign key referencing catalog
+BEGIN;
+ALTER TABLE pg_description ADD PRIMARY KEY USING INDEX pg_description_o_c_o_index;
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ROLLBACK;
+-- RangeVarCallbackOwnsRelation()
+BEGIN;
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+ROLLBACK;
+-- RangeVarCallbackForAlterRelation()
+BEGIN;
+ALTER TABLE pg_description RENAME TO pg_comment;
+ROLLBACK;
+BEGIN;
+ALTER TABLE pg_description SET SCHEMA public;
+ROLLBACK;
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+ERROR:  directory "/no/such/location" does not exist
+-- triggers
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+DROP TRIGGER t2 ON pg_description;
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ALTER RULE r1 ON pg_description RENAME TO r2;
+DROP RULE r2 ON pg_description;
+-- cleanup
+REVOKE ALL ON pg_description FROM regress_user_ast;
+DROP USER regress_user_ast;
+DROP FUNCTION tf1;
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index a639601f66..58892bbb2b 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3221,13 +3221,9 @@ WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
 
 -- Checks on creating and manipulation of user defined relations in
 -- pg_catalog.
---
--- XXX: It would be useful to add checks around trying to manipulate
--- catalog tables, but that might have ugly consequences when run
--- against an existing server with allow_system_table_mods = on.
-SHOW allow_system_table_mods;
- allow_system_table_mods 
--------------------------
+SHOW allow_system_table_ddl;
+ allow_system_table_ddl 
+------------------------
  off
 (1 row)
 
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index f23fe8d870..72c0e515b2 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -111,7 +111,7 @@ test: plancache limit plpgsql copy2 temp domain rangefuncs prepare conversion tr
 # ----------
 # Another group of parallel tests
 # ----------
-test: partition_join partition_prune reloptions hash_part indexing partition_aggregate partition_info
+test: partition_join partition_prune reloptions hash_part indexing partition_aggregate partition_info alter_system_table
 
 # event triggers cannot run concurrently with any test that runs DDL
 test: event_trigger
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index ca200eb599..7cacbffae8 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -191,6 +191,7 @@ test: hash_part
 test: indexing
 test: partition_aggregate
 test: partition_info
+test: alter_system_table
 test: event_trigger
 test: fast_default
 test: stats
diff --git a/src/test/regress/sql/alter_system_table.sql b/src/test/regress/sql/alter_system_table.sql
new file mode 100644
index 0000000000..9753cef11d
--- /dev/null
+++ b/src/test/regress/sql/alter_system_table.sql
@@ -0,0 +1,187 @@
+--
+-- Tests for things affected by allow_system_table_ddl
+--
+-- We run the same set of commands once with allow_system_table_ddl
+-- off and then again with on.
+--
+-- The "on" tests should where possible be wrapped in BEGIN/ROLLBACK
+-- blocks so as to not leave a mess around.  Also, be sure to clean up
+-- to not leave objects that cannot be dumped or restored by the
+-- pg_upgrade test suite.
+
+CREATE USER regress_user_ast;
+
+SET allow_system_table_ddl = off;
+
+-- create new table in pg_catalog
+CREATE TABLE pg_catalog.test (a int);
+
+-- anyarray column
+CREATE TABLE t1x (a int, b anyarray);
+
+-- index on system catalog
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+
+-- write to system catalog table as superuser
+-- (allowed even without allow_system_table_ddl)
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 0, 'foo');
+
+-- write to system catalog table as normal user
+GRANT INSERT ON pg_description TO regress_user_ast;
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 1, 'foo');
+RESET ROLE;
+
+-- policy on system catalog
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+
+-- reserved schema name
+CREATE SCHEMA pg_foo;
+
+-- drop system table
+DROP TABLE pg_description;
+
+-- truncate of system table
+TRUNCATE pg_description;
+
+-- rename column of system table
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+
+-- ATSimplePermissions()
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+
+-- SET STATISTICS
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+
+-- foreign key referencing catalog
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+
+-- RangeVarCallbackOwnsRelation()
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+
+-- RangeVarCallbackForAlterRelation()
+ALTER TABLE pg_description RENAME TO pg_comment;
+ALTER TABLE pg_description SET SCHEMA public;
+
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+
+-- triggers
+CREATE FUNCTION tf1() RETURNS trigger
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RETURN NULL;
+END $$;
+
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+--DROP TRIGGER t2 ON pg_description;
+
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ALTER RULE r1 ON pg_description RENAME TO r2;
+--DROP RULE r2 ON pg_description;
+
+
+SET allow_system_table_ddl = on;
+
+-- create new table in pg_catalog
+BEGIN;
+CREATE TABLE pg_catalog.test (a int);
+ROLLBACK;
+
+-- anyarray column
+BEGIN;
+CREATE TABLE t1 (a int, b anyarray);
+ROLLBACK;
+
+-- index on system catalog
+BEGIN;
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+ROLLBACK;
+
+-- write to system catalog table as superuser
+BEGIN;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 2, 'foo');
+ROLLBACK;
+
+-- write to system catalog table as normal user
+-- (not allowed)
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 3, 'foo');
+RESET ROLE;
+
+-- policy on system catalog
+BEGIN;
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+ROLLBACK;
+
+-- reserved schema name
+BEGIN;
+CREATE SCHEMA pg_foo;
+ROLLBACK;
+
+-- drop system table
+-- (This will fail anyway because it's pinned.)
+BEGIN;
+DROP TABLE pg_description;
+ROLLBACK;
+
+-- truncate of system table
+BEGIN;
+TRUNCATE pg_description;
+ROLLBACK;
+
+-- rename column of system table
+BEGIN;
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+ROLLBACK;
+
+-- ATSimplePermissions()
+BEGIN;
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+ROLLBACK;
+
+-- SET STATISTICS
+BEGIN;
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+ROLLBACK;
+
+-- foreign key referencing catalog
+BEGIN;
+ALTER TABLE pg_description ADD PRIMARY KEY USING INDEX pg_description_o_c_o_index;
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ROLLBACK;
+
+-- RangeVarCallbackOwnsRelation()
+BEGIN;
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+ROLLBACK;
+
+-- RangeVarCallbackForAlterRelation()
+BEGIN;
+ALTER TABLE pg_description RENAME TO pg_comment;
+ROLLBACK;
+BEGIN;
+ALTER TABLE pg_description SET SCHEMA public;
+ROLLBACK;
+
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+
+-- triggers
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+DROP TRIGGER t2 ON pg_description;
+
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ALTER RULE r1 ON pg_description RENAME TO r2;
+DROP RULE r2 ON pg_description;
+
+
+-- cleanup
+REVOKE ALL ON pg_description FROM regress_user_ast;
+DROP USER regress_user_ast;
+DROP FUNCTION tf1;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 0369909b50..0c87a54b8e 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2016,12 +2016,8 @@ CREATE TEMP TABLE filenode_mapping AS
 
 -- Checks on creating and manipulation of user defined relations in
 -- pg_catalog.
---
--- XXX: It would be useful to add checks around trying to manipulate
--- catalog tables, but that might have ugly consequences when run
--- against an existing server with allow_system_table_mods = on.
 
-SHOW allow_system_table_mods;
+SHOW allow_system_table_ddl;
 -- disallowed because of search_path issues with pg_dump
 CREATE TABLE pg_catalog.new_system_table();
 -- instead create in public first, move to catalog

base-commit: 74b7cc8c02137f059703972aa14445b1e073f005
-- 
2.22.0

#19

Bruce Momjian

bruce@momjian.us

over 6 years ago

In reply to: Tom Lane (#16)

Re: allow_system_table_mods stuff

On Mon, Jun 24, 2019 at 11:20:51AM -0400, Tom Lane wrote:

I do see value in two switches not one, but it's what I said above,
to not need to give people *more* chance-to-break-things than they
had before when doing manual catalog fixes. That is, we need a
setting that corresponds more or less to current default behavior.

There's an aesthetic argument to be had about whether to have two
bools or one three-way switch, but I prefer the former; there's
no backward-compatibility issue here since allow_system_table_mods
couldn't be set by applications anyway.

I like a single three-way switch since if you are allowing DDL, you
probably don't care if you restrict DML. log_statement already has a
similar distinction with values of none, ddl, mod, all. I assume
allow_system_table_mods could have value of false, dml, true.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

#20

Bruce Momjian

bruce@momjian.us

over 6 years ago

In reply to: Bruce Momjian (#19)

Re: allow_system_table_mods stuff

On Sun, Jul 7, 2019 at 11:45:49PM -0400, Bruce Momjian wrote:

On Mon, Jun 24, 2019 at 11:20:51AM -0400, Tom Lane wrote:

I do see value in two switches not one, but it's what I said above,
to not need to give people *more* chance-to-break-things than they
had before when doing manual catalog fixes. That is, we need a
setting that corresponds more or less to current default behavior.

There's an aesthetic argument to be had about whether to have two
bools or one three-way switch, but I prefer the former; there's
no backward-compatibility issue here since allow_system_table_mods
couldn't be set by applications anyway.

I like a single three-way switch since if you are allowing DDL, you
probably don't care if you restrict DML. log_statement already has a
similar distinction with values of none, ddl, mod, all. I assume
allow_system_table_mods could have value of false, dml, true.

Or, to match log_statement, use: none, dml, all.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

#21

Alvaro Herrera

alvherre@2ndquadrant.com

over 6 years ago

In reply to: Peter Eisentraut (#18)

Re: allow_system_table_mods stuff

On 2019-Jun-28, Peter Eisentraut wrote:

Here is a new patch after the discussion.

- Rename allow_system_table_mods to allow_system_table_ddl.

(This makes room for a new allow_system_table_dml, but it's not
implemented here.)

- Make allow_system_table_ddl SUSET.

- Add regression test.

- Remove the behavior that allow_system_table_mods allowed
non-superusers to do DML on catalog tables without further access checking.

I think there was general agreement on all these points.

I think this patch is at a point where it merits closer review from
fellow committers, so I marked it RfC for now. I hope non-committers
would also look at it some more, though.

--
ï¿½lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#22

Michael Paquier

michael@paquier.xyz

about 6 years ago

In reply to: Alvaro Herrera (#21)

Re: allow_system_table_mods stuff

On Fri, Sep 13, 2019 at 06:39:40PM -0300, Alvaro Herrera wrote:

I think this patch is at a point where it merits closer review from
fellow committers, so I marked it RfC for now. I hope non-committers
would also look at it some more, though.

I guess so. The patch has conflicts in the serial and parallel
schedules, so I have moved it to next CF, waiting on author for a
rebase.

Peter, are you planning to look at that again? Note: the patch has no
reviewers registered.
--
Michael

#23

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

about 6 years ago

In reply to: Michael Paquier (#22)

3 attachment(s)

Re: allow_system_table_mods stuff

On 2019-11-27 09:26, Michael Paquier wrote:

On Fri, Sep 13, 2019 at 06:39:40PM -0300, Alvaro Herrera wrote:

I think this patch is at a point where it merits closer review from
fellow committers, so I marked it RfC for now. I hope non-committers
would also look at it some more, though.

I guess so. The patch has conflicts in the serial and parallel
schedules, so I have moved it to next CF, waiting on author for a
rebase.

Peter, are you planning to look at that again? Note: the patch has no
reviewers registered.

Here is an updated patch series.

After re-reading the discussion again, I have kept the existing name of
the option. I have also moved the tests to the "unsafe_tests" suite,
which seems like a better place. And I have split the patch into three.

Other than those cosmetic changes, I think everything here has been
discussed and agreed to, so unless anyone expresses any concern or a
wish to do more review, I think this is ready to commit.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

v3-0001-Remove-any-user-DML-capability-from-allow_system_.patchtext/plain; charset=UTF-8; name=v3-0001-Remove-any-user-DML-capability-from-allow_system_.patch; x-mac-creator=0; x-mac-type=0Download

From fbe9dd7d9fb4674cf2ca25b6cd4f05556c201d89 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 27 Nov 2019 16:32:50 +0100
Subject: [PATCH v3 1/3] Remove any-user DML capability from
 allow_system_table_mods

Previously, allow_system_table_mods allowed a non-superuser to do DML
on a system table without further permission checks.  This has been
removed, as it was quite inconsistent with the rest of the meaning of
this setting.  (Since allow_system_table_mods was previously only
accessible with a server restart, it is unlikely that anyone was using
this possibility.)

Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com
---
 src/backend/catalog/aclchk.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index bed10f9409..ea5666ebb8 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3851,7 +3851,7 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
 
 	/*
 	 * Deny anyone permission to update a system catalog unless
-	 * pg_authid.rolsuper is set.  Also allow it if allowSystemTableMods.
+	 * pg_authid.rolsuper is set.
 	 *
 	 * As of 7.4 we have some updatable system views; those shouldn't be
 	 * protected in this way.  Assume the view rules can take care of
@@ -3860,8 +3860,7 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
 	if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) &&
 		IsSystemClass(table_oid, classForm) &&
 		classForm->relkind != RELKIND_VIEW &&
-		!superuser_arg(roleid) &&
-		!allowSystemTableMods)
+		!superuser_arg(roleid))
 	{
 #ifdef ACLDEBUG
 		elog(DEBUG2, "permission denied for system catalog update");

base-commit: ca266a069a20c32a8f0a1df982a5a67d9483bcb3
-- 
2.24.0

v3-0002-Make-allow_system_table_mods-settable-at-run-time.patchtext/plain; charset=UTF-8; name=v3-0002-Make-allow_system_table_mods-settable-at-run-time.patch; x-mac-creator=0; x-mac-type=0Download

From 3a78625fbe62ae78279357e534825a53cb11051a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 27 Nov 2019 16:34:27 +0100
Subject: [PATCH v3 2/3] Make allow_system_table_mods settable at run time

Make allow_system_table_mods settable at run time by superusers.  It
was previously postmaster start only.

We don't want to make system catalog DDL wide-open, but there are
occasionally useful things to do like setting reloptions or statistics
on a busy system table, and blocking those doesn't help anyone.  Also,
this enables the possibility of writing a test suite for this setting.

Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com
---
 doc/src/sgml/config.sgml     | 9 ++++++---
 src/backend/utils/misc/guc.c | 2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index d4d1fe45cc..34fd759862 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9501,9 +9501,12 @@ <title>Developer Options</title>
       </term>
       <listitem>
        <para>
-        Allows modification of the structure of system tables.
-        This is used by <command>initdb</command>.
-        This parameter can only be set at server start.
+        Allows modification of the structure of system tables as well as
+        certain other risky actions on system tables.  This is otherwise not
+        allowed even for superusers.  This is used by
+        <command>initdb</command>.  Inconsiderate use of this setting can
+        cause irretrievable data loss or seriously corrupt the database
+        system.  Only superusers can change this setting.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ba4edde71a..5fccc9683e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1777,7 +1777,7 @@ static struct config_bool ConfigureNamesBool[] =
 	},
 
 	{
-		{"allow_system_table_mods", PGC_POSTMASTER, DEVELOPER_OPTIONS,
+		{"allow_system_table_mods", PGC_SUSET, DEVELOPER_OPTIONS,
 			gettext_noop("Allows modifications of the structure of system tables."),
 			NULL,
 			GUC_NOT_IN_SAMPLE
-- 
2.24.0

v3-0003-Add-a-regression-test-for-allow_system_table_mods.patchtext/plain; charset=UTF-8; name=v3-0003-Add-a-regression-test-for-allow_system_table_mods.patch; x-mac-creator=0; x-mac-type=0Download

From e07165d97290a882b30a6180c142d749fd72a16f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 27 Nov 2019 16:39:38 +0100
Subject: [PATCH v3 3/3] Add a regression test for allow_system_table_mods

Add a regression test file that exercises the kinds of commands that
allow_system_table_mods allows.

This is put in the "unsafe_tests" suite, so it won't accidentally
create a mess if someone runs the normal regression tests against an
instance that they care about.

Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com
---
 src/test/modules/unsafe_tests/Makefile        |   2 +-
 .../expected/alter_system_table.out           | 168 ++++++++++++++++
 .../unsafe_tests/sql/alter_system_table.sql   | 185 ++++++++++++++++++
 src/test/regress/expected/alter_table.out     |   4 -
 src/test/regress/sql/alter_table.sql          |   4 -
 5 files changed, 354 insertions(+), 9 deletions(-)
 create mode 100644 src/test/modules/unsafe_tests/expected/alter_system_table.out
 create mode 100644 src/test/modules/unsafe_tests/sql/alter_system_table.sql

diff --git a/src/test/modules/unsafe_tests/Makefile b/src/test/modules/unsafe_tests/Makefile
index 321252f8d5..3ecf5fcfc5 100644
--- a/src/test/modules/unsafe_tests/Makefile
+++ b/src/test/modules/unsafe_tests/Makefile
@@ -1,6 +1,6 @@
 # src/test/modules/unsafe_tests/Makefile
 
-REGRESS = rolenames
+REGRESS = rolenames alter_system_table
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
diff --git a/src/test/modules/unsafe_tests/expected/alter_system_table.out b/src/test/modules/unsafe_tests/expected/alter_system_table.out
new file mode 100644
index 0000000000..09a557f837
--- /dev/null
+++ b/src/test/modules/unsafe_tests/expected/alter_system_table.out
@@ -0,0 +1,168 @@
+--
+-- Tests for things affected by allow_system_table_mods
+--
+-- We run the same set of commands once with allow_system_table_mods
+-- off and then again with on.
+--
+-- The "on" tests should where possible be wrapped in BEGIN/ROLLBACK
+-- blocks so as to not leave a mess around.
+CREATE USER regress_user_ast;
+SET allow_system_table_mods = off;
+-- create new table in pg_catalog
+CREATE TABLE pg_catalog.test (a int);
+ERROR:  permission denied to create "pg_catalog.test"
+DETAIL:  System catalog modifications are currently disallowed.
+-- anyarray column
+CREATE TABLE t1x (a int, b anyarray);
+ERROR:  column "b" has pseudo-type anyarray
+-- index on system catalog
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+ERROR:  permission denied: "pg_namespace" is a system catalog
+-- write to system catalog table as superuser
+-- (allowed even without allow_system_table_mods)
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 0, 'foo');
+-- write to system catalog table as normal user
+GRANT INSERT ON pg_description TO regress_user_ast;
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 1, 'foo');
+ERROR:  permission denied for table pg_description
+RESET ROLE;
+-- policy on system catalog
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+ERROR:  permission denied: "pg_description" is a system catalog
+-- reserved schema name
+CREATE SCHEMA pg_foo;
+ERROR:  unacceptable schema name "pg_foo"
+DETAIL:  The prefix "pg_" is reserved for system schemas.
+-- drop system table
+DROP TABLE pg_description;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- truncate of system table
+TRUNCATE pg_description;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- rename column of system table
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- ATSimplePermissions()
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- SET STATISTICS
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- foreign key referencing catalog
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ERROR:  permission denied: "pg_description" is a system catalog
+-- RangeVarCallbackOwnsRelation()
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+ERROR:  permission denied: "pg_description" is a system catalog
+-- RangeVarCallbackForAlterRelation()
+ALTER TABLE pg_description RENAME TO pg_comment;
+ERROR:  permission denied: "pg_description" is a system catalog
+ALTER TABLE pg_description SET SCHEMA public;
+ERROR:  permission denied: "pg_description" is a system catalog
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+ERROR:  unacceptable tablespace name "pg_foo"
+DETAIL:  The prefix "pg_" is reserved for system tablespaces.
+-- triggers
+CREATE FUNCTION tf1() RETURNS trigger
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RETURN NULL;
+END $$;
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ERROR:  permission denied: "pg_description" is a system catalog
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+ERROR:  permission denied: "pg_description" is a system catalog
+--DROP TRIGGER t2 ON pg_description;
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ERROR:  permission denied: "pg_description" is a system catalog
+ALTER RULE r1 ON pg_description RENAME TO r2;
+ERROR:  permission denied: "pg_description" is a system catalog
+--DROP RULE r2 ON pg_description;
+SET allow_system_table_mods = on;
+-- create new table in pg_catalog
+BEGIN;
+CREATE TABLE pg_catalog.test (a int);
+ROLLBACK;
+-- anyarray column
+BEGIN;
+CREATE TABLE t1 (a int, b anyarray);
+ROLLBACK;
+-- index on system catalog
+BEGIN;
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+ROLLBACK;
+-- write to system catalog table as superuser
+BEGIN;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 2, 'foo');
+ROLLBACK;
+-- write to system catalog table as normal user
+-- (not allowed)
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 3, 'foo');
+ERROR:  permission denied for table pg_description
+RESET ROLE;
+-- policy on system catalog
+BEGIN;
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+ROLLBACK;
+-- reserved schema name
+BEGIN;
+CREATE SCHEMA pg_foo;
+ROLLBACK;
+-- drop system table
+-- (This will fail anyway because it's pinned.)
+BEGIN;
+DROP TABLE pg_description;
+ERROR:  cannot drop table pg_description because it is required by the database system
+ROLLBACK;
+-- truncate of system table
+BEGIN;
+TRUNCATE pg_description;
+ROLLBACK;
+-- rename column of system table
+BEGIN;
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+ROLLBACK;
+-- ATSimplePermissions()
+BEGIN;
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+ROLLBACK;
+-- SET STATISTICS
+BEGIN;
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+ROLLBACK;
+-- foreign key referencing catalog
+BEGIN;
+ALTER TABLE pg_description ADD PRIMARY KEY USING INDEX pg_description_o_c_o_index;
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ROLLBACK;
+-- RangeVarCallbackOwnsRelation()
+BEGIN;
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+ROLLBACK;
+-- RangeVarCallbackForAlterRelation()
+BEGIN;
+ALTER TABLE pg_description RENAME TO pg_comment;
+ROLLBACK;
+BEGIN;
+ALTER TABLE pg_description SET SCHEMA public;
+ROLLBACK;
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+ERROR:  directory "/no/such/location" does not exist
+-- triggers
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+DROP TRIGGER t2 ON pg_description;
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ALTER RULE r1 ON pg_description RENAME TO r2;
+DROP RULE r2 ON pg_description;
+-- cleanup
+REVOKE ALL ON pg_description FROM regress_user_ast;
+DROP USER regress_user_ast;
+DROP FUNCTION tf1;
diff --git a/src/test/modules/unsafe_tests/sql/alter_system_table.sql b/src/test/modules/unsafe_tests/sql/alter_system_table.sql
new file mode 100644
index 0000000000..60769c8d51
--- /dev/null
+++ b/src/test/modules/unsafe_tests/sql/alter_system_table.sql
@@ -0,0 +1,185 @@
+--
+-- Tests for things affected by allow_system_table_mods
+--
+-- We run the same set of commands once with allow_system_table_mods
+-- off and then again with on.
+--
+-- The "on" tests should where possible be wrapped in BEGIN/ROLLBACK
+-- blocks so as to not leave a mess around.
+
+CREATE USER regress_user_ast;
+
+SET allow_system_table_mods = off;
+
+-- create new table in pg_catalog
+CREATE TABLE pg_catalog.test (a int);
+
+-- anyarray column
+CREATE TABLE t1x (a int, b anyarray);
+
+-- index on system catalog
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+
+-- write to system catalog table as superuser
+-- (allowed even without allow_system_table_mods)
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 0, 'foo');
+
+-- write to system catalog table as normal user
+GRANT INSERT ON pg_description TO regress_user_ast;
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 1, 'foo');
+RESET ROLE;
+
+-- policy on system catalog
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+
+-- reserved schema name
+CREATE SCHEMA pg_foo;
+
+-- drop system table
+DROP TABLE pg_description;
+
+-- truncate of system table
+TRUNCATE pg_description;
+
+-- rename column of system table
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+
+-- ATSimplePermissions()
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+
+-- SET STATISTICS
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+
+-- foreign key referencing catalog
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+
+-- RangeVarCallbackOwnsRelation()
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+
+-- RangeVarCallbackForAlterRelation()
+ALTER TABLE pg_description RENAME TO pg_comment;
+ALTER TABLE pg_description SET SCHEMA public;
+
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+
+-- triggers
+CREATE FUNCTION tf1() RETURNS trigger
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RETURN NULL;
+END $$;
+
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+--DROP TRIGGER t2 ON pg_description;
+
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ALTER RULE r1 ON pg_description RENAME TO r2;
+--DROP RULE r2 ON pg_description;
+
+
+SET allow_system_table_mods = on;
+
+-- create new table in pg_catalog
+BEGIN;
+CREATE TABLE pg_catalog.test (a int);
+ROLLBACK;
+
+-- anyarray column
+BEGIN;
+CREATE TABLE t1 (a int, b anyarray);
+ROLLBACK;
+
+-- index on system catalog
+BEGIN;
+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;
+ROLLBACK;
+
+-- write to system catalog table as superuser
+BEGIN;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 2, 'foo');
+ROLLBACK;
+
+-- write to system catalog table as normal user
+-- (not allowed)
+SET ROLE regress_user_ast;
+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 3, 'foo');
+RESET ROLE;
+
+-- policy on system catalog
+BEGIN;
+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');
+ROLLBACK;
+
+-- reserved schema name
+BEGIN;
+CREATE SCHEMA pg_foo;
+ROLLBACK;
+
+-- drop system table
+-- (This will fail anyway because it's pinned.)
+BEGIN;
+DROP TABLE pg_description;
+ROLLBACK;
+
+-- truncate of system table
+BEGIN;
+TRUNCATE pg_description;
+ROLLBACK;
+
+-- rename column of system table
+BEGIN;
+ALTER TABLE pg_description RENAME COLUMN description TO comment;
+ROLLBACK;
+
+-- ATSimplePermissions()
+BEGIN;
+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;
+ROLLBACK;
+
+-- SET STATISTICS
+BEGIN;
+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;
+ROLLBACK;
+
+-- foreign key referencing catalog
+BEGIN;
+ALTER TABLE pg_description ADD PRIMARY KEY USING INDEX pg_description_o_c_o_index;
+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);
+ROLLBACK;
+
+-- RangeVarCallbackOwnsRelation()
+BEGIN;
+CREATE INDEX pg_descripton_test_index ON pg_description (description);
+ROLLBACK;
+
+-- RangeVarCallbackForAlterRelation()
+BEGIN;
+ALTER TABLE pg_description RENAME TO pg_comment;
+ROLLBACK;
+BEGIN;
+ALTER TABLE pg_description SET SCHEMA public;
+ROLLBACK;
+
+-- reserved tablespace name
+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+
+-- triggers
+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();
+ALTER TRIGGER t1 ON pg_description RENAME TO t2;
+DROP TRIGGER t2 ON pg_description;
+
+-- rules
+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;
+ALTER RULE r1 ON pg_description RENAME TO r2;
+DROP RULE r2 ON pg_description;
+
+
+-- cleanup
+REVOKE ALL ON pg_description FROM regress_user_ast;
+DROP USER regress_user_ast;
+DROP FUNCTION tf1;
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5189fd8188..f65611a62c 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3309,10 +3309,6 @@ WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
 
 -- Checks on creating and manipulation of user defined relations in
 -- pg_catalog.
---
--- XXX: It would be useful to add checks around trying to manipulate
--- catalog tables, but that might have ugly consequences when run
--- against an existing server with allow_system_table_mods = on.
 SHOW allow_system_table_mods;
  allow_system_table_mods 
 -------------------------
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 14d06c116f..abe7be3223 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2079,10 +2079,6 @@ CREATE TEMP TABLE filenode_mapping AS
 
 -- Checks on creating and manipulation of user defined relations in
 -- pg_catalog.
---
--- XXX: It would be useful to add checks around trying to manipulate
--- catalog tables, but that might have ugly consequences when run
--- against an existing server with allow_system_table_mods = on.
 
 SHOW allow_system_table_mods;
 -- disallowed because of search_path issues with pg_dump
-- 
2.24.0

#24

Tom Lane

tgl@sss.pgh.pa.us

about 6 years ago

In reply to: Peter Eisentraut (#23)

Re: allow_system_table_mods stuff

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2019-11-27 09:26, Michael Paquier wrote:

Peter, are you planning to look at that again? Note: the patch has no
reviewers registered.

Here is an updated patch series.

After re-reading the discussion again, I have kept the existing name of
the option. I have also moved the tests to the "unsafe_tests" suite,
which seems like a better place. And I have split the patch into three.

Personally I'd have gone with the renaming to allow_system_table_ddl,
but it's not a huge point. Updating the code to agree with that
naming would make the patch much more invasive, so maybe it's not
worth it.

Other than those cosmetic changes, I think everything here has been
discussed and agreed to, so unless anyone expresses any concern or a
wish to do more review, I think this is ready to commit.

I read through the patch set and have just one quibble: in the
proposed new docs,

+        Allows modification of the structure of system tables as well as
+        certain other risky actions on system tables.  This is otherwise not
+        allowed even for superusers.  This is used by
+        <command>initdb</command>.  Inconsiderate use of this setting can
+        cause irretrievable data loss or seriously corrupt the database
+        system.  Only superusers can change this setting.

"Inconsiderate" doesn't seem like le mot juste. Maybe "Ill-advised"?

(I'm also wondering whether the sentence about initdb is worth keeping.)

I marked the CF entry RFC.

regards, tom lane

#25

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

about 6 years ago

In reply to: Tom Lane (#24)

Re: allow_system_table_mods stuff

On 2019-11-28 17:11, Tom Lane wrote:

I read through the patch set and have just one quibble: in the
proposed new docs,
+        Allows modification of the structure of system tables as well as
+        certain other risky actions on system tables.  This is otherwise not
+        allowed even for superusers.  This is used by
+        <command>initdb</command>.  Inconsiderate use of this setting can
+        cause irretrievable data loss or seriously corrupt the database
+        system.  Only superusers can change this setting.
"Inconsiderate" doesn't seem like le mot juste. Maybe "Ill-advised"?

(I'm also wondering whether the sentence about initdb is worth keeping.)

committed with those adjustments

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services