Обсуждение: allow_system_table_mods stuff

After the earlier thread [0] that dealt with ALTER TABLE on system
catalogs, I took a closer look at the allow_system_table_mods setting.
I found a few oddities, and it seems there is some room for improvement.

Attached are some patches to get the discussion rolling: One patch makes
allow_system_table_mods settable at run time by superuser, the second
one is a test suite that documents the current behavior that I gathered
after analyzing the source code, the third one removes some code that
was found useless by the tests.  (The first patch might be useful on its
own, but right now it's just to facilitate the test suite.)

Some observations:

- For the most part, a_s_t_m establishes an additional level of access
control on top of superuserdom for doing DDL on system catalogs.  That
seems like a useful definition.

- But enabling a_s_t_m also allows a non-superuser to do DML on system
catalogs.  That seems like an entirely unrelated and surprising behavior.

- Some checks are redundant with the pinning concept of the dependency
system.  For example, you can't drop a system catalog even with a_s_t_m
on.  That seems useful, of course, but as a result there is a bit of
dead or useless code around.  (The dependency system is newer than a_s_t_m.)

- The source code comments indicate that SET STATISTICS on system
catalogs is supposed to be allowed without a_s_t_m, but it actually
doesn't work.

Proposals and discussion points:

- Having a test suite like this seems useful.

- The behavior that a_s_t_m allows non-superusers to do DML on system
catalogs should be removed.  (Regular permissions can be used for that.)

- Things that are useful in normal use, for example SET STATISTICS, some
or all reloptions, should always be allowed (subject to other access
control).

- There is currently no support in pg_dump to preserve any of those
changes.  Maybe that's not a big problem.

- Dead code or code that is redundant with pinning should be removed.

Any other thoughts?


[0]:
https://www.postgresql.org/message-id/flat/e49f825b-fb25-0bc8-8afc-d5ad895c7975%402ndquadrant.com

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jun 21, 2019 at 5:12 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Attached are some patches to get the discussion rolling: One patch makes
> allow_system_table_mods settable at run time by superuser, the second
> one is a test suite that documents the current behavior that I gathered
> after analyzing the source code, the third one removes some code that
> was found useless by the tests.  (The first patch might be useful on its
> own, but right now it's just to facilitate the test suite.)

Sounds generally sensible (but I didn't read the code).  I
particularly like the first idea.

> Any other thoughts?

I kinda feel like we should prohibit DML on system catalogs, even by
superusers, unless you press the big red button that says "I am
definitely sure that I know what I'm doing." Linking that with
allow_system_table_mods is some way seems natural, but I'm not totally
sure it's the right thing to do.  I guess we could have
alter_table_system_mods={no,yes,yesyesyes}, the former allowing DML
and not-too-scary things and the latter allowing anything at all.

A related issue is that alter_system_table_mods prohibits both stuff
that's probably not going to cause any big problem and stuff that is
almost guaranteed to make the system permanently unusable - e.g. you
could 'SET STORAGE' on a system catalog column, which is really pretty
innocuous, or you could change the oid column of pg_database to a
varlena type, which is guaranteed to destroy the universe.  Here
again, maybe some operations should be more protected than others, or
maybe the relatively safe things just shouldn't be subject to
allow_system_table_mods at all.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jun 21, 2019 at 5:12 AM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
> > Any other thoughts?
>
> I kinda feel like we should prohibit DML on system catalogs, even by
> superusers, unless you press the big red button that says "I am
> definitely sure that I know what I'm doing." Linking that with
> allow_system_table_mods is some way seems natural, but I'm not totally
> sure it's the right thing to do.  I guess we could have
> alter_table_system_mods={no,yes,yesyesyes}, the former allowing DML
> and not-too-scary things and the latter allowing anything at all.

I agree that we should be strongly discouraging even superusers from
doing DML or DDL on system catalogs, and making them jump through hoops
to make it happen at all.

> A related issue is that alter_system_table_mods prohibits both stuff
> that's probably not going to cause any big problem and stuff that is
> almost guaranteed to make the system permanently unusable - e.g. you
> could 'SET STORAGE' on a system catalog column, which is really pretty
> innocuous, or you could change the oid column of pg_database to a
> varlena type, which is guaranteed to destroy the universe.  Here
> again, maybe some operations should be more protected than others, or
> maybe the relatively safe things just shouldn't be subject to
> allow_system_table_mods at all.

If there are things which are through proper grammar (ALTER TABLE or
such) and which will actually usefully work when done against a system
catalog table (eg: GRANT), then I'm all for just allowing that, provided
the regular security checks are done.  I don't think we should ever be
allowed DML though, or any DDL which we know will break the system,
without making them go through hoops.  Personally, I'd rather disallow
all DDL on system catalogs and then explicitly add support for specific
DDL when someone complains and has done a sufficient review to show that
allowing that DDL is a good thing and will actually work as intended.

Thanks,

Stephen

Robert Haas <robertmhaas@gmail.com> writes:
> I kinda feel like we should prohibit DML on system catalogs, even by
> superusers, unless you press the big red button that says "I am
> definitely sure that I know what I'm doing."

Keep in mind that DML-on-system-catalogs is unfortunately a really
standard hack in extension upgrade scripts.  (If memory serves,
some of our contrib scripts do that, and we've certainly told third
parties that it's the only way out of some box or other.)  I don't
think we can just shut it off.  What you seem to be proposing is to
allow it only after

SET allow_system_table_mods = on;

which would be all right except that an extension script containing
such a command will fail outright in existing releases.  I think we
need to be friendlier than that to extension authors who are, for the
most part, trying to work around some deficiency of ours not theirs.

I'm not saying that DML-off-by-default is a bad goal to work toward;
I'm just saying "mind the collateral damage".

> A related issue is that alter_system_table_mods prohibits both stuff
> that's probably not going to cause any big problem and stuff that is
> almost guaranteed to make the system permanently unusable - e.g. you
> could 'SET STORAGE' on a system catalog column, which is really pretty
> innocuous, or you could change the oid column of pg_database to a
> varlena type, which is guaranteed to destroy the universe.  Here
> again, maybe some operations should be more protected than others, or
> maybe the relatively safe things just shouldn't be subject to
> allow_system_table_mods at all.

Meh.  It doesn't really seem to me that distinguishing these cases
is a productive use of code space or maintenance effort.  Superusers
are assumed to know what they're doing, and most especially so if
they've hit the big red button.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > I kinda feel like we should prohibit DML on system catalogs, even by
> > superusers, unless you press the big red button that says "I am
> > definitely sure that I know what I'm doing."
>
> Keep in mind that DML-on-system-catalogs is unfortunately a really
> standard hack in extension upgrade scripts.  (If memory serves,
> some of our contrib scripts do that, and we've certainly told third
> parties that it's the only way out of some box or other.)  I don't
> think we can just shut it off.  What you seem to be proposing is to
> allow it only after
>
> SET allow_system_table_mods = on;

That's basically what my feeling is, yes.

> which would be all right except that an extension script containing
> such a command will fail outright in existing releases.  I think we
> need to be friendlier than that to extension authors who are, for the
> most part, trying to work around some deficiency of ours not theirs.

As with other cases where someone needs to do DML against the catalog
for some reason or another- we should fix that.  If there's example
cases, great!  Let's look at those and come up with a proper solution.

Other options include- letting an extension set that GUC (seems likely
that any case where this is needed is a case where the extension is
installing C functions and therefore is being run by a superuser
anyway...), or implicitly setting that GUC when we're running an
extension's script (urrggghhhh...  I don't care for that one bit, but I
like it better than letting any superuser who wishes UPDATE random bits
in the catalog).

> I'm not saying that DML-off-by-default is a bad goal to work toward;
> I'm just saying "mind the collateral damage".

Sure, makes sense.

> > A related issue is that alter_system_table_mods prohibits both stuff
> > that's probably not going to cause any big problem and stuff that is
> > almost guaranteed to make the system permanently unusable - e.g. you
> > could 'SET STORAGE' on a system catalog column, which is really pretty
> > innocuous, or you could change the oid column of pg_database to a
> > varlena type, which is guaranteed to destroy the universe.  Here
> > again, maybe some operations should be more protected than others, or
> > maybe the relatively safe things just shouldn't be subject to
> > allow_system_table_mods at all.
>
> Meh.  It doesn't really seem to me that distinguishing these cases
> is a productive use of code space or maintenance effort.  Superusers
> are assumed to know what they're doing, and most especially so if
> they've hit the big red button.

The direction I took the above was that we should actually be thinking
about if there are acceptable cases to be running DDL against the
catalog and, if so, specifically allow those.  I'm not convinced at the
moment that any such exist and therefore I'd rather have it denied
(unless you push the big red button) and then tell people to show us
their use case and then we can decide if it's an 'ok' thing to allow, or
what.

I'd really like to stop the cases like stackoverflow articles that
describe how to "remove" an enum value by simply modifying the catalog,
or at least make them have to add a "well, push this big red button
first that the PG people tell you never to push, and then.." to the
start.

Thanks,

Stephen

On 2019-06-21 11:12:38 +0200, Peter Eisentraut wrote:
> After the earlier thread [0] that dealt with ALTER TABLE on system
> catalogs, I took a closer look at the allow_system_table_mods setting.
> I found a few oddities, and it seems there is some room for improvement.

I complained about this recently again, and unfortunately the reaction
wasn't that welcoming:
https://postgr.es/m/20190509145054.byiwa255xvdbfh3a%40alap3.anarazel.de

> Attached are some patches to get the discussion rolling: One patch makes
> allow_system_table_mods settable at run time by superuser

+1 - this seems to have agreement.


> - For the most part, a_s_t_m establishes an additional level of access
> control on top of superuserdom for doing DDL on system catalogs.  That
> seems like a useful definition.
>
> - But enabling a_s_t_m also allows a non-superuser to do DML on system
> catalogs.  That seems like an entirely unrelated and surprising behavior.

Indeed.


> - Some checks are redundant with the pinning concept of the dependency
> system.  For example, you can't drop a system catalog even with a_s_t_m
> on.  That seems useful, of course, but as a result there is a bit of
> dead or useless code around.  (The dependency system is newer than a_s_t_m.)

I'm not fond of deduplicating things around this. This seems like a
separate layers of defense to me.


> - Having a test suite like this seems useful.

+1


> - The behavior that a_s_t_m allows non-superusers to do DML on system
> catalogs should be removed.  (Regular permissions can be used for that.)

+1


> - Dead code or code that is redundant with pinning should be removed.

-1


> Any other thoughts?

* a_s_t_m=off should forbid modifying catalog tables, even for
  superusers.



Greetings,

Andres Freund

Hi,

On 2019-06-21 12:28:43 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > I kinda feel like we should prohibit DML on system catalogs, even by
> > superusers, unless you press the big red button that says "I am
> > definitely sure that I know what I'm doing."
> 
> Keep in mind that DML-on-system-catalogs is unfortunately a really
> standard hack in extension upgrade scripts.  (If memory serves,
> some of our contrib scripts do that, and we've certainly told third
> parties that it's the only way out of some box or other.)  I don't
> think we can just shut it off.  What you seem to be proposing is to
> allow it only after
> 
> SET allow_system_table_mods = on;
> 
> which would be all right except that an extension script containing
> such a command will fail outright in existing releases.  I think we
> need to be friendlier than that to extension authors who are, for the
> most part, trying to work around some deficiency of ours not theirs.

I'm not quite convinced we need to go very far with compatibility here -
pretty much by definition scripts that do this are tied a lot more to
the internals than ones using DDL. But if we want to, we could just -
for now at least - set allow_system_table_mods to a new 'warn' - when
processing extension scripts as superusers.


> > A related issue is that alter_system_table_mods prohibits both stuff
> > that's probably not going to cause any big problem and stuff that is
> > almost guaranteed to make the system permanently unusable - e.g. you
> > could 'SET STORAGE' on a system catalog column, which is really pretty
> > innocuous, or you could change the oid column of pg_database to a
> > varlena type, which is guaranteed to destroy the universe.  Here
> > again, maybe some operations should be more protected than others, or
> > maybe the relatively safe things just shouldn't be subject to
> > allow_system_table_mods at all.
> 
> Meh.  It doesn't really seem to me that distinguishing these cases
> is a productive use of code space or maintenance effort.  Superusers
> are assumed to know what they're doing, and most especially so if
> they've hit the big red button.

I really don't buy this. You need superuser for nearly all CREATE
EXTENSION invocations, and for a lot of other routine tasks. Making the
non-routine crazy stuff slightly harder is worthwhile. I don't think we
can really separate those two into fully separate roles unfortunately,
because the routine CREATE EXTENSION stuff obviously can be used to
elevate privs.

Greetings,

Andres Freund

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2019-06-21 12:28:43 -0400, Tom Lane wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> > > A related issue is that alter_system_table_mods prohibits both stuff
> > > that's probably not going to cause any big problem and stuff that is
> > > almost guaranteed to make the system permanently unusable - e.g. you
> > > could 'SET STORAGE' on a system catalog column, which is really pretty
> > > innocuous, or you could change the oid column of pg_database to a
> > > varlena type, which is guaranteed to destroy the universe.  Here
> > > again, maybe some operations should be more protected than others, or
> > > maybe the relatively safe things just shouldn't be subject to
> > > allow_system_table_mods at all.
> >
> > Meh.  It doesn't really seem to me that distinguishing these cases
> > is a productive use of code space or maintenance effort.  Superusers
> > are assumed to know what they're doing, and most especially so if
> > they've hit the big red button.
>
> I really don't buy this. You need superuser for nearly all CREATE
> EXTENSION invocations, and for a lot of other routine tasks. Making the
> non-routine crazy stuff slightly harder is worthwhile. I don't think we
> can really separate those two into fully separate roles unfortunately,
> because the routine CREATE EXTENSION stuff obviously can be used to
> elevate privs.

I'm not sure what you're intending to respond to here, but I don't think
it's what was being discussed.

The question went something like this- if we decide that setting the
default for relpages made sense in some use-case, should a superuser
have to hit the big red button to do:

ALTER TABLE pg_class SET DEFAULT relpages = 100;

or should we just allow it?

Tom's opinion, if I followed it correctly, was 'no, that is rare enough
that it just is not worth the extra code to allow that without the big
red button, but deny everything else.'  My opinion was 'if they bring us
a legitimate use-case for such, then, sure, maybe we allow specficially
that without hitting the big red button.'  Robert was suggesting that we
could have a tri-state for a_s_t_m, where you could hit the big red
button only half-way, and certain things would then be allowed.

At least, I think that's about how it went.  None of it was about doing
typical CREATE EXTENSION and similar routine tasks that don't involve
running ALTER TABLE or DML against catalog tables.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Keep in mind that DML-on-system-catalogs is unfortunately a really
>> standard hack in extension upgrade scripts.  (If memory serves,
>> some of our contrib scripts do that, and we've certainly told third
>> parties that it's the only way out of some box or other.)

> As with other cases where someone needs to do DML against the catalog
> for some reason or another- we should fix that.  If there's example
> cases, great!  Let's look at those and come up with a proper solution.

As I said, we've got examples.  I traced the existing UPDATEs-on-catalogs
in contrib scripts back to their origin commits, and found these:


commit a89b4b1be0d3550a7860250ff74dc69730555a1f
    Update citext extension for parallel query.

    This could have been done cleaner if we had ALTER AGGREGATE variants
    that would let us add an aggcombine function after the fact and mark
    the aggregate PARALLEL SAFE.  Seems like a reasonable feature, but
    it still doesn't exist today, three years later.

commit 94be9e3f0ca9e7ced66168397eb586565bced9ca
    Fix citext's upgrade-from-unpackaged script to set its collation correctly.
commit 9b97b7f8356c63ea0b6704718d75ea01ec3035bf
    Fix citext upgrade script to update derived copies of pg_type.typcollation.

    The difficulty here was lack of ALTER TYPE to change a type's
    collation.  We'd have to rethink the denormalized storage of
    typcollation in a lot of other places if we wanted to support that,
    but possibly it'd be worth it.

commit 749a787c5b25ae33b3d4da0ef12aa05214aa73c7
    Handle contrib's GIN/GIST support function signature changes honestly.

    This needed to change the declared argument types of some support
    functions, without having their OIDs change.  No, I *don't* think
    it'd be a good idea to provide a DDL command to do that.

commit de1d042f5979bc1388e9a6d52a4d445342b04932
    Support index-only scans in contrib/cube and contrib/seg GiST indexes.

    "The only exciting part of this is that ALTER OPERATOR FAMILY lacks
    a way to drop a support function that was declared as being part of
    an opclass rather than being loose in the family.  For the moment,
    we'll hack our way to a solution with a manual update of the pg_depend
    entry type, which is what distinguishes the two cases.  Perhaps
    someday it'll be worth providing a cleaner way to do that, but for
    now it seems like a very niche problem."

commit 0024e348989254d48dc4afe9beab98a6994a791e
    Fix upgrade of contrib/intarray and contrib/unaccent from 9.0.
commit 4eb49db7ae634fab9af7437b2e7b6388dfd83bd3
    Fix contrib/pg_trgm to have smoother updates from 9.0.

    More cases where we had to change the proargtypes of a pg_proc
    entry without letting its OID change.

commit 472f608e436a41865b795c999bda3369725fa097
    One more hack to make contrib upgrades from 9.0 match fresh 9.1 installs.

    Lack of a way to replace a support function in an existing opclass.
    It's possible this could be done better, but on the other hand, it'd
    be *really* hard to have an ALTER OPCLASS feature for that that would
    be even a little bit concurrency-safe.


So there's certainly some fraction of these cases where we could have
avoided doing manual catalog updates by expending work on some ALTER
command instead.  But I don't see much reason to think that we could,
or should try to, insist that every such case be done that way.  The
cost/benefit ratio is not there in some cases, and in others, exposing
a DDL command to do it would just be providing easier access to
something that's fundamentally unsafe anyway.

The change-proargtypes example actually brings up a larger point:
exactly how is, say, screwing with the contents of the pg_class
row for a system catalog any safer than doing "DDL" on the catalog?
I don't think we should fool ourselves that the one thing is
inherently safer than the other.

In none of these cases are we ever going to be able to say "that's
generically safe", or at least if we try, we're going to find that
distinguishing safe cases from unsafe requires unreasonable amounts
of effort.  I don't think it's a productive thing to spend time on.
I don't mind having two separate "allow_system_table_ddl" and
"allow_system_table_dml" flags, because it's easy to tell what each
of those is supposed to enforce.  But I'm going to run away screaming
from any proposal to invent "allow_safe_system_table_dml".  It's a
recipe for infinite security bugs and it's just not worth it.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> So there's certainly some fraction of these cases where we could have
> avoided doing manual catalog updates by expending work on some ALTER
> command instead.  But I don't see much reason to think that we could,
> or should try to, insist that every such case be done that way.  The
> cost/benefit ratio is not there in some cases, and in others, exposing
> a DDL command to do it would just be providing easier access to
> something that's fundamentally unsafe anyway.

In the cases where we can do better by providing a DDL command, it's
certainly my opinion that we should go that route.  I don't think we
should allow something that's fundamentally unsafe in that way- for
those cases though, how is the extension script making it 'safe'?  If it
simply is hoping, well, that smells like a bug, and we probably should
try to avoid having that in our extensions as folks do like to copy
them.

When it comes to cases that fundamentally are one-off's and that we
don't think really deserve a proper DDL command, then I'd say we make
the extensions set the flag.  At least then it's clear "hey, we had to
do something really grotty here, maybe don't copy this into your new
extension, or don't use this method."  We should also un-set the flag
after.

> The change-proargtypes example actually brings up a larger point:
> exactly how is, say, screwing with the contents of the pg_class
> row for a system catalog any safer than doing "DDL" on the catalog?
> I don't think we should fool ourselves that the one thing is
> inherently safer than the other.

I don't believe one to be safer than the other...

> In none of these cases are we ever going to be able to say "that's
> generically safe", or at least if we try, we're going to find that
> distinguishing safe cases from unsafe requires unreasonable amounts
> of effort.  I don't think it's a productive thing to spend time on.
> I don't mind having two separate "allow_system_table_ddl" and
> "allow_system_table_dml" flags, because it's easy to tell what each
> of those is supposed to enforce.

Which implies that it doesn't make sense to have two different flags
for it.

> But I'm going to run away screaming
> from any proposal to invent "allow_safe_system_table_dml".  It's a
> recipe for infinite security bugs and it's just not worth it.

Yeah, I'm not really a fan of that either.

Thanks,

Stephen

On 6/21/19 3:07 PM, Stephen Frost wrote:
> When it comes to cases that fundamentally are one-off's and that we
> don't think really deserve a proper DDL command, then I'd say we make
> the extensions set the flag.  At least then it's clear "hey, we had to
> do something really grotty here, maybe don't copy this into your new
> extension, or don't use this method."  We should also un-set the flag
> after.

I'd be leery of collateral damage from that to extension update scripts
in extension releases currently in the wild.

Maybe there should be a new extension control file setting

needs_system_table_mods = (boolean)

which means what it says if it's there, but if an ALTER EXTENSION
UPDATE sees a control file that lacks the setting, assume true
(with a warning?).

Regards,
-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> I'd be leery of collateral damage from that to extension update scripts
> in extension releases currently in the wild.

Yeah, that's my primary concern here.

> Maybe there should be a new extension control file setting
> needs_system_table_mods = (boolean)
> which means what it says if it's there, but if an ALTER EXTENSION
> UPDATE sees a control file that lacks the setting, assume true
> (with a warning?).

I think that having a SET command in the update script is the way to go;
for one thing it simplifies testing the script by just sourcing it,
and for another it defaults to no-special-privileges which is the
right default.  Also, non-backward-compatible control files aren't
any nicer than non-backward-compatible script files.

We do have to get past the compatibility issue though.  My thought was
that for a period of N years we could force allow_system_table_dml = on
while running extension scripts, and then cease doing so.  This would
give extension authors a reasonable window in which their scripts would
work in either old or new backends.  At some point in that time they'd
probably have occasion to make other changes that render their scripts
not backwards compatible, at which point they can insert "SET
allow_system_table_dml = on" so that the script keeps working when we
remove the compatibility hack.

(Of course, we have an awful track record about actually doing things
N years after we said we would, but doesn't anybody around here have
a calendar app?)

This line of thought leads to the conclusion that we do want
separate "allow_system_table_dml" and "allow_system_table_ddl"
bools.  Otherwise, the backwards-compatibility hack would need
to turn on a level of unsafety that extension scripts have *not*
had before and surely shouldn't have by default.

            regards, tom lane

On 6/21/19 4:37 PM, Tom Lane wrote:
> We do have to get past the compatibility issue though.  My thought was
> that for a period of N years we could force allow_system_table_dml = on
> while running extension scripts, and then cease doing so.  This would
> give extension authors a reasonable window in which their scripts would
> work in either old or new backends.  At some point in that time they'd
> probably have occasion to make other changes that render their scripts
> not backwards compatible, at which point they can insert "SET
> allow_system_table_dml = on" so that the script keeps working when we
> remove the compatibility hack.

I was having second thoughts too, like maybe to tweak ALTER EXTENSION
UPDATE to unconditionally force the flag on before the update script and
reset it after, but warn if it is actually still set at the reset-after
point.

Extension maintainers could then make the warning go away by releasing
versions where the update scripts contain an explicit RESET (at the very
top, if they do nothing fancy), or a(n initially redundant) SET at the
top and RESET at the bottom. No new control file syntax.

Regards,
-Chap

Hi,

On 2019-06-21 16:37:16 -0400, Tom Lane wrote:
> We do have to get past the compatibility issue though.  My thought was
> that for a period of N years we could force allow_system_table_dml = on
> while running extension scripts, and then cease doing so.  This would
> give extension authors a reasonable window in which their scripts would
> work in either old or new backends.  At some point in that time they'd
> probably have occasion to make other changes that render their scripts
> not backwards compatible, at which point they can insert "SET
> allow_system_table_dml = on" so that the script keeps working when we
> remove the compatibility hack.

I'd modify this approach by having a allow_system_table_dml level that
warns when DDL to system tables is performed, and then set
allow_system_table_dml to that when processing extension scripts (and
perhaps modify the warning message when creating_extension ==
true). That way it'd be easier to spot such extension scripts.

And I'd personally probably just set allow_system_table_dml to warn when
working interactively, just to improve logging etc.


> This line of thought leads to the conclusion that we do want
> separate "allow_system_table_dml" and "allow_system_table_ddl"
> bools.  Otherwise, the backwards-compatibility hack would need
> to turn on a level of unsafety that extension scripts have *not*
> had before and surely shouldn't have by default.

+1

Greetings,

Andres Freund

On Fri, Jun 21, 2019 at 4:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This line of thought leads to the conclusion that we do want
> separate "allow_system_table_dml" and "allow_system_table_ddl"
> bools.  Otherwise, the backwards-compatibility hack would need
> to turn on a level of unsafety that extension scripts have *not*
> had before and surely shouldn't have by default.

Right, exactly.

I'm repeating myself, but I still think it's super-useful to
distinguish things which are "for expert use only" from things which
are "totally bonkers." You can argue that if you're an expert, you
should know enough to avoid the totally bonkers things, but PostgreSQL
is pretty popular these days [citation needed] and there are a lot of
people administering databases who know what they are doing to a
pretty reasonable degree but don't have anywhere near the level of
understanding of someone who spends their days hacking core.  Putting
up some kind of a stop sign that lets you know when you're about to go
from adventurous to lethal will help those people.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jun 21, 2019 at 4:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This line of thought leads to the conclusion that we do want
>> separate "allow_system_table_dml" and "allow_system_table_ddl"
>> bools.  Otherwise, the backwards-compatibility hack would need
>> to turn on a level of unsafety that extension scripts have *not*
>> had before and surely shouldn't have by default.

> Right, exactly.

> I'm repeating myself, but I still think it's super-useful to
> distinguish things which are "for expert use only" from things which
> are "totally bonkers."

Agreed, although "DML vs DDL" is a pretty poor approximation of that
boundary.  As shown in examples upthread, you can find reasonable things
to do and totally-catastrophic things to do in both categories.

The position I'm maintaining is that it's not worth our trouble to try to
mechanically distinguish which things are which.  Once you've broken the
glass and flipped either the big red switch or the slightly smaller orange
switch, it's entirely on you to not screw up your database beyond
recovery.

I do see value in two switches not one, but it's what I said above,
to not need to give people *more* chance-to-break-things than they
had before when doing manual catalog fixes.  That is, we need a
setting that corresponds more or less to current default behavior.

There's an aesthetic argument to be had about whether to have two
bools or one three-way switch, but I prefer the former; there's
no backward-compatibility issue here since allow_system_table_mods
couldn't be set by applications anyway.

            regards, tom lane

On Mon, Jun 24, 2019 at 11:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I'm repeating myself, but I still think it's super-useful to
> > distinguish things which are "for expert use only" from things which
> > are "totally bonkers."
>
> Agreed, although "DML vs DDL" is a pretty poor approximation of that
> boundary.  As shown in examples upthread, you can find reasonable things
> to do and totally-catastrophic things to do in both categories.

I agree.  I would like it if there were a way to do better, but I'm
not sure that there is, at least for a reasonable level of effort.

> There's an aesthetic argument to be had about whether to have two
> bools or one three-way switch, but I prefer the former; there's
> no backward-compatibility issue here since allow_system_table_mods
> couldn't be set by applications anyway.

I'm happy to defer on that point.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Mon, Jun 24, 2019 at 11:20:51AM -0400, Tom Lane wrote:
> I do see value in two switches not one, but it's what I said above,
> to not need to give people *more* chance-to-break-things than they
> had before when doing manual catalog fixes.  That is, we need a
> setting that corresponds more or less to current default behavior.
> 
> There's an aesthetic argument to be had about whether to have two
> bools or one three-way switch, but I prefer the former; there's
> no backward-compatibility issue here since allow_system_table_mods
> couldn't be set by applications anyway.

I like a single three-way switch since if you are allowing DDL, you
probably don't care if you restrict DML.  log_statement already has a
similar distinction with values of none, ddl, mod, all.  I assume
allow_system_table_mods could have value of false, dml, true.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Sun, Jul  7, 2019 at 11:45:49PM -0400, Bruce Momjian wrote:
> On Mon, Jun 24, 2019 at 11:20:51AM -0400, Tom Lane wrote:
> > I do see value in two switches not one, but it's what I said above,
> > to not need to give people *more* chance-to-break-things than they
> > had before when doing manual catalog fixes.  That is, we need a
> > setting that corresponds more or less to current default behavior.
> > 
> > There's an aesthetic argument to be had about whether to have two
> > bools or one three-way switch, but I prefer the former; there's
> > no backward-compatibility issue here since allow_system_table_mods
> > couldn't be set by applications anyway.
> 
> I like a single three-way switch since if you are allowing DDL, you
> probably don't care if you restrict DML.  log_statement already has a
> similar distinction with values of none, ddl, mod, all.  I assume
> allow_system_table_mods could have value of false, dml, true.

Or, to match log_statement, use:  none, dml, all.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On 2019-Jun-28, Peter Eisentraut wrote:

> 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.

I think this patch is at a point where it merits closer review from
fellow committers, so I marked it RfC for now.  I hope non-committers
would also look at it some more, though.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Sep 13, 2019 at 06:39:40PM -0300, Alvaro Herrera wrote:
> I think this patch is at a point where it merits closer review from
> fellow committers, so I marked it RfC for now.  I hope non-committers
> would also look at it some more, though.

I guess so.  The patch has conflicts in the serial and parallel
schedules, so I have moved it to next CF, waiting on author for a
rebase.

Peter, are you planning to look at that again?  Note: the patch has no
reviewers registered.
--
Michael

On 2019-11-27 09:26, Michael Paquier wrote:
> On Fri, Sep 13, 2019 at 06:39:40PM -0300, Alvaro Herrera wrote:
>> I think this patch is at a point where it merits closer review from
>> fellow committers, so I marked it RfC for now.  I hope non-committers
>> would also look at it some more, though.
> 
> I guess so.  The patch has conflicts in the serial and parallel
> schedules, so I have moved it to next CF, waiting on author for a
> rebase.
> 
> Peter, are you planning to look at that again?  Note: the patch has no
> reviewers registered.

Here is an updated patch series.

After re-reading the discussion again, I have kept the existing name of 
the option.  I have also moved the tests to the "unsafe_tests" suite, 
which seems like a better place.  And I have split the patch into three.

Other than those cosmetic changes, I think everything here has been 
discussed and agreed to, so unless anyone expresses any concern or a 
wish to do more review, I think this is ready to commit.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-11-27 09:26, Michael Paquier wrote:
>> Peter, are you planning to look at that again?  Note: the patch has no
>> reviewers registered.

> Here is an updated patch series.

> After re-reading the discussion again, I have kept the existing name of 
> the option.  I have also moved the tests to the "unsafe_tests" suite, 
> which seems like a better place.  And I have split the patch into three.

Personally I'd have gone with the renaming to allow_system_table_ddl,
but it's not a huge point.  Updating the code to agree with that
naming would make the patch much more invasive, so maybe it's not
worth it.

> Other than those cosmetic changes, I think everything here has been 
> discussed and agreed to, so unless anyone expresses any concern or a 
> wish to do more review, I think this is ready to commit.

I read through the patch set and have just one quibble: in the
proposed new docs,

+        Allows modification of the structure of system tables as well as
+        certain other risky actions 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.

"Inconsiderate" doesn't seem like le mot juste.  Maybe "Ill-advised"?

(I'm also wondering whether the sentence about initdb is worth keeping.)

I marked the CF entry RFC.

            regards, tom lane

On 2019-11-28 17:11, Tom Lane wrote:
> I read through the patch set and have just one quibble: in the
> proposed new docs,
> 
> +        Allows modification of the structure of system tables as well as
> +        certain other risky actions 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.
> 
> "Inconsiderate" doesn't seem like le mot juste.  Maybe "Ill-advised"?
> 
> (I'm also wondering whether the sentence about initdb is worth keeping.)

committed with those adjustments

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services