On 9/19/22 4:52 PM, Jonathan S. Katz wrote:
On 9/19/22 11:16 AM, Alvaro Herrera wrote:
This seems a pretty arbitrary restriction. It feels like you're adding this restriction precisely so that you don't have to write the code to reject the ALTER .. SET SCHEMA if an incompatible configuration is detected. But we already have such checks in several cases, so I don't see why this one does not seem a good idea. The whole FOR ALL TABLES IN SCHEMA thing seems pretty weird in several aspects. Others have already commented about the syntax, which is unlike what GRANT uses; I'm also surprised that we've gotten away with it being superuser-only. Why are we building more superuser-only features in this day and age? I think not even FOR ALL TABLES should require superuser.FYI, I've added this to the PG15 open items as there are some open questions to resolve in this thread.
(Replying personally, not RMT).I wanted to enumerate the concerns raised in this thread in the context of the open item to understand what needs to be addressed, and also give an opinion. I did read up on the original thread to better understand context around decisions.
I believe the concerns are these 3 things:1. Allowing calls that have "ALL TABLES IN SCHEMA" that include calls to specific tables in schema 2. The syntax of the "ALL TABLES IN SCHEMA" and comparing it to similar behaviors in PostgreSQL
3. Adding on an additional "superuser-only" featureFor #1 (allowing calls that have schema/table overlap...), there appears to be both a patch that allows this (reversing[8]), and a suggestion for dealing with a corner-case that is reasonable, i.e. disallowing adding schemas to a publication when specifying column-lists. Do we think we can have consensus on this prior to the RC1 freeze?
For #2 ("ALL TABLES IN SCHEMA" syntax), this was heavily discussed on the original thread[1][3][4][5][7]. I thought Tom's proposal on the syntax[3] was reasonable as it "future proofs" for when we allow other schema-scoped objects to be published and give control over which ones can be published.
The bigger issue seems to be around the behavior in regards to the syntax. The current behavior is that when one specifies "ALL TABLES IN SCHEMA", any future tables created in that schema are added to the publication. While folks tried to find parallels to GRANT[6], I think this actually resembles how we handle partitions that are published[9][10], i.e.:
"When a partitioned table is added to a publication, all of its existing and future partitions are implicitly considered to be part of the publication."[10]
Additionally, this is the behavior that is already present in "FOR ALL TABLES":
"Marks the publication as one that replicates changes for all tables in the database, including tables created in the future."[10]
I don't think we should change this behavior that's already in logical replication. While I understand the reasons why "GRANT ... ALL TABLES IN SCHEMA" has a different behavior (i.e. it's not applied to future objects) and do not advocate to change it, I have personally been affected where I thought a permission would be applied to all future objects, only to discover otherwise. I believe it's more intuitive to think that "ALL" applies to "everything, always."
For #3 (more superuser-only), in general I do agree that we shouldn't be adding more of these. However, we have in this release, and not just to this feature. ALTER SUBSCRIPTION ... SKIP[11] requires superuser. I think it's easier for us to "relax" privileges (e.g. w/predefined roles) than to make something "superuser-only" in the future, so I don't see this being a blocker for v15. The feature will continue to work for users even if we remove "superuser-only" in the future.
To summarize:1. I do think we should fix the issue that Peter originally brought up in this thread before v15. That is an open item. 2. I don't see why we need to change the syntax/behavior, I think that will make this feature much harder to use. 3. I don't think we need to change the superuser requirement right now, but we should do that for a future release.
Thanks, Jonathan[1] https://www.postgresql.org/message-id/CAFiTN-u_m0cq7Rm5Bcu9EW4gSHG94WaLuxLfibwE-o7%2BLea2GQ%40mail.gmail.com [2] https://www.postgresql.org/message-id/C4D04B90-AC4D-42A7-B93C-4799CEDDDD96%40enterprisedb.com
[3] https://www.postgresql.org/message-id/155565.1628954580%40sss.pgh.pa.us[4] https://www.postgresql.org/message-id/CAHut%2BPvNwzp-EdtsDNazwrNrV4ziqCovNdLywzOJKSy52LvRjw%40mail.gmail.com [5] https://www.postgresql.org/message-id/CAHut%2BPt6Czj0KsE0ip6nMsPf4FatHgNDni-wSu2KkYNYF9mDAw%40mail.gmail.com [6] https://www.postgresql.org/message-id/CAA4eK1Lwtea0St1MV5nfSg9FrFeU04YKpHvhQ0i4W-tOBw%3D9Qw%40mail.gmail.com [7] https://www.postgresql.org/message-id/202109241325.eag5g6mpvoup@alvherre.pgsql [8] https://www.postgresql.org/message-id/CALDaNm1BEXtvg%3Dfq8FzM-FoYvETTEuvA_Gf8rCAjFr1VrB5aBA%40mail.gmail.com [9] https://www.postgresql.org/message-id/CAJcOf-fyM3075t9%2B%3DB-BSFz2FG%3D5BnDSPX4YtL8k1nnK%3DwjgWA%40mail.gmail.com
[10] https://www.postgresql.org/docs/current/sql-createpublication.html [11] https://www.postgresql.org/docs/15/sql-altersubscription.html
OpenPGP_signature
Description: OpenPGP digital signature
