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.
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]:
https://www.postgresql.org/message-id/flat/e49f825b-fb25-0bc8-8afc-d5ad895c7975%402ndquadrant.com
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From d9143b1ca4d00ac90b1cd8cd6eefba86cb4685b6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
From 97515b52944b663c068a8dee763e41855739fd56 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
From e0008c8e1504af1e8c5de25c2991423adb8ea30f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
