On Thu, Mar 5, 2026 at 2:23 PM Jeff Davis <[email protected]> wrote: > > On Thu, 2026-03-05 at 09:21 +0530, Amit Kapila wrote: > > We revoke view rights on subconninfo from the public. See below [A] > > in > > system_views.sql. Do we want to do the same for subserver or is it > > okay for users to see it? > > I can't think of a reason that the server name should be secret, but > let me know if you think so. > > > I think the following comment and some place > > in docs needs to be updated. > > [A] > > -- All columns of pg_subscription except subconninfo are publicly > > readable. > > REVOKE ALL ON pg_subscription FROM public; > > GRANT SELECT (oid, subdbid, subskiplsn, subname, subowner, > > Good catch! Thank you. > > > 2. We may want to update the following text in pg_dump docs about the > > new way of connecting to hosts. See [B] (When dumping logical > > replication subscriptions, pg_dump will generate CREATE SUBSCRIPTION > > commands that use the connect = false option, so that restoring the > > subscription does not make remote connections for creating a > > replication slot or for initial table copy. That way, the dump can be > > restored without requiring network access to the remote servers. It > > is > > then up to the user to reactivate the subscriptions in a suitable > > way. > > If the involved hosts have changed, the connection information might > > have to be changed.) > > > > [B] - https://www.postgresql.org/docs/devel/app-pgdump.html > > > > I think the above comment is still correct -- it would be a bit easier > to deal with servers rather than raw connection strings, but the > comment already says "...might have to be changed" which is just a > reminder to look. > > > Attached a new patch that also addressed the review comments from here: > > https://www.postgresql.org/message-id/cad21aocpr8ufmongkz92hz-78cr2h+3tbs9qlveyownwbfx...@mail.gmail.com > > Additionally, I ran into a problem that's worth highlighting: > > DROP SERVER ... CASCADE was broken, because the subscription is > dependent on it but that's in a global catalog, which is not handled by > doDeletion(). The subscription is conceptually a per-database object, > but it's in a shared catalog with a subdbid field. I solved that > problem by adding a guard to findDependentObjects() to check for the > referenced object belonging to a shared catalog, and if so it just > throws an error (so CASCADE is not supported for servers used in > subscriptions). That's a simple but not a very satisfying solution, so > let me know if you see a problem with that.
I shared the awkwardness, but don't have any better ideas. However, it does raise a question as to why do we need an FDW to be database specific or for that matter a SERVER database specific. That might be because it requires an extension which is database specific. Probably we should support extensions which are database agnostic. However that's way beyond the scope of this patch. Other way around why do we need subscriptions to be shared objects? Again probably beyond the scope of this patch. I also see some code duplicated across Create and Alter subscription code paths. Even without this patch the code was duplicated, but with this patch the amount of duplication has increased. Can we deduplicate some of the code? I don't think we need a separate ForeignServerName function. In AlterSubscription() we already have ForeignSever object which has server name in it. Other two callers invoke ForeignServerConnectionString() which in turn fetches ForeignServer object. Those callers instead may fetch ForeignServer object themselves and pass it to ForeignServerConnectionString() and use it in the error message. The patch has changes to pg_dump.c but there is no corresponding test. But I don't think we need a separate test. If the objects created in regress/*.sql tests are not dropped, 002_pg_upgrade.pl would test dump/restore of subscriptions with server. I think we need tests for testing changes in connection when ALTER SUBSCRIPTION ... SERVER is executed and also those for switching between SERVER and CONNECTION. -- Best Wishes, Ashutosh Bapat
