about allow_system_table_mods and SET STATISTICS

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

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