Re: allow_system_table_mods stuff

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)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