Обсуждение: 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

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