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
From b44c7c7b7177a87e777b3b2a7fe10ea739bfaa72 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@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
