On 2019-12-05 00:16, Tom Lane wrote:
Seems reasonable. The argument for making this an exception to
allow_system_table_mods was always more about expediency than logical
cleanliness. After the recent changes I think it's okay to remove the
special case (especially since nobody has griped about it being broken).
However ... if we're not going to have that special case, couldn't
we get rid of the whole business of having a special permissions test?
What is it that ATSimplePermissions can't handle here? The business
about requiring a colName certainly doesn't need to be done before the
ownership check (in fact, it could be left to execution, so we don't need
a prep function at all anymore).
Good point. Done in the attached patch.
(If someone wanted to revive the original functionality, it would
nowadays probably be easier to add a flag ATT_SYSTEM_TABLE to
ATSimplePermissions(), so there is really no reason to keep the old
function separate.)
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From ca4b1eefade67af55fb16827fca9b1c3c7b1c942 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 10 Dec 2019 13:36:11 +0100
Subject: [PATCH v2] Remove ATPrepSetStatistics
It was once possible to do ALTER TABLE ... SET STATISTICS on system
tables without allow_sytem_table_mods. This was changed apparently by
accident between PostgreSQL 9.1 and 9.2, but a code comment still
claimed this was possible. Without that functionality, having a
separate ATPrepSetStatistics() is useless, so use the generic
ATSimplePermissions() instead and move the remaining custom code into
ATExecSetStatistics().
---
src/backend/commands/tablecmds.c | 57 ++++++++------------------------
1 file changed, 14 insertions(+), 43 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5440eb9015..fdc02d9f7f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -386,8 +386,6 @@ static ObjectAddress ATExecAddIdentity(Relation rel, const
char *colName,
static ObjectAddress ATExecSetIdentity(Relation rel, const char *colName,
Node
*def, LOCKMODE lockmode);
static ObjectAddress ATExecDropIdentity(Relation rel, const char *colName,
bool missing_ok, LOCKMODE lockmode);
-static void ATPrepSetStatistics(Relation rel, const char *colName, int16
colNum,
- Node *newValue,
LOCKMODE lockmode);
static ObjectAddress ATExecSetStatistics(Relation rel, const char *colName,
int16 colNum,
Node *newValue, LOCKMODE lockmode);
static ObjectAddress ATExecSetOptions(Relation rel, const char *colName,
@@ -3948,9 +3946,9 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
pass = AT_PASS_COL_ATTRS;
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
+ ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW |
ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
- /* Performs own permission checks */
- ATPrepSetStatistics(rel, cmd->name, cmd->num, cmd->def,
lockmode);
+ /* No command-specific prep needed */
pass = AT_PASS_MISC;
break;
case AT_SetOptions: /* ALTER COLUMN SET ( options )
*/
@@ -6702,45 +6700,7 @@ ATExecDropIdentity(Relation rel, const char *colName,
bool missing_ok, LOCKMODE
/*
* ALTER TABLE ALTER COLUMN SET STATISTICS
- */
-static void
-ATPrepSetStatistics(Relation rel, const char *colName, int16 colNum, Node
*newValue, LOCKMODE lockmode)
-{
- /*
- * 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.
- */
- if (rel->rd_rel->relkind != RELKIND_RELATION &&
- rel->rd_rel->relkind != RELKIND_MATVIEW &&
- rel->rd_rel->relkind != RELKIND_INDEX &&
- rel->rd_rel->relkind != RELKIND_PARTITIONED_INDEX &&
- rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
- rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table, materialized
view, index, or foreign table",
- RelationGetRelationName(rel))));
-
- /*
- * We allow referencing columns by numbers only for indexes, since table
- * column numbers could contain gaps if columns are later dropped.
- */
- if (rel->rd_rel->relkind != RELKIND_INDEX &&
- rel->rd_rel->relkind != RELKIND_PARTITIONED_INDEX &&
- !colName)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot refer to non-index column by
number")));
-
- /* Permissions checks */
- if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER,
get_relkind_objtype(rel->rd_rel->relkind),
- RelationGetRelationName(rel));
-}
-
-/*
+ *
* Return value is the address of the modified column
*/
static ObjectAddress
@@ -6756,6 +6716,17 @@ ATExecSetStatistics(Relation rel, const char *colName,
int16 colNum, Node *newVa
Assert(IsA(newValue, Integer));
newtarget = intVal(newValue);
+ /*
+ * We allow referencing columns by numbers only for indexes, since table
+ * column numbers could contain gaps if columns are later dropped.
+ */
+ if (rel->rd_rel->relkind != RELKIND_INDEX &&
+ rel->rd_rel->relkind != RELKIND_PARTITIONED_INDEX &&
+ !colName)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot refer to non-index column by
number")));
+
/*
* Limit target to a sane range
*/
--
2.24.0
