about allow_system_table_mods and SET STATISTICS
Until PostgreSQL 9.1, it was possible to run ALTER TABLE ... SET
STATISTICS without allow_system_table_mods. In PostgreSQL 9.2 and
later, this no longer works. This change was apparently accidental. (I
gave up after a while trying to bisect it exactly, but probably
something related to 1489e2f26a4c0318938b3085f50976512f321d84.)
(It didn't work on mapped relations, so it wasn't all roses.)
A comment in ATPrepSetStatistics() still makes references to this being
possible. I propose to remove this comment.
There was some discussion about (re-)allowing this and some other
commands like this, but after the recent changes to make
allow_system_table_mods easier to use, I think this has less urgency, so
I'd rather get the comment correct in the meantime.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Remove-comment-about-SET-STATISTICS-on-system-tables.patchtext/plain; charset=UTF-8; name=0001-Remove-comment-about-SET-STATISTICS-on-system-tables.patch; x-mac-creator=0; x-mac-type=0Download
From 203571a0402face9feb92a058d8423317501545e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 4 Dec 2019 23:46:00 +0100
Subject: [PATCH] Remove comment about SET STATISTICS on system tables
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. Remove that comment. (Maybe someone wants
to revive that functionality, but not right now.)
---
src/backend/commands/tablecmds.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5440eb9015..55b9b63336 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6707,10 +6707,8 @@ 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.
+ * We do our own permission checking because we want to allow SET
+ * STATISTICS on indexes (for expressional index columns).
*/
if (rel->rd_rel->relkind != RELKIND_RELATION &&
rel->rd_rel->relkind != RELKIND_MATVIEW &&
--
2.24.0
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Until PostgreSQL 9.1, it was possible to run ALTER TABLE ... SET
STATISTICS without allow_system_table_mods. In PostgreSQL 9.2 and
later, this no longer works. This change was apparently accidental. (I
gave up after a while trying to bisect it exactly, but probably
something related to 1489e2f26a4c0318938b3085f50976512f321d84.)
(It didn't work on mapped relations, so it wasn't all roses.)
A comment in ATPrepSetStatistics() still makes references to this being
possible. I propose to remove this comment.
There was some discussion about (re-)allowing this and some other
commands like this, but after the recent changes to make
allow_system_table_mods easier to use, I think this has less urgency, so
I'd rather get the comment correct in the meantime.
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).
regards, tom lane
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
Attachments:
v2-0001-Remove-ATPrepSetStatistics.patchtext/plain; charset=UTF-8; name=v2-0001-Remove-ATPrepSetStatistics.patch; x-mac-creator=0; x-mac-type=0Download
From ca4b1eefade67af55fb16827fca9b1c3c7b1c942 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
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.)
Yeah --- that way, the behavior would also be conveniently available
to other ALTER TABLE subcommands.
This patch looks good, with one trivial nitpick: it looks a bit odd
to insert the relkind check into ATExecSetStatistics between the
assignment of "newtarget" and the validity check for same. I'd
put it either before or after that whole stanza. Just a cosmetic
thing though.
regards, tom lane
On 2019-12-10 17:23, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
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.)Yeah --- that way, the behavior would also be conveniently available
to other ALTER TABLE subcommands.This patch looks good, with one trivial nitpick: it looks a bit odd
to insert the relkind check into ATExecSetStatistics between the
assignment of "newtarget" and the validity check for same. I'd
put it either before or after that whole stanza. Just a cosmetic
thing though.
Committed that way. Thanks.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services