Re: [19] CREATE SUBSCRIPTION ... SERVER

On Thu, Mar 5, 2026 at 2:23 PM Jeff Davis <pgsql@j-davis.com> 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+3Tbs9QLveYoWnWBfxrxw@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