Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Document new catalog columns, missed in commit 8185bb5347.

  2. Refactor to remove ForeignServerName().

  3. GetSubscription(): use per-object memory context.

  4. Fix dependency on FDW's connection function.

  5. ALTER SUBSCRIPTION ... SERVER test.

  6. Fix pg_dump for CREATE FOREIGN DATA WRAPPER ... CONNECTION.

  7. Clean up postgres_fdw/t/010_subscription.pl.

  8. CREATE SUBSCRIPTION ... SERVER.

  9. Allow upgrades to preserve the full subscription's state.

  1. [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-08-30T06:42:00Z

    Synopsis:

  Publisher:

    CREATE TABLE x(i INT);
    CREATE TABLE y(i INT);
    INSERT INTO x VALUES(1);
    INSERT INTO y VALUES(-1);
    CREATE PUBLICATION pub1 FOR TABLE x;
    CREATE PUBLICATION pub2 FOR TABLE y;

  Subscriber:

    CREATE SERVER myserver FOR CONNECTION ONLY OPTIONS (
      host '...', dbname '...'
    );
    CREATE USER MAPPING FOR PUBLIC SERVER myserver OPTIONS (
      user '...', password '...'
    );

    CREATE TABLE x(i INT);
    CREATE TABLE y(i INT);
    CREATE SUBSCRIPTION sub1 SERVER myserver PUBLICATION pub1;
    CREATE SUBSCRIPTION sub2 SERVER myserver PUBLICATION pub2;

Motivation:

  * Allow managing connections separately from managing the
    subscriptions themselves. For instance, if you update an
    authentication method or the location of the publisher, updating
    the server alone will update all subscriptions at once.
  * Enable separating the privileges to create a subscription from the
    privileges to create a connection string. (By default
    pg_create_subscription has both privileges for compatibility with
    v16, but the connection privilege can be revoked from
    pg_create_subscription, see below.)
  * Enable changing of single connection parameters without pasting
    the rest of the connection string as well. E.g. "ALTER SERVER
    ... OPTIONS (SET ... '...');".
  * Benefit from user mappings and ACLs on foreign server object if
    you have multiple roles creating subscriptions.

Details:

The attached patch implements "CREATE SUBSCRIPTION ... SERVER myserver"
as an alternative to "CREATE SUBSCRIPTION ... CONNECTION '...'". The
user must be a member of pg_create_subscription and have USAGE
privileges on the server.

The server "myserver" must have been created with the new syntax:

   CREATE SERVER myserver FOR CONNECTION ONLY

instead of specifying FOREIGN DATA WRAPPER. In other words, a server
FOR CONNECTION ONLY doesn't have a real FDW, it's a special server just
used for the postgres connection options. To create a server FOR
CONNECTION ONLY, the user must be a member of the new predefined role
pg_create_connection. A server FOR CONNECTION ONLY still uses ACLs and
user mappings the same way as other foreign servers, but cannot be used
to create foreign tables.

The predefined role pg_create_subscription is also a member of the role
pg_create_connection, so that existing members of the
pg_create_subscription role may continue to create subscriptions using
CONNECTION just like in v16 without any additional grant.

Security:

One motivation of this patch is to enable separating the privileges to
create a subscription from the privileges to create a connection
string, because each have their own security implications and may be
done through separate processes. To separate the privileges, simply
revoke pg_create_connection from pg_create_subscription; then you can
grant each one independently as you see fit.

For instance, there may be an administrator that controls what
postgres instances are available, and what connections may be
reasonable between those instances. That admin will need the
pg_create_connection role, and can proactively create all the servers
(using FOR CONNECTION ONLY) and user mappings that may be useful, and
manage and update those as necessary without breaking
subscriptions. Another role may be used to manage the subscriptions
themselves, and they would need to be a member of
pg_create_subscription but do not need the privileges to create raw
connection strings.

Note: the ability to revoke pg_create_connection from
pg_create_subscription avoids some risks in some environments; but
creating a subcription should still be considered a highly privileged
operation whether using SERVER or CONNECTION.

Remaining work:

The code for options handling needs some work. It's similar to
postgres_fdw in behavior, but I didn't spend as much time on it because
I suspect we will want to refactor the various ways connection strings
are handled (in CREATE SUBSCRIPTION ... CONNECTION, postgres_fdw, and
dblink) to make them more consistent.

Also, there are some nuances in handling connection options that I
don't fully understand. postgres_fdw makes a lot of effort: it
overrides client_encoding, it does a
post-connection security check, and allows GSS instead of a password
option for non-superusers. But CREATE SUBSCRIPTION ... CONNECTION makes
little effort, only checking whether the password is specified or not.
I'd like to understand why they are different and what we can unify.

Also, right now dblink has it's own dblink_fdw, and perhaps a server
FOR CONNECTION ONLY should become the preferred method instead.


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

  2. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2023-08-30T13:41:59Z

    Hi Jeff,

On Wed, Aug 30, 2023 at 2:12 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> The server "myserver" must have been created with the new syntax:
>
>    CREATE SERVER myserver FOR CONNECTION ONLY
>
> instead of specifying FOREIGN DATA WRAPPER. In other words, a server
> FOR CONNECTION ONLY doesn't have a real FDW, it's a special server just
> used for the postgres connection options. To create a server FOR
> CONNECTION ONLY, the user must be a member of the new predefined role
> pg_create_connection. A server FOR CONNECTION ONLY still uses ACLs and
> user mappings the same way as other foreign servers, but cannot be used
> to create foreign tables.

Are you suggesting that SERVERs created with FDW can not be used as
publishers? I think there's value in knowing that the publisher which
contains a replica of a table is the same as the foreign server which
is referenced by another foreign table. We can push down a join
between a replicated table and foreign table down to the foreign
server. A basic need for sharding with replicated tables. Of course
there's a lot work that we have to do in order to actually achieve
such a push down but by restricting this feature to only CONNECTION
ONLY, we are restricting the possibility of such a push down.

-- 
Best Wishes,
Ashutosh Bapat

  3. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-08-30T13:49:45Z

    Jeff Davis <pgsql@j-davis.com> writes:
> The server "myserver" must have been created with the new syntax:
>    CREATE SERVER myserver FOR CONNECTION ONLY
> instead of specifying FOREIGN DATA WRAPPER. In other words, a server
> FOR CONNECTION ONLY doesn't have a real FDW, it's a special server just
> used for the postgres connection options.

This seems like it requires a whole lot of new mechanism (parser
and catalog infrastructure) that could be done far more easily
in other ways.  In particular, how about inventing a built-in
dummy FDW to serve the purpose?  That could have some use for
other testing as well.

			regards, tom lane

  4. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-08-30T15:30:45Z

    On Wed, 2023-08-30 at 19:11 +0530, Ashutosh Bapat wrote:
> Are you suggesting that SERVERs created with FDW can not be used as
> publishers?

Correct. Without that, how would the subscription know that the FDW
contains valid postgres connection information? I suppose it could
create a connection string out of the options itself and do another
round of validation, is that what you had in mind?

> We can push down a join
> between a replicated table and foreign table down to the foreign
> server.

Interesting idea.

Regards,
	Jeff Davis

  5. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-08-30T16:09:43Z

    On Wed, 2023-08-30 at 09:49 -0400, Tom Lane wrote:
> This seems like it requires a whole lot of new mechanism (parser
> and catalog infrastructure) that could be done far more easily
> in other ways.  In particular, how about inventing a built-in
> dummy FDW to serve the purpose?

That was my initial approach, but it was getting a bit messy.

FDWs don't have a schema, so we can't put it in pg_catalog, and names
beginning with "pg_" aren't restricted now. Should I retroactively
restrict FDW names that begin with "pg_"? Or just use special cases in
pg_dump and elsewhere? Also I didn't see a great place to document it.

Admittedly, I didn't complete the dummy-FDW approach, so perhaps it
works out better overall. I can give it a try.

Regards,
	Jeff Davis

  6. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2023-08-31T05:29:03Z

    On Wed, Aug 30, 2023 at 9:00 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Wed, 2023-08-30 at 19:11 +0530, Ashutosh Bapat wrote:
> > Are you suggesting that SERVERs created with FDW can not be used as
> > publishers?
>
> Correct. Without that, how would the subscription know that the FDW
> contains valid postgres connection information? I suppose it could
> create a connection string out of the options itself and do another
> round of validation, is that what you had in mind?

The server's FDW has to be postgres_fdw. So we have to handle the
awkward dependency between core and postgres_fdw (an extension). The
connection string should be created from options itself. A special
user mapping for replication may be used. That's how I see it at a
high level.

-- 
Best Wishes,
Ashutosh Bapat

  7. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Robert Haas <robertmhaas@gmail.com> — 2023-08-31T12:37:49Z

    On Wed, Aug 30, 2023 at 1:19 PM Jeff Davis <pgsql@j-davis.com> wrote:
> On Wed, 2023-08-30 at 09:49 -0400, Tom Lane wrote:
> > This seems like it requires a whole lot of new mechanism (parser
> > and catalog infrastructure) that could be done far more easily
> > in other ways.  In particular, how about inventing a built-in
> > dummy FDW to serve the purpose?
>
> That was my initial approach, but it was getting a bit messy.
>
> FDWs don't have a schema, so we can't put it in pg_catalog, and names
> beginning with "pg_" aren't restricted now. Should I retroactively
> restrict FDW names that begin with "pg_"? Or just use special cases in
> pg_dump and elsewhere? Also I didn't see a great place to document it.
>
> Admittedly, I didn't complete the dummy-FDW approach, so perhaps it
> works out better overall. I can give it a try.

What I feel is kind of weird about this syntax is that it seems like
it's entangled with the FDW mechanism but doesn't really overlap with
it. You could have something that is completely separate (CREATE
SUBSCRIPTION CONNECTION) or something that truly does have some
overlap (no new syntax and a dummy fdw, as Tom proposes, or somehow
knowing that postgres_fdw is special, as Ashutosh proposes). But this
seems like sort of an odd middle ground.

I also think that the decision to make pg_create_connection a member
of pg_create_subscription by default, but encouraging users to think
about revoking it, is kind of strange. I don't think we really want to
encourage users to tinker with predefined roles in this kind of way. I
think there are better ways of achieving the goals here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  8. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-08-31T16:50:45Z

    On Wed, 2023-08-30 at 09:09 -0700, Jeff Davis wrote:
> Admittedly, I didn't complete the dummy-FDW approach, so perhaps it
> works out better overall. I can give it a try.

We need to hide the dummy FDW from pg_dump. And we need to hide it from
psql's \dew, because that's used in tests and prints the owner's name,
and the bootstrap superuser doesn't have a consistent name. But I
didn't find a good way to hide it because it doesn't have a schema.

The best I could come up with is special-casing by the name, but that
seems like a pretty bad hack. For other built-in objects, psql is
willing to print them out if you just specify something like "\dT
pg_catalog.*", but that wouldn't work here. We could maybe do something
based on the "pg_" prefix, but we'd have to retroactively restrict FDWs
with that prefix, which sounds like a bad idea.

Suggestions?

Regards,
	Jeff Davis

  9. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-08-31T16:52:59Z

    On Thu, 2023-08-31 at 10:59 +0530, Ashutosh Bapat wrote:
> The server's FDW has to be postgres_fdw. So we have to handle the
> awkward dependency between core and postgres_fdw (an extension).

That sounds more than just "awkward". I can't think of any precedent
for that and it seems to violate the idea of an "extension" entirely.

Can you explain more concretely how we might resolve that?

Regards,
	Jeff Davis

  10. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-08-31T17:28:42Z

    On Thu, 2023-08-31 at 08:37 -0400, Robert Haas wrote:
> What I feel is kind of weird about this syntax is that it seems like
> it's entangled with the FDW mechanism but doesn't really overlap with
> it.

I like the fact that it works with user mappings and benefits from the
other thinking that's gone into that system. I would call that a
"feature" not an "entanglement".

>  You could have something that is completely separate (CREATE
> SUBSCRIPTION CONNECTION)

I thought about that but it would be a new object type with a new
catalog and I didn't really see an upside. It would open up questions
about permissions, raw string vs individual options, whether we need
user mappings or not, etc., and those have all been worked out already
with foreign servers.

>  or something that truly does have some
> overlap (no new syntax and a dummy fdw, as Tom proposes, or somehow
> knowing that postgres_fdw is special, as Ashutosh proposes).

I ran into a (perhaps very minor?) challenge[1] with the dummy FDW:

https://www.postgresql.org/message-id/c47e8ba923bf0a13671f7d8230a81d465c21fb04.camel@j-davis.com

suggestions welcome there, of course.

Regarding core code depending on postgres_fdw: how would that work?
Would that be acceptable?

>  But this
> seems like sort of an odd middle ground.

I assume here that you're talking about the CREATE SERVER ... FOR
CONNECTION ONLY syntax. I don't think it's odd. We have lots of objects
that are a lot like another object but treated differently for various
reasons. A foreign table is an obvious example.

> I also think that the decision to make pg_create_connection a member
> of pg_create_subscription by default, but encouraging users to think
> about revoking it, is kind of strange. I don't think we really want
> to
> encourage users to tinker with predefined roles in this kind of way.
> I
> think there are better ways of achieving the goals here.

Such as?

Regards,
	Jeff Davis

  11. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Joe Conway <mail@joeconway.com> — 2023-08-31T21:17:29Z

    On 8/31/23 12:52, Jeff Davis wrote:
> On Thu, 2023-08-31 at 10:59 +0530, Ashutosh Bapat wrote:
>> The server's FDW has to be postgres_fdw. So we have to handle the
>> awkward dependency between core and postgres_fdw (an extension).
> 
> That sounds more than just "awkward". I can't think of any precedent
> for that and it seems to violate the idea of an "extension" entirely.
> 
> Can you explain more concretely how we might resolve that?


Maybe move postgres_fdw to be a first class built in feature instead of 
an extension?

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  12. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2023-09-01T06:58:43Z

    On Fri, Sep 1, 2023 at 2:47 AM Joe Conway <mail@joeconway.com> wrote:
>
> On 8/31/23 12:52, Jeff Davis wrote:
> > On Thu, 2023-08-31 at 10:59 +0530, Ashutosh Bapat wrote:
> >> The server's FDW has to be postgres_fdw. So we have to handle the
> >> awkward dependency between core and postgres_fdw (an extension).
> >
> > That sounds more than just "awkward". I can't think of any precedent
> > for that and it seems to violate the idea of an "extension" entirely.
> >
> > Can you explain more concretely how we might resolve that?
>
>
> Maybe move postgres_fdw to be a first class built in feature instead of
> an extension?

Yes, that's one way.

Thinking larger, how about we allow any FDW to be used here. We might
as well, allow extensions to start logical receivers which accept
changes from non-PostgreSQL databases. So we don't have to make an
exception for postgres_fdw. But I think there's some value in bringing
together these two subsystems which deal with foreign data logically
(as in logical vs physical view of data).

-- 
Best Wishes,
Ashutosh Bapat

  13. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-09-01T18:54:44Z

    On Fri, 2023-09-01 at 12:28 +0530, Ashutosh Bapat wrote:
> Thinking larger, how about we allow any FDW to be used here.

That's a possibility, but I think that means the subscription would
need to constantly re-check the parameters rather than relying on the
FDW's validator.

Otherwise it might be the wrong kind of FDW, and the user might be able
to circumvent the password_required protection. It might not even be a
postgres-related FDW at all, which would be a bit strange.

If it's constantly re-checking the parameters then it raises the
possibility that some "ALTER SERVER" or "ALTER USER MAPPING" succeeds
but then subscriptions to that foreign server start failing, which
would not be ideal. But I could be fine with that.

> But I think there's some value in bringing
> together these two subsystems which deal with foreign data logically
> (as in logical vs physical view of data).

I still don't understand how a core dependency on an extension would
work.

Regards,
	Jeff Davis

  14. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-09-01T18:57:01Z

    On Thu, 2023-08-31 at 17:17 -0400, Joe Conway wrote:
> Maybe move postgres_fdw to be a first class built in feature instead
> of 
> an extension?

That could make sense, but we still have to solve the problem of how to
present a built-in FDW.

FDWs don't have a schema, so it can't be inside pg_catalog. So we'd
need some special logic somewhere to make pg_dump and psql \dew work as
expected, and I'm not quite sure what to do there.

Regards,
	Jeff Davis

  15. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Robert Haas <robertmhaas@gmail.com> — 2023-09-01T20:11:30Z

    On Fri, Sep 1, 2023 at 4:04 PM Jeff Davis <pgsql@j-davis.com> wrote:
> On Thu, 2023-08-31 at 17:17 -0400, Joe Conway wrote:
> > Maybe move postgres_fdw to be a first class built in feature instead
> > of
> > an extension?
>
> That could make sense, but we still have to solve the problem of how to
> present a built-in FDW.
>
> FDWs don't have a schema, so it can't be inside pg_catalog. So we'd
> need some special logic somewhere to make pg_dump and psql \dew work as
> expected, and I'm not quite sure what to do there.

I'm worried that an approach based on postgres_fdw would have security
problems. I think that we don't want postgres_fdw installed in every
PostgreSQL cluster for security reasons. And I think that the set of
people who should be permitted to manage connection strings for
logical replication subscriptions could be different from the set of
people who are entitled to use postgres_fdw.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  16. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2023-09-04T12:31:57Z

    On Sat, Sep 2, 2023 at 12:24 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2023-09-01 at 12:28 +0530, Ashutosh Bapat wrote:
> > Thinking larger, how about we allow any FDW to be used here.
>
> That's a possibility, but I think that means the subscription would
> need to constantly re-check the parameters rather than relying on the
> FDW's validator.
>
> Otherwise it might be the wrong kind of FDW, and the user might be able
> to circumvent the password_required protection. It might not even be a
> postgres-related FDW at all, which would be a bit strange.
>
> If it's constantly re-checking the parameters then it raises the
> possibility that some "ALTER SERVER" or "ALTER USER MAPPING" succeeds
> but then subscriptions to that foreign server start failing, which
> would not be ideal. But I could be fine with that.

Why do we need to re-check parameters constantly? We will need to
restart subscriptions which are using the user mapping of FDW when
user mapping or server options change. If that mechanism isn't there,
we will need to build it. But that's doable.

I didn't understand your worry about circumventing password_required protection.

>
> > But I think there's some value in bringing
> > together these two subsystems which deal with foreign data logically
> > (as in logical vs physical view of data).
>
> I still don't understand how a core dependency on an extension would
> work.

We don't need to if we allow any FDW (even if non-postgreSQL) to be
specified there. For non-postgresql FDW the receiver will need to
construct the appropriate command and use appropriate protocol to get
the changes and apply locally. The  server at the other end may not
even have logical replication capability. The extension or "input
plugin" (as against output plugin) would decide whether it can deal
with the foreign server specific logical replication protocol. We add
another callback to FDW which can return whether the given foreign
server supports logical replication or not. If the users
misconfigured, their subscriptions will throw errors.

But with this change we open a built-in way to "replicate in" as we
have today to "replicate out".

-- 
Best Wishes,
Ashutosh Bapat

  17. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2023-09-04T12:34:28Z

    On Sat, Sep 2, 2023 at 1:41 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Sep 1, 2023 at 4:04 PM Jeff Davis <pgsql@j-davis.com> wrote:
> > On Thu, 2023-08-31 at 17:17 -0400, Joe Conway wrote:
> > > Maybe move postgres_fdw to be a first class built in feature instead
> > > of
> > > an extension?
> >
> > That could make sense, but we still have to solve the problem of how to
> > present a built-in FDW.
> >
> > FDWs don't have a schema, so it can't be inside pg_catalog. So we'd
> > need some special logic somewhere to make pg_dump and psql \dew work as
> > expected, and I'm not quite sure what to do there.
>
> I'm worried that an approach based on postgres_fdw would have security
> problems. I think that we don't want postgres_fdw installed in every
> PostgreSQL cluster for security reasons. And I think that the set of
> people who should be permitted to manage connection strings for
> logical replication subscriptions could be different from the set of
> people who are entitled to use postgres_fdw.

If postgres_fdw was the only way to specify a connection to be used
with subscriptions, what you are saying makes sense. But it's not. We
will continue to support current mechanism which doesn't require
postgres_fdw to be installed on every PostgreSQL cluster.

What security problems do you foresee if postgres_fdw is used in
addition to the current mechanism?

-- 
Best Wishes,
Ashutosh Bapat

  18. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-09-05T19:08:52Z

    On Mon, 2023-09-04 at 18:01 +0530, Ashutosh Bapat wrote:
> Why do we need to re-check parameters constantly? We will need to
> restart subscriptions which are using the user mapping of FDW when
> user mapping or server options change.

"Constantly" was an exaggeration, but the point is that it's a separate
validation step after the ALTER SERVER or ALTER USER MAPPING has
already happened, so the subscription would start failing.

Perhaps this is OK, but it's not the ideal user experience. Ideally,
the user would get some indication from the ALTER SERVER or ALTER USER
MAPPING that it's about to break a subscription that depends on it.

> I didn't understand your worry about circumventing password_required
> protection.

If the subscription doesn't do its own validation, and if the FDW
doesn't ensure that the password is set, then it could end up creating
a creating a connection string without supplying the password.

> We don't need to if we allow any FDW (even if non-postgreSQL) to be
> specified there.

OK, so we could have a built-in FDW called pg_connection that would do
the right kinds of validation; and then also allow other FDWs but the
subscription would have to do its own validation.

Regards,
	Jeff Davis

  19. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-09-12T22:55:52Z

    On Tue, 2023-09-05 at 12:08 -0700, Jeff Davis wrote:
> OK, so we could have a built-in FDW called pg_connection that would
> do
> the right kinds of validation; and then also allow other FDWs but the
> subscription would have to do its own validation.

While working on this, I found a minor bug and there's another
discussion happening here:

https://www.postgresql.org/message-id/e5892973ae2a80a1a3e0266806640dae3c428100.camel%40j-davis.com

It looks like that's going in the direction of checking for the
presence of a password in the connection string at connection time.

Ashutosh, that's compatible with your suggestion that CREATE
SUBSCRIPTION ... SERVER works for any FDW that supplies the right
information, because we need to validate it at connection time anyway.
I'll wait to see how that discussion gets resolved, and then I'll post
the next version.

Regards,
	Jeff Davis

  20. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-12-29T23:22:23Z

    On Tue, 2023-09-05 at 12:08 -0700, Jeff Davis wrote:
> OK, so we could have a built-in FDW called pg_connection that would
> do
> the right kinds of validation; and then also allow other FDWs but the
> subscription would have to do its own validation.

Attached a rough rebased version implementing the above with a
pg_connection_fdw foreign data wrapper built in.

Regards,
	Jeff Davis

  21. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2023-12-31T18:59:23Z

    On Fri, 2023-12-29 at 15:22 -0800, Jeff Davis wrote:
> On Tue, 2023-09-05 at 12:08 -0700, Jeff Davis wrote:
> > OK, so we could have a built-in FDW called pg_connection that would
> > do
> > the right kinds of validation; and then also allow other FDWs but
> > the
> > subscription would have to do its own validation.
> 
> Attached a rough rebased version.

Attached a slightly better version which fixes a pg_dump issue and
improves the documentation.

Regards,
	Jeff Davis

  22. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-01-02T09:44:36Z

    On Mon, Jan 1, 2024 at 12:29 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2023-12-29 at 15:22 -0800, Jeff Davis wrote:
> > On Tue, 2023-09-05 at 12:08 -0700, Jeff Davis wrote:
> > > OK, so we could have a built-in FDW called pg_connection that would
> > > do
> > > the right kinds of validation; and then also allow other FDWs but
> > > the
> > > subscription would have to do its own validation.
> >
> > Attached a rough rebased version.
>
> Attached a slightly better version which fixes a pg_dump issue and
> improves the documentation.

Hi,  I spent some time today reviewing the v4 patch and below are my
comments. BTW, the patch needs a rebase due to commit 9a17be1e2.

1.
+        /*
+         * We don't want to allow unprivileged users to be able to trigger
+         * attempts to access arbitrary network destinations, so
require the user
+         * to have been specifically authorized to create connections.
+         */
+        if (!has_privs_of_role(owner, ROLE_PG_CREATE_CONNECTION))

Can the pg_create_connection predefined role related code be put into
a separate 0001 patch? I think this can go in a separate commit.

2. Can one use {FDW, user_mapping, foreign_server} combo other than
the built-in pg_connection_fdw? If yes, why to allow say oracle_fdw
foreign server and user mapping with logical replication? Isn't this a
security concern?

3. I'd like to understand how the permission model works with this
feature amidst various users a) subscription owner b) table owner c)
FDW owner d) user mapping owner e) foreign server owner f) superuser
g) user with which logical replication bg workers (table sync,
{parallel} apply workers) are started up and running.
What if foreign server owner doesn't have permissions on the table
being applied by logical replication bg workers?
What if foreign server owner is changed with ALTER SERVER ... OWNER TO
when logical replication is in-progress?
What if the owner of  {FDW, user_mapping, foreign_server} is different
from a subscription owner with USAGE privilege granted? Can the
subscription still use the foreign server?

4. How does the invalidation of {FDW, user_mapping, foreign_server}
affect associated subscription and vice-versa?

5. What if the password is changed in user mapping with ALTER USER
MAPPING? Will it refresh the subscription so that all the logical
replication workers get restarted with new connection info?

6. How does this feature fit if a subscription is created with
run_as_owner? Will it check if the table owner has permissions to use
{FDW, user_mapping, foreign_server} comob?

7.
+            if (strcmp(d->defname, "user") == 0 ||
+                strcmp(d->defname, "password") == 0 ||
+                strcmp(d->defname, "sslpassword") == 0 ||
+                strcmp(d->defname, "password_required") == 0)
+                ereport(ERROR,
+                        (errmsg("invalid option \"%s\" for pg_connection_fdw",

+                ereport(ERROR,
+                        (errmsg("invalid user mapping option \"%s\"
for pg_connection_fdw",
+                                d->defname)));

Can we emit an informative error message and hint using
initClosestMatch, updateClosestMatch, getClosestMatch similar to other
FDWs elsewhere in the code?

8.
+                     errmsg("password is required"),
+                     errdetail("Non-superusers must provide a
password in the connection string.")));

The error message and detail look generic, can it be improved to
include something about pg_connection_fdw?

9.
+{ oid => '6015', oid_symbol => 'PG_CONNECTION_FDW',
+  descr => 'Pseudo FDW for connections to Postgres',
+  fdwname => 'pg_connection_fdw', fdwowner => 'POSTGRES',

What if the database cluster is initialized with an owner different
than 'POSTGRES' at the time of initdb? Will the fdwowner be correct in
that case?

10.
+# src/include/catalog/pg_foreign_data_wrapper.dat
+{ oid => '6015', oid_symbol => 'PG_CONNECTION_FDW',

Do we want to REVOKE USAGE ON FOREIGN DATA WRAPPER pg_connection_fdw
FROM PUBLIC and REVOKE EXECUTE ON its handler functions? With this,
the permissions are granted explicitly to the foreign server/user
mapping creators.

11. How about splitting patches in the following manner for better
manageability (all of which can go as separate commits) of this
feature?
0001 for pg_create_connection predefined role per comment #1.
0002 for introducing in-built FDW pg_connection_fdw.
0003 utilizing in-built FDW for logical replication to provide CREATE
SUBSCRIPTION ... SERVER.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  23. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-01-05T00:56:11Z

    On Tue, 2024-01-02 at 15:14 +0530, Bharath Rupireddy wrote:
> Can the pg_create_connection predefined role related code be put into
> a separate 0001 patch? I think this can go in a separate commit.

Done (see below for details).

> 2. Can one use {FDW, user_mapping, foreign_server} combo other than
> the built-in pg_connection_fdw?

Yes, you can use any FDW for which you have USAGE privileges, passes
the validations, and provides enough of the expected fields to form a
connection string.

There was some discussion on this point already. Initially, I
implemented it with more catalog and grammar support, which improved
error checking, but others objected that the grammar wasn't worth it
and that it was too inflexible. See:

https://www.postgresql.org/message-id/172273.1693403385%40sss.pgh.pa.us
https://www.postgresql.org/message-id/CAExHW5unvpDv6yMSmqurHP7Du1PqoJFWVxeK-4YNm5EnoNJiSQ%40mail.gmail.com

>  If yes, why to allow say oracle_fdw
> foreign server and user mapping with logical replication? Isn't this
> a
> security concern?

A user would need USAGE privileges on that other FDW and also must be a
member of pg_create_subscription.

In v16, a user with such privileges would already be able to create
such connection by specifying the raw connection string, so that's not
a new risk with my proposal.

> 3. I'd like to understand how the permission model works with this
> feature amidst various users a) subscription owner b) table owner c)
> FDW owner d) user mapping owner e) foreign server owner f) superuser
> g) user with which logical replication bg workers (table sync,
> {parallel} apply workers) are started up and running.

(a) The subscription owner is only relevant if the subscription is
created with run_as_owner=true, in which case the logical worker
applies the changes with the privileges of the subscription owner. [No
change.]
(b) The table owner is only relevant if the subscription is created
with run_as_owner=false (default), in which case the logical worker
applies the changes with the privileges of the table owner. [No
change.]
(c) The FDW owner is irrelevant, though the creator of a foreign server
must have USAGE privileges on it. [No change.]
(d) User mappings do not have owners. [No change.]
(e) The foreign server owner is irrelevant, but USAGE privileges on the
foreign server are needed to create a subscription to it. [New
behavior.]
(f) Not sure what you mean here, but superusers can do anything. [No
change.]
(g) The actual user the process runs as is still the subscription
owner. If run_as_owner=false, the actions are performed as the table
owner; if run_as_owner=true, the actions are performed as the
subscription owner. [No change.]

There are only two actual changes to the model:

1. Users with USAGE privileges on a foreign server can create
subscriptions using that foreign server instead of a connection string
(they still need to be a member of pg_create_subscription).

2. I created a conceptual separation of privileges between
pg_create_subscription and pg_create_connection; though by default
pg_create_subscription has exactly the same capabilities as before.
There is no behavior change unless the administrator revokes
pg_create_connection from pg_create_subscription.

I'd like to also add the capability for subscriptions to a server to
use a passwordless connection as long as the server is trusted somehow.
The password_required subscription option is already fairly complex, so
we'd need to come up with a sensible way for those options to interact.

> What if foreign server owner doesn't have permissions on the table
> being applied by logical replication bg workers?

The owner of the foreign server is irrelevant -- only the USAGE
privileges on that foreign server matter, and only when it comes to
creating subscriptions.

> What if foreign server owner is changed with ALTER SERVER ... OWNER
> TO
> when logical replication is in-progress?

That should have no effect as long as the USAGE priv is still present.

Note that if the owner of the *subscription* changes, it may find a
different user mapping.

> What if the owner of  {FDW, user_mapping, foreign_server} is
> different
> from a subscription owner with USAGE privilege granted? Can the
> subscription still use the foreign server?

Yes.

> 4. How does the invalidation of {FDW, user_mapping, foreign_server}
> affect associated subscription and vice-versa?

If the user mapping or foreign server change, it causes the apply
worker to re-build the connection string from those objects and restart
if something important changed.

If the FDW changes I don't think that matters.

> 5. What if the password is changed in user mapping with ALTER USER
> MAPPING? Will it refresh the subscription so that all the logical
> replication workers get restarted with new connection info?

Yes. Notice the subscription_change_cb.

That's actually one of the nice features -- if your connection info
changes, update it in one place to affect all subscriptions to that
server.

> 6. How does this feature fit if a subscription is created with
> run_as_owner? Will it check if the table owner has permissions to use
> {FDW, user_mapping, foreign_server} comob?

See above.

> Can we emit an informative error message and hint using
> initClosestMatch, updateClosestMatch, getClosestMatch similar to
> other
> FDWs elsewhere in the code?

Done.

> 8.
> +                     errmsg("password is required"),
> +                     errdetail("Non-superusers must provide a
> password in the connection string.")));
> 
> The error message and detail look generic, can it be improved to
> include something about pg_connection_fdw?

I believe this is addressed after some refactoring -- the FDW itself
doesn't try to validate that a password exists, because we can't rely
on that anyway (someone can use an FDW with no validation or different
validation). Instead, the subscription does this validation.

Note that there is an unrelated hole in the way the subscription does
the validation of password_required, which will be addressed separately
as a part of this other thread:

https://www.postgresql.org/message-id/e5892973ae2a80a1a3e0266806640dae3c428100.camel%40j-davis.com

> 9.
> +{ oid => '6015', oid_symbol => 'PG_CONNECTION_FDW',
> +  descr => 'Pseudo FDW for connections to Postgres',
> +  fdwname => 'pg_connection_fdw', fdwowner => 'POSTGRES',
> 
> What if the database cluster is initialized with an owner different
> than 'POSTGRES' at the time of initdb? Will the fdwowner be correct
> in
> that case?

Thank you, I changed it to use the conventional BKI_DEFAULT(POSTGRES)
instead. (The previous way worked, but was not consistent with existing
patterns.)

> 10.
> +# src/include/catalog/pg_foreign_data_wrapper.dat
> +{ oid => '6015', oid_symbol => 'PG_CONNECTION_FDW',
> 
> Do we want to REVOKE USAGE ON FOREIGN DATA WRAPPER pg_connection_fdw
> FROM PUBLIC

The FDW doesn't have USAGE privileges by default so we don't need to
revoke them.

>  and REVOKE EXECUTE ON its handler functions?

It has no handler function.

I don't see a reason to restrict privileges on
postgresql_fdw_validator(); it seems useful for testing/debugging.

> 11. How about splitting patches in the following manner for better
> manageability (all of which can go as separate commits) of this
> feature?
> 0001 for pg_create_connection predefined role per comment #1.
> 0002 for introducing in-built FDW pg_connection_fdw.
> 0003 utilizing in-built FDW for logical replication to provide CREATE
> SUBSCRIPTION ... SERVER.

Good suggestion, though I split it a bit differently:

0001: fix postgresql_fdw_validator to use libpq options via walrcv
method. This is appropriate for looser validation that doesn't try to
check for password_required or that a password is set -- that's left up
to the subscription.

0002: built-in pg_connection_fdw, also includes code for validation and
transforming into a connection string. This creates a lot of test diffs
in foreign_data.out because I need to exclude the built in FDW (it's
owned by the bootstrap supseruser which is not a stable username). It
would be nice if there was a way to use a negative-matching regex in a
psql \dew+ command -- something like "(?!pg_)*" -- but I couldn't find
a way to do that because "(?...)" seems to not work in psql. Let me
know if you know a trick to do so.

0003: CREATE SUBSCRIPTION... SERVER.

0004: Add pg_create_connection role.

Regards,
	Jeff Davis

  24. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-01-05T07:19:20Z

    On Fri, Jan 5, 2024 at 6:26 AM Jeff Davis <pgsql@j-davis.com> wrote:

>
> > 2. Can one use {FDW, user_mapping, foreign_server} combo other than
> > the built-in pg_connection_fdw?
>
> Yes, you can use any FDW for which you have USAGE privileges, passes
> the validations, and provides enough of the expected fields to form a
> connection string.
>
> There was some discussion on this point already. Initially, I
> implemented it with more catalog and grammar support, which improved
> error checking, but others objected that the grammar wasn't worth it
> and that it was too inflexible. See:
>
> https://www.postgresql.org/message-id/172273.1693403385%40sss.pgh.pa.us
> https://www.postgresql.org/message-id/CAExHW5unvpDv6yMSmqurHP7Du1PqoJFWVxeK-4YNm5EnoNJiSQ%40mail.gmail.com
>

Can you please provide an example using postgres_fdw to create a
subscription using this patch. I think we should document it in
postgres_fdw and add a test for the same.

-- 
Best Wishes,
Ashutosh Bapat

  25. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-01-05T08:04:26Z

    On Fri, 2024-01-05 at 12:49 +0530, Ashutosh Bapat wrote:
> Can you please provide an example using postgres_fdw to create a
> subscription using this patch. I think we should document it in
> postgres_fdw and add a test for the same.

There's a basic test for postgres_fdw in patch 0003, just testing the
syntax and validation.

A manual end-to-end test is pretty straightforward:

  -- on publisher
  create table foo(i int primary key);
  create publication pub1 for table foo;
  insert into foo values(42);

  -- on subscriber
  create extension postgres_fdw;
  create table foo(i int primary key);
  create server server1
    foreign data wrapper postgres_fdw
    options (host '/tmp', port '5432', dbname 'postgres');
  create user mapping for u1 server server1
    options (user 'u1');
  select pg_conninfo_from_server('server1','u1',true);
  create subscription sub1 server server1 publication pub1;

I don't think we need to add an end-to-end test for each FDW, because
it's just using the assembled connection string. To see if it's
assembling the connection string properly, we can unit test with
pg_conninfo_from_server().

Regards,
	Jeff Davis

  26. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-01-05T10:41:59Z

    On Fri, Jan 5, 2024 at 1:34 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2024-01-05 at 12:49 +0530, Ashutosh Bapat wrote:
> > Can you please provide an example using postgres_fdw to create a
> > subscription using this patch. I think we should document it in
> > postgres_fdw and add a test for the same.
>
> There's a basic test for postgres_fdw in patch 0003, just testing the
> syntax and validation.
>
> A manual end-to-end test is pretty straightforward:
>
>   -- on publisher
>   create table foo(i int primary key);
>   create publication pub1 for table foo;
>   insert into foo values(42);
>
>   -- on subscriber
>   create extension postgres_fdw;
>   create table foo(i int primary key);
>   create server server1
>     foreign data wrapper postgres_fdw
>     options (host '/tmp', port '5432', dbname 'postgres');
>   create user mapping for u1 server server1
>     options (user 'u1');
>   select pg_conninfo_from_server('server1','u1',true);
>   create subscription sub1 server server1 publication pub1;
>
> I don't think we need to add an end-to-end test for each FDW, because
> it's just using the assembled connection string. To see if it's
> assembling the connection string properly, we can unit test with
> pg_conninfo_from_server().

Thanks for the steps.

I don't think we need to add a test for every FDW. E.g. adding a test
in file_fdw would be pointless. But postgres_fdw is special. The test
could further create a foreign table ftab_foo on subscriber
referencing foo on publisher and then compare the data from foo and
ftab_foo to make sure that the replication is happening. This will
serve as a good starting point for replicated tables setup in a
sharded cluster.

-- 
Best Wishes,
Ashutosh Bapat

  27. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-01-13T01:17:26Z

    On Fri, 2024-01-05 at 16:11 +0530, Ashutosh Bapat wrote:
> I don't think we need to add a test for every FDW. E.g. adding a test
> in file_fdw would be pointless. But postgres_fdw is special. The test
> could further create a foreign table ftab_foo on subscriber
> referencing foo on publisher and then compare the data from foo and
> ftab_foo to make sure that the replication is happening. This will
> serve as a good starting point for replicated tables setup in a
> sharded cluster.
> 

Attached updated patch set with added TAP test for postgres_fdw, which
uses a postgres_fdw server as the source for subscription connection
information.

I think 0004 needs a bit more work, so I'm leaving it off for now, but
I'll bring it back in the next patch set.

Regards,
	Jeff Davis

  28. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Joe Conway <mail@joeconway.com> — 2024-01-15T20:53:04Z

    On 1/12/24 20:17, Jeff Davis wrote:
> On Fri, 2024-01-05 at 16:11 +0530, Ashutosh Bapat wrote:
>> I don't think we need to add a test for every FDW. E.g. adding a test
>> in file_fdw would be pointless. But postgres_fdw is special. The test
>> could further create a foreign table ftab_foo on subscriber
>> referencing foo on publisher and then compare the data from foo and
>> ftab_foo to make sure that the replication is happening. This will
>> serve as a good starting point for replicated tables setup in a
>> sharded cluster.
>> 
> 
> Attached updated patch set with added TAP test for postgres_fdw, which
> uses a postgres_fdw server as the source for subscription connection
> information.
> 
> I think 0004 needs a bit more work, so I'm leaving it off for now, but
> I'll bring it back in the next patch set.

I took a quick scan through the patch. The only thing that jumped out at 
me was that it seems like it might make sense to use 
quote_literal_cstr() rather than defining your own appendEscapedValue() 
function?

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  29. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-01-15T21:34:18Z

    On Mon, 2024-01-15 at 15:53 -0500, Joe Conway wrote:
> I took a quick scan through the patch. The only thing that jumped out
> at 
> me was that it seems like it might make sense to use 
> quote_literal_cstr() rather than defining your own
> appendEscapedValue() 
> function?

The rules are slightly different. Libpq expects a connection string to
escape only single-quote and backslash, and the escape character is
always backslash:

https://www.postgresql.org/docs/16/libpq-connect.html#LIBPQ-CONNSTRING-KEYWORD-VALUE

quote_literal_cstr() has more complicated rules. If there's a backslash
anywhere in the string, it uses the E'' form. If it encounters a
backslash it escapes it with backslash, but if it encounters a single-
quote it escapes it with single-quote. See:

https://www.postgresql.org/docs/16/sql-syntax-lexical.html#SQL-SYNTAX-STRINGS
https://www.postgresql.org/docs/16/sql-syntax-lexical.html#SQL-SYNTAX-STRINGS-ESCAPE

I'll include some tests and a better comment for it in the next patch
set.

Regards,
	Jeff Davis

  30. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-01-16T01:55:44Z

    On Fri, 2024-01-12 at 17:17 -0800, Jeff Davis wrote:
> I think 0004 needs a bit more work, so I'm leaving it off for now,
> but
> I'll bring it back in the next patch set.

Here's the next patch set. 0001 - 0003 are mostly the same with some
improved error messages and some code fixes. I am looking to start
committing 0001 - 0003 soon, as they have received some feedback
already and 0004 isn't required for the earlier patches to be useful.

0004 could use more discussion. The purpose is to split the privileges
of pg_create_subscription into two: pg_create_subscription, and
pg_create_connection. By separating the privileges, it's possible to
allow someone to create/manage subscriptions to a predefined set of
foreign servers (on which they have USAGE privileges) without allowing
them to write an arbitrary connection string.

The reasoning behind the separation is that creating a connection
string has different and more nuanced security implications than
creating a subscription (cf. extensive discussion[1] related to the
password_required setting on a subscription).

By default, pg_create_subscription is a member of pg_create_connection,
so there's no change/break of the default behavior. But administrators
who want the privileges to be separated can simply "REVOKE
pg_create_connection FROM pg_create_subscription".

Given that CREATE SUBSCRIPTION ... SERVER works on a server of any FDW,
we would also need to protect against someone making using an
unexpected FDW (with no validation or different validation) to
construct a foreign server with malicious connection settings. To do
so, I added to the grammar "CREATE SERVER ... FOR SUBSCRIPTION" (and a
boolean catalog entry in pg_foreign_server) that can only be set by a
member of pg_create_connection.

There was some resistance[2] to adding more grammar/catalog impact than
necessary, so I'm not sure if others think this is the right approach.
The earlier patches are still worth it without 0004, but I do think the
idea of separating the privileges is useful and it would be nice to
find an agreeable solution to do so. At least with the 0004, the
approach is a bit more direct.

Regards,
	Jeff Davis

[1]
https://www.postgresql.org/message-id/9DFC88D3-1300-4DE8-ACBC-4CEF84399A53%40enterprisedb.com

[2]
https://www.postgresql.org/message-id/172273.1693403385%40sss.pgh.pa.us

  31. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-01-16T03:53:13Z

    On Tue, Jan 16, 2024 at 7:25 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2024-01-12 at 17:17 -0800, Jeff Davis wrote:
> > I think 0004 needs a bit more work, so I'm leaving it off for now,
> > but
> > I'll bring it back in the next patch set.
>
> Here's the next patch set. 0001 - 0003 are mostly the same with some
> improved error messages and some code fixes. I am looking to start
> committing 0001 - 0003 soon, as they have received some feedback
> already and 0004 isn't required for the earlier patches to be useful.

Thanks. Here are some comments on 0001. I'll look at other patches very soon.

1.
+    /* Load the library providing us libpq calls. */
+    load_file("libpqwalreceiver", false);

At first glance, it looks odd that libpqwalreceiver library is being
linked to every backend that uses postgresql_fdw_validator. After a
bit of grokking, this feels/is a better and easiest way to not link
libpq to the main postgresql executable as specified at the beginning
of libpqwalreceiver.c file comments. May be a more descriptive note is
worth here instead of just saying "Load the library providing us libpq
calls."?

2. Why not typedef keyword before the ConnectionOption structure? This
way all the "struct ConnectionOption" can be remvoed, no? I know the
previously there is no typedef, but we can add it now so that the code
looks cleaner.

typedef struct ConnectionOption
{
    const char *optname;
    bool        issecret;        /* is option for a password? */
    bool        isdebug;        /* is option a debug option? */
} ConnectionOption;

FWIW, with the above change and removal of struct before every use of
ConnectionOption, the code compiles cleanly for me.

3.
+static const struct ConnectionOption *
+libpqrcv_conninfo_options(void)

Why is libpqrcv_conninfo_options returning the const ConnectionOption?
Is it that we don't expect callers to modify the result? I think it's
not needed given the fact that PQconndefaults doesn't constify the
return value.

4.
+    /* skip options that must be overridden */
+    if (strcmp(option, "client_encoding") == 0)
+        return false;
+

Options that must be overriden or disallow specifiing
"client_encoding" in the SERVER/USER MAPPING definition (just like the
dblink)?

    /* Disallow "client_encoding" */
    if (strcmp(opt->keyword, "client_encoding") == 0)
        return false;

5.
"By using the correct libpq options, it no longer needs to be
deprecated, and can be used by the upcoming pg_connection_fdw."

Use of postgresql_fdw_validator for pg_connection_fdw seems a bit odd
to me. I don't mind pg_connection_fdw having its own validator
pg_connection_fdw_validator even if it duplicates the code. To avoid
code duplication we can move the guts to an internal function in
foreign.c so that both postgresql_fdw_validator and
pg_connection_fdw_validator can use it. This way the code is cleaner
and we can just leave postgresql_fdw_validator as deprecated.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  32. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-01-18T07:17:01Z

    On Tue, 2024-01-16 at 09:23 +0530, Bharath Rupireddy wrote:
> 1.
>  May be a more descriptive note is
> worth here instead of just saying "Load the library providing us
> libpq calls."?

OK, will be in the next patch set.

> 2. Why not typedef keyword before the ConnectionOption structure?

Agreed. An earlier unpublished iteration had the struct more localized,
but here it makes more sense to be typedef'd.

> 3.
> +static const struct ConnectionOption *
> +libpqrcv_conninfo_options(void)
> 
> Why is libpqrcv_conninfo_options returning the const
> ConnectionOption?

I did that so I could save the result, and each subsequent call would
be free (just returning the same pointer). That also means that the
caller doesn't need to free the result, which would require another
entry point in the API.

> Is it that we don't expect callers to modify the result? I think it's
> not needed given the fact that PQconndefaults doesn't constify the
> return value.

The result of PQconndefaults() can change from call to call when the
defaults change. libpqrcv_conninfo_options() only depends on the
available option names (and dispchar), which should be a static list.

> 4.
> +    /* skip options that must be overridden */
> +    if (strcmp(option, "client_encoding") == 0)
> +        return false;
> +
> 
> Options that must be overriden or disallow specifiing
> "client_encoding" in the SERVER/USER MAPPING definition (just like
> the
> dblink)?

I'm not quite sure of your question, but I'll try to improve the
comment.

> 5.
> "By using the correct libpq options, it no longer needs to be
> deprecated, and can be used by the upcoming pg_connection_fdw."
> 
> Use of postgresql_fdw_validator for pg_connection_fdw seems a bit odd
> to me. I don't mind pg_connection_fdw having its own validator
> pg_connection_fdw_validator even if it duplicates the code. To avoid
> code duplication we can move the guts to an internal function in
> foreign.c so that both postgresql_fdw_validator and
> pg_connection_fdw_validator can use it. This way the code is cleaner
> and we can just leave postgresql_fdw_validator as deprecated.

Will do so in the next patch set.

Thank you for taking a look.

Regards,
	Jeff Davis

  33. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-01-22T13:11:07Z

    Hi Jeff,

On Tue, Jan 16, 2024 at 7:25 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2024-01-12 at 17:17 -0800, Jeff Davis wrote:
> > I think 0004 needs a bit more work, so I'm leaving it off for now,
> > but
> > I'll bring it back in the next patch set.
>
> Here's the next patch set. 0001 - 0003 are mostly the same with some
> improved error messages and some code fixes. I am looking to start
> committing 0001 - 0003 soon, as they have received some feedback
> already and 0004 isn't required for the earlier patches to be useful.
>

I am reviewing the patches. Here are some random comments.

0002 adds a prefix "regress_" to almost every object that is created
in foreign_data.sql. The commit message doesn't say why it's doing so.
But more importantly, the new tests added are lost in all the other
changes. It will be good to have prefix adding changes into its own
patch explaining the reason. The new tests may stay in 0002.
Interestingly the foreign server created in the new tests doesn't have
"regress_" prefix. Why?

Dummy FDW makes me nervous. The way it's written, it may grow into a
full-fledged postgres_fdw and in the process might acquire the same
concerns that postgres_fdw has today. But I will study the patches and
discussion around it more carefully.

I enhanced the postgres_fdw TAP test to use foreign table. Please see
the attached patch. It works as expected. Of course a follow-on work
will require linking the local table and its replica on the publisher
table so that push down will work on replicated tables. But the
concept at least works with your changes. Thanks for that.

I am not sure we need a full-fledged TAP test for testing
subscription. I wouldn't object to it, but TAP tests are heavy. It
should be possible to write the same test as a SQL test by creating
two databases and switching between them. Do you think it's worth
trying that way?

> 0004 could use more discussion. The purpose is to split the privileges
> of pg_create_subscription into two: pg_create_subscription, and
> pg_create_connection. By separating the privileges, it's possible to
> allow someone to create/manage subscriptions to a predefined set of
> foreign servers (on which they have USAGE privileges) without allowing
> them to write an arbitrary connection string.

Haven't studied this patch yet. Will continue reviewing the patches.

--
Best Wishes,
Ashutosh Bapat

  34. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-01-22T19:03:50Z

    On Mon, 2024-01-22 at 18:41 +0530, Ashutosh Bapat wrote:
> 0002 adds a prefix "regress_" to almost every object that is created
> in foreign_data.sql.

psql \dew outputs the owner, which in the case of a built-in FDW is the
bootstrap superuser, which is not a stable name. I used the prefix to
exclude the built-in FDW -- if you have a better suggestion, please let
me know. (Though reading below, we might not even want a built-in FDW.)

> Dummy FDW makes me nervous. The way it's written, it may grow into a
> full-fledged postgres_fdw and in the process might acquire the same
> concerns that postgres_fdw has today. But I will study the patches
> and
> discussion around it more carefully.

I introduced that based on this comment[1].

I also thought it fit with your previous suggestion to make it work
with postgres_fdw, but I suppose it's not required. We could just not
offer the built-in FDW, and expect users to either use postgres_fdw or
create their own dummy FDW.

> I enhanced the postgres_fdw TAP test to use foreign table. Please see
> the attached patch. It works as expected. Of course a follow-on work
> will require linking the local table and its replica on the publisher
> table so that push down will work on replicated tables. But the
> concept at least works with your changes. Thanks for that.

Thank you, I'll include it in the next patch set.

> I am not sure we need a full-fledged TAP test for testing
> subscription. I wouldn't object to it, but TAP tests are heavy. It
> should be possible to write the same test as a SQL test by creating
> two databases and switching between them. Do you think it's worth
> trying that way?

I'm not entirely sure what you mean here, but I am open to test
simplifications if you see an opportunity.

Regards,
	Jeff Davis
> 

[1] 
https://www.postgresql.org/message-id/172273.1693403385%40sss.pgh.pa.us

  35. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-01-23T09:51:37Z

    On Tue, Jan 23, 2024 at 12:33 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2024-01-22 at 18:41 +0530, Ashutosh Bapat wrote:
> > 0002 adds a prefix "regress_" to almost every object that is created
> > in foreign_data.sql.
>
> psql \dew outputs the owner, which in the case of a built-in FDW is the
> bootstrap superuser, which is not a stable name. I used the prefix to
> exclude the built-in FDW -- if you have a better suggestion, please let
> me know. (Though reading below, we might not even want a built-in FDW.)

I am with the prefix. The changes it causes make review difficult. If
you can separate those changes into a patch that will help.

>
> > Dummy FDW makes me nervous. The way it's written, it may grow into a
> > full-fledged postgres_fdw and in the process might acquire the same
> > concerns that postgres_fdw has today. But I will study the patches
> > and
> > discussion around it more carefully.
>
> I introduced that based on this comment[1].
>
> I also thought it fit with your previous suggestion to make it work
> with postgres_fdw, but I suppose it's not required. We could just not
> offer the built-in FDW, and expect users to either use postgres_fdw or
> create their own dummy FDW.

I am fine with this.

-- 
Best Wishes,
Ashutosh Bapat

  36. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-01-24T01:45:07Z

    On Tue, 2024-01-23 at 15:21 +0530, Ashutosh Bapat wrote:
> I am with the prefix. The changes it causes make review difficult. If
> you can separate those changes into a patch that will help.

I ended up just removing the dummy FDW. Real users are likely to want
to use postgres_fdw, and if not, it's easy enough to issue a CREATE
FOREIGN DATA WRAPPER. Or I can bring it back if desired.

Updated patch set (patches are renumbered):

  * removed dummy FDW and test churn
  * made a new pg_connection_validator function which leaves
postgresql_fdw_validator in place. (I didn't document the new function
-- should I?)
  * included your tests improvements
  * removed dependency from the subscription to the user mapping -- we
don't depend on the user mapping for foreign tables, so we shouldn't
depend on them here. Of course a change to a user mapping still
invalidates the subscription worker and it will restart.
  * general cleanup

Overall it's simpler and hopefully easier to review. The patch to
introduce the pg_create_connection role could use some more discussion,
but I believe 0001 and 0002 are nearly ready.

Regards,
	Jeff Davis

  37. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-01-29T17:41:41Z

    On Wed, Jan 24, 2024 at 7:15 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Tue, 2024-01-23 at 15:21 +0530, Ashutosh Bapat wrote:
> > I am with the prefix. The changes it causes make review difficult. If
> > you can separate those changes into a patch that will help.
>
> I ended up just removing the dummy FDW. Real users are likely to want
> to use postgres_fdw, and if not, it's easy enough to issue a CREATE
> FOREIGN DATA WRAPPER. Or I can bring it back if desired.
>
> Updated patch set (patches are renumbered):
>
>   * removed dummy FDW and test churn
>   * made a new pg_connection_validator function which leaves
> postgresql_fdw_validator in place. (I didn't document the new function
> -- should I?)
>   * included your tests improvements
>   * removed dependency from the subscription to the user mapping -- we
> don't depend on the user mapping for foreign tables, so we shouldn't
> depend on them here. Of course a change to a user mapping still
> invalidates the subscription worker and it will restart.
>   * general cleanup
>
> Overall it's simpler and hopefully easier to review. The patch to
> introduce the pg_create_connection role could use some more discussion,
> but I believe 0001 and 0002 are nearly ready.

Thanks for the patches. I have some comments on v9-0001:

1.
+SELECT pg_conninfo_from_server('testserver1', CURRENT_USER, false);
+      pg_conninfo_from_server
+-----------------------------------
+ user = 'value' password = 'value'

Isn't this function an unsafe one as it shows the password? I don't
see its access being revoked from the public. If it seems important
for one to understand how the server forms a connection string by
gathering bits and pieces from foreign server and user mapping, why
can't it look for the password in the result string and mask it before
returning it as output?

2.
+ */
+typedef const struct ConnectionOption *(*walrcv_conninfo_options_fn) (void);
+

struct here is unnecessary as the structure definition of
ConnectionOption is typedef-ed already.

3.
+  OPTIONS (user 'publicuser', password $pwd$'\"$# secret'$pwd$);

Is pwd here present working directory name? If yes, isn't it going to
be different on BF animals making test output unstable?

4.
-struct ConnectionOption
+struct TestConnectionOption
 {

How about say PgFdwConnectionOption instead of TestConnectionOption?

5. Comment #4 makes me think - why not get rid of
postgresql_fdw_validator altogether and use pg_connection_validator
instead for testing purposes? The tests don't complain much, see the
patch Remove-deprecated-postgresql_fdw_validator.diff created on top
of v9-0001.

I'll continue to review the other patches.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  38. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-01-29T17:47:40Z

    On Mon, Jan 29, 2024 at 11:11 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Jan 24, 2024 at 7:15 AM Jeff Davis <pgsql@j-davis.com> wrote:
> >
> > On Tue, 2024-01-23 at 15:21 +0530, Ashutosh Bapat wrote:
> > > I am with the prefix. The changes it causes make review difficult. If
> > > you can separate those changes into a patch that will help.
> >
> > I ended up just removing the dummy FDW. Real users are likely to want
> > to use postgres_fdw, and if not, it's easy enough to issue a CREATE
> > FOREIGN DATA WRAPPER. Or I can bring it back if desired.
> >
> > Updated patch set (patches are renumbered):
> >
> >   * removed dummy FDW and test churn
> >   * made a new pg_connection_validator function which leaves
> > postgresql_fdw_validator in place. (I didn't document the new function
> > -- should I?)
> >   * included your tests improvements
> >   * removed dependency from the subscription to the user mapping -- we
> > don't depend on the user mapping for foreign tables, so we shouldn't
> > depend on them here. Of course a change to a user mapping still
> > invalidates the subscription worker and it will restart.
> >   * general cleanup
> >
> > Overall it's simpler and hopefully easier to review. The patch to
> > introduce the pg_create_connection role could use some more discussion,
> > but I believe 0001 and 0002 are nearly ready.
>
> Thanks for the patches. I have some comments on v9-0001:
>
> 1.
> +SELECT pg_conninfo_from_server('testserver1', CURRENT_USER, false);
> +      pg_conninfo_from_server
> +-----------------------------------
> + user = 'value' password = 'value'
>
> Isn't this function an unsafe one as it shows the password? I don't
> see its access being revoked from the public. If it seems important
> for one to understand how the server forms a connection string by
> gathering bits and pieces from foreign server and user mapping, why
> can't it look for the password in the result string and mask it before
> returning it as output?
>
> 2.
> + */
> +typedef const struct ConnectionOption *(*walrcv_conninfo_options_fn) (void);
> +
>
> struct here is unnecessary as the structure definition of
> ConnectionOption is typedef-ed already.
>
> 3.
> +  OPTIONS (user 'publicuser', password $pwd$'\"$# secret'$pwd$);
>
> Is pwd here present working directory name? If yes, isn't it going to
> be different on BF animals making test output unstable?
>
> 4.
> -struct ConnectionOption
> +struct TestConnectionOption
>  {
>
> How about say PgFdwConnectionOption instead of TestConnectionOption?
>
> 5. Comment #4 makes me think - why not get rid of
> postgresql_fdw_validator altogether and use pg_connection_validator
> instead for testing purposes? The tests don't complain much, see the
> patch Remove-deprecated-postgresql_fdw_validator.diff created on top
> of v9-0001.
>
> I'll continue to review the other patches.

I forgot to attach the diff patch as specified in comment #5, please
find the attached. Sorry for the noise.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  39. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-01-30T10:47:55Z

    On Wed, Jan 24, 2024 at 7:15 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Tue, 2024-01-23 at 15:21 +0530, Ashutosh Bapat wrote:
> > I am with the prefix. The changes it causes make review difficult. If
> > you can separate those changes into a patch that will help.
>
> I ended up just removing the dummy FDW. Real users are likely to want
> to use postgres_fdw, and if not, it's easy enough to issue a CREATE
> FOREIGN DATA WRAPPER. Or I can bring it back if desired.
>
> Updated patch set (patches are renumbered):
>
>   * removed dummy FDW and test churn
>   * made a new pg_connection_validator function which leaves
> postgresql_fdw_validator in place. (I didn't document the new function
> -- should I?)
>   * included your tests improvements
>   * removed dependency from the subscription to the user mapping -- we
> don't depend on the user mapping for foreign tables, so we shouldn't
> depend on them here. Of course a change to a user mapping still
> invalidates the subscription worker and it will restart.
>   * general cleanup
>

Thanks.

> Overall it's simpler and hopefully easier to review. The patch to
> introduce the pg_create_connection role could use some more discussion,
> but I believe 0001 and 0002 are nearly ready.

0001 commit message says "in preparation of CREATE SUBSCRIPTION" but I
do not see the function being used anywhere except in testcases. Am I
missing something? Is this function necessary for this feature?

But more importantly this function and its minions are closely tied
with libpq and not an FDW. Converting a server and user mapping to
conninfo should be delegated to the FDW being used since that FDW
knows best how to use those options. Similarly options_to_conninfo()
should be delegated to the FDW. I imagine that the FDWs which want to
support subscriptions will need to implement hooks in
WalReceiverFunctionsType which seems to be designed to be pluggable.
--- quote
This API should be considered internal at the moment, but we could open it
up for 3rd party replacements of libpqwalreceiver in the future, allowing
pluggable methods for receiving WAL.
--- unquote
Not all of those hooks are applicable to every FDW since the publisher
may be different and may not provide all the functionality. So we
might need to rethink WalReceiverFunctionsType interface eventually.
But for now, we will need to change postgres_fdw to implement it.

We should mention something about the user mapping that will be used
to connect to SERVER when subscription specifies SERVER. I am not sure
where to mention this. May be we can get some clue from foreign server
documentation.

--
Best Wishes,
Ashutosh Bapat

  40. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-01-30T20:45:39Z

    On Tue, 2024-01-30 at 16:17 +0530, Ashutosh Bapat wrote:
> Converting a server and user mapping to
> conninfo should be delegated to the FDW being used since that FDW
> knows best how to use those options.

If I understand you correctly, you mean that there would be a new
optional function associated with an FDW (in addition to the HANDLER
and VALIDATOR) like "CONNECTION", which would be able to return the
conninfo from a server using that FDW. Is that right?

I like the idea -- it further decouples the logic from the core server.
I suspect it will make postgres_fdw the primary way (though not the
only possible way) to use this feature. There would be little need to
create a new builtin FDW to make this work.

To get the subscription invalidation right, we'd need to make the
(reasonable) assumption that the connection information is based only
on the FDW, server, and user mapping. A FDW wouldn't be able to use,
for example, some kind of configuration table or GUC to control how the
connection string gets created. That's easy enough to solve with
documentation.

I'll work up a new patch for this.


Regards,
	Jeff Davis

  41. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-01-31T05:40:00Z

    On Wed, Jan 31, 2024 at 2:16 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Tue, 2024-01-30 at 16:17 +0530, Ashutosh Bapat wrote:
> > Converting a server and user mapping to
> > conninfo should be delegated to the FDW being used since that FDW
> > knows best how to use those options.
>
> If I understand you correctly, you mean that there would be a new
> optional function associated with an FDW (in addition to the HANDLER
> and VALIDATOR) like "CONNECTION", which would be able to return the
> conninfo from a server using that FDW. Is that right?

I am not sure whether it fits {HANDLER,VALIDATOR} set or should be
part of FdwRoutine or a new set of hooks similar to FdwRoutine. But
something like that. Since the hooks for query planning and execution
have different characteristics from the ones used for replication, it
might make sense to create a new set of hooks similar to FdwRoutine,
say FdwReplicationRoutines and rename FdwRoutines to FdwQueryRoutines.
This way, we know whether an FDW can handle subscription connections
or not. A SERVER whose FDW does not support replication routines
should not be used with a subscription.

>
> I like the idea -- it further decouples the logic from the core server.
> I suspect it will make postgres_fdw the primary way (though not the
> only possible way) to use this feature. There would be little need to
> create a new builtin FDW to make this work.

That's what I see as well. I am glad that we are on the same page.

>
> To get the subscription invalidation right, we'd need to make the
> (reasonable) assumption that the connection information is based only
> on the FDW, server, and user mapping. A FDW wouldn't be able to use,
> for example, some kind of configuration table or GUC to control how the
> connection string gets created. That's easy enough to solve with
> documentation.
>

I think that's true for postgres_fdw as well right? But I think it's
more important for a subscription since it's expected to live very
long almost as long as the server itself does. So I agree. But that's
FDW's responsibility.

-- 
Best Wishes,
Ashutosh Bapat

  42. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-03-08T08:20:32Z

    On Wed, 2024-01-31 at 11:10 +0530, Ashutosh Bapat wrote:
> > I like the idea -- it further decouples the logic from the core
> > server.
> > I suspect it will make postgres_fdw the primary way (though not the
> > only possible way) to use this feature. There would be little need
> > to
> > create a new builtin FDW to make this work.
> 
> That's what I see as well. I am glad that we are on the same page.

Implemented in v11, attached.

Is this what you had in mind? It leaves a lot of the work to
postgres_fdw and it's almost unusable without postgres_fdw.

That's not a bad thing, but it makes the core functionality a bit
harder to test standalone. I can work on the core tests some more. The
postgres_fdw tests passed without modification, though, and offer a
simple example of how to use it.

Regards,
	Jeff Davis

  43. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-10-30T15:08:52Z

    On Fri, 2024-03-08 at 00:20 -0800, Jeff Davis wrote:
> Implemented in v11, attached.

Rebased, v12 attached.

Regards,
	Jeff Davis

  44. Re: [17] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2024-12-17T04:05:12Z

    On Wed, 2024-10-30 at 08:08 -0700, Jeff Davis wrote:
> Rebased, v12 attached.

Rebased v13 attached.

Regards,
	Jeff Davis

  45. Re: [18] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2025-02-28T23:05:10Z

    On Mon, 2024-12-16 at 20:05 -0800, Jeff Davis wrote:
> On Wed, 2024-10-30 at 08:08 -0700, Jeff Davis wrote:
> 

Rebased v14.

The approach has changed multiple times. It starte off with more in-
core code, but in response to review feedback, has become more
decoupled from core and more coupled to postgres_fdw.

But the patch has been about the same (just rebases) since March of
last year, and hasn't gotten feedback since. I still think it's a nice
feature, but I'd like some feedback on the externals of the feature.

As a note, this will require a version bump for postgres_fdw for the
new connection method.

Regards,
	Jeff Davis

  46. Re: [18] CREATE SUBSCRIPTION ... SERVER

    vignesh C <vignesh21@gmail.com> — 2025-03-24T12:56:44Z

    On Sat, 1 Mar 2025 at 04:35, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2024-12-16 at 20:05 -0800, Jeff Davis wrote:
> > On Wed, 2024-10-30 at 08:08 -0700, Jeff Davis wrote:
> >
>
> Rebased v14.
>
> The approach has changed multiple times. It starte off with more in-
> core code, but in response to review feedback, has become more
> decoupled from core and more coupled to postgres_fdw.
>
> But the patch has been about the same (just rebases) since March of
> last year, and hasn't gotten feedback since. I still think it's a nice
> feature, but I'd like some feedback on the externals of the feature.

+1 for this feature.

I started having a look at the patch, here are some initial comments:
1) The hint given here does not help anymore as subscription is global object:
postgres=# drop server myserver ;
ERROR:  cannot drop server myserver because other objects depend on it
DETAIL:  user mapping for vignesh on server myserver depends on server myserver
subscription tap_sub depends on server myserver
HINT:  Use DROP ... CASCADE to drop the dependent objects too.

postgres=# drop server myserver cascade;
NOTICE:  drop cascades to 2 other objects
DETAIL:  drop cascades to user mapping for vignesh on server myserver
drop cascades to subscription tap_sub
ERROR:  global objects cannot be deleted by doDeletion

Should we do anything about this?

2) I felt this change is not required as TAP_TESTS is already defined:
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index adfbd2ef758..59b805656c1 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -19,6 +19,8 @@ DATA = postgres_fdw--1.0.sql
postgres_fdw--1.0--1.1.sql postgres_fdw--1.1--1.2.s
 REGRESS = postgres_fdw query_cancel
 TAP_TESTS = 1

+TAP_TESTS = 1
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)

3) Copyright year to be updated:
diff --git a/contrib/postgres_fdw/t/010_subscription.pl
b/contrib/postgres_fdw/t/010_subscription.pl
new file mode 100644
index 00000000000..a39e8fdbba4
--- /dev/null
+++ b/contrib/postgres_fdw/t/010_subscription.pl
@@ -0,0 +1,71 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Basic logical replication test

4) I'm not sure if so many records are required, may be 10 records is enough:
+# Create some preexisting content on publisher
+$node_publisher->safe_psql('postgres',
+       "CREATE TABLE tab_ins AS SELECT a, a + 1 as b FROM
generate_series(1,1002) AS a");
+

5) Should subscription be server and user mapping here in the comments?
+       /* Keep us informed about subscription changes. */
+       CacheRegisterSyscacheCallback(FOREIGNSERVEROID,
+
subscription_change_cb,
+                                                                 (Datum) 0);
+       /* Keep us informed about subscription changes. */
+       CacheRegisterSyscacheCallback(USERMAPPINGOID,
+
subscription_change_cb,
+                                                                 (Datum) 0);


6) Should "initial data" be "incremental data" here:
+$node_publisher->wait_for_catchup('tap_sub');
+
+$result =
+  $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM
(SELECT f.b = l.b as match FROM tab_ins l, f_tab_ins f WHERE l.a =
f.a) WHERE match");
+is($result, qq(1050), 'check initial data was copied to subscriber');

Regards,
Vignesh

  47. Re: [18] CREATE SUBSCRIPTION ... SERVER

    vignesh C <vignesh21@gmail.com> — 2025-03-25T02:29:11Z

    On Sat, 1 Mar 2025 at 04:35, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2024-12-16 at 20:05 -0800, Jeff Davis wrote:
> > On Wed, 2024-10-30 at 08:08 -0700, Jeff Davis wrote:
> >
>
> Rebased v14.
>
> The approach has changed multiple times. It starte off with more in-
> core code, but in response to review feedback, has become more
> decoupled from core and more coupled to postgres_fdw.
>
> But the patch has been about the same (just rebases) since March of
> last year, and hasn't gotten feedback since. I still think it's a nice
> feature, but I'd like some feedback on the externals of the feature.

Few comments:
1) \dRs+ sub does not include the server info:
postgres=# \dRs+ sub*

                                 List of subscriptions
 Name |  Owner  | Enabled | Publication | Binary | Streaming |
Two-phase commit | Disable on error | Origin | Password required | Run
as owner? | Failover | Synchronous commit |
    Conninfo                 | Skip LSN
------+---------+---------+-------------+--------+-----------+------------------+------------------+--------+-------------------+---------------+----------+--------------------+-------------
-----------------------------+----------
 sub  | vignesh | t       | {pub1}      | f      | parallel  | d
         | f                | any    | t                 | f
  | f        | off                |
                             | 0/0

2) Tab completion for alter subscription also should include server:
+++ b/src/bin/psql/tab-complete.in.c
@@ -3704,7 +3704,7 @@ match_previous_words(int pattern_id,

 /* CREATE SUBSCRIPTION */
        else if (Matches("CREATE", "SUBSCRIPTION", MatchAny))
-               COMPLETE_WITH("CONNECTION");
+               COMPLETE_WITH("SERVER", "CONNECTION");


postgres=# alter subscription sub3
ADD PUBLICATION      DISABLE              ENABLE               REFRESH
PUBLICATION  SET
CONNECTION           DROP PUBLICATION     OWNER TO             RENAME
TO            SKIP (

3) In case of binary mode, pg_dump creates subscription using server
option, but not in normal mode:
+       if (dopt->binary_upgrade && fout->remoteVersion >= 180000)
+               appendPQExpBufferStr(query, " fs.srvname AS subservername,\n"
+                                                        "
o.remote_lsn AS suboriginremotelsn,\n"
+                                                        " s.subenabled,\n"
+                                                        " s.subfailover\n");
+       else
+               appendPQExpBufferStr(query, " NULL AS subservername,\n"
+                                                        " NULL AS
suboriginremotelsn,\n"
+                                                        " false AS
subenabled,\n"
+                                                        " false AS
subfailover\n");

If there is some specific reason, we should at least add some comments.

Regards,
Vignesh

  48. Re: [18] CREATE SUBSCRIPTION ... SERVER

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-04-02T12:28:30Z

    On Sat, 1 Mar 2025 at 04:35, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2024-12-16 at 20:05 -0800, Jeff Davis wrote:
> > On Wed, 2024-10-30 at 08:08 -0700, Jeff Davis wrote:
> >
>
> Rebased v14.
>
> The approach has changed multiple times. It starte off with more in-
> core code, but in response to review feedback, has become more
> decoupled from core and more coupled to postgres_fdw.
>
> But the patch has been about the same (just rebases) since March of
> last year, and hasn't gotten feedback since. I still think it's a nice
> feature, but I'd like some feedback on the externals of the feature.
>
> As a note, this will require a version bump for postgres_fdw for the
> new connection method.
>
Hi Jeff,

I reviewed the patch and I have a comment:

If version is >=18, the query will have 'suboriginremotelsn',
'subenabled', 'subfailover' twice.

  if (fout->remoteVersion >= 170000)
  appendPQExpBufferStr(query,
- " s.subfailover\n");
+ " s.subfailover,\n");
  else
  appendPQExpBuffer(query,
-   " false AS subfailover\n");
+   " false AS subfailover,\n");
+
+ if (dopt->binary_upgrade && fout->remoteVersion >= 180000)
+ appendPQExpBufferStr(query, " fs.srvname AS subservername,\n"
+ " o.remote_lsn AS suboriginremotelsn,\n"
+ " s.subenabled,\n"
+ " s.subfailover\n");
+ else
+ appendPQExpBufferStr(query, " NULL AS subservername,\n"
+ " NULL AS suboriginremotelsn,\n"
+ " false AS subenabled,\n"
+ " false AS subfailover\n");

query formed is something like:
"SELECT s.tableoid, s.oid, s.subname,\n s.subowner,\n s.subconninfo,
s.subslotname, s.subsynccommit,\n s.subpublications,\n s.subbinary,\n
s.substream,\n s.subtwophasestate,\n s.subdisableonerr,\n
s.subpasswordrequired,\n s.subrunasowner,\n s.suborigin,\n NULL AS
suboriginremotelsn,\n false AS subenabled,\n s.subfailover,\n NULL AS
subservername,\n NULL AS suboriginremotelsn,\n false AS subenabled,\n
false AS subfailover\nFROM pg_subscription s\nWHERE s.subdbid =
(SELECT oid FROM pg_database\n.."

is it expected?

Thanks and Regards,
Shlok Kyal

  49. Re: [18] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2025-04-08T20:19:03Z

    On Wed, 2025-04-02 at 17:58 +0530, Shlok Kyal wrote:
> I reviewed the patch and I have a comment:

Thank you and vignesh for the feedback. This patch didn't quite make it
for v18, but I will address it for the next CF.

Regards,
	Jeff Davis

  50. Re: [18] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2025-12-26T21:52:57Z

    On Wed, 2025-04-02 at 17:58 +0530, Shlok Kyal wrote:
> I reviewed the patch and I have a comment:
> 
> If version is >=18, the query will have 'suboriginremotelsn',
> 'subenabled', 'subfailover' twice.

Thank you. Fixed and rebased.

Note that this patch will require a postgres_fdw version bump.

Regards,
	Jeff Davis

  51. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-01-11T04:55:01Z

    On Fri, 2025-12-26 at 13:52 -0800, Jeff Davis wrote:
> On Wed, 2025-04-02 at 17:58 +0530, Shlok Kyal wrote:
> > I reviewed the patch and I have a comment:
> > 
> > If version is >=18, the query will have 'suboriginremotelsn',
> > 'subenabled', 'subfailover' twice.
> 
> Thank you. Fixed and rebased.

Attached new version with significant changes:

  - fixed several issues (including some improper merges in the last
rebase)
  - refactored to share code between postgres_fdw_connection() and
connect_pg_server()
  - added docs in postgres_fdw
  - added tests in core
  - bumped postgres_fdw version to 1.3

Regards,
	Jeff Davis

  52. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Masahiko Sawada <sawada.mshk@gmail.com> — 2026-02-04T04:53:20Z

    On Sat, Jan 10, 2026 at 8:55 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2025-12-26 at 13:52 -0800, Jeff Davis wrote:
> > On Wed, 2025-04-02 at 17:58 +0530, Shlok Kyal wrote:
> > > I reviewed the patch and I have a comment:
> > >
> > > If version is >=18, the query will have 'suboriginremotelsn',
> > > 'subenabled', 'subfailover' twice.
> >
> > Thank you. Fixed and rebased.
>
> Attached new version with significant changes:
>
>   - fixed several issues (including some improper merges in the last
> rebase)
>   - refactored to share code between postgres_fdw_connection() and
> connect_pg_server()
>   - added docs in postgres_fdw
>   - added tests in core
>   - bumped postgres_fdw version to 1.3
>

I've reviewed the latest patch set. I understand the motivation behind
this proposal and find it useful. Here are some comments:

@@ -5580,6 +5580,8 @@ fdw_option:
            | NO HANDLER                        { $$ =
makeDefElem("handler", NULL, @1); }
            | VALIDATOR handler_name            { $$ =
makeDefElem("validator", (Node *) $2, @1); }
            | NO VALIDATOR                      { $$ =
makeDefElem("validator", NULL, @1); }
+           | CONNECTION handler_name           { $$ =
makeDefElem("connection", (Node *) $2, @1); }
+           | NO CONNECTION                     { $$ =
makeDefElem("connection", NULL, @1); }
        ;

The documentation for ALTER FOREIGN DATA WRAPPER needs to be updated.

---
The security section[1] of logical replication chapter would also need
to be updated. Currently we have:

To create a subscription, the user must have the privileges of the
pg_create_subscription role, as well as CREATE privileges on the
database.

IIUC if the user uses the SERVER clause, they must have the USAGE
privilege on the foreign server too.

---
We might want to mention in the documentation of CREATE SERVER[2] that
a foreign server's name can be used to connect publication in CREATE
SUBSCRIPTION as we have a similar description for dblink_connect():

When using the dblink module, a foreign server's name can be used as
an argument of the dblink_connect function to indicate the connection
parameters. It is necessary to have the USAGE privilege on the foreign
server to be able to use it in this way.

---
dblink_connect() function can retrieve the connection string from a
foreign server specified in the second argument, which is a very
similar use case to CREATE SUBSCRIPTION. Should we make dblink use the
new function ForeignServerConnectionString() to get the connection
string (in get_connect_string())?

---
It would be better to enhance psql's \dRs command to show the server
name specified in the subscription.

Regards,

[1] https://www.postgresql.org/docs/devel/logical-replication-security.html
[2] https://www.postgresql.org/docs/devel/sql-createserver.html

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  53. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-02-26T19:12:01Z

    On Wed, 2026-02-04 at 13:53 +0900, Masahiko Sawada wrote:
> I've reviewed the latest patch set. I understand the motivation
> behind
> this proposal and find it useful.

Thank you, that's important feedback.

> The documentation for ALTER FOREIGN DATA WRAPPER needs to be updated.

Done.

> ---
> The security section[1] of logical replication chapter would also
> need
> to be updated.

Done.

> We might want to mention in the documentation of CREATE SERVER[2]
> that
> a foreign server's name can be used to connect publication in CREATE
> SUBSCRIPTION as we have a similar description for dblink_connect():

Done.

> ---
> dblink_connect() function can retrieve the connection string from a
> foreign server specified in the second argument, which is a very
> similar use case to CREATE SUBSCRIPTION. Should we make dblink use
> the
> new function ForeignServerConnectionString() to get the connection
> string (in get_connect_string())?

ForeignServerConnectionString() goes through the new FDW
connection_function, whereas dblink builds the string itself.
Technically, changing that could break things, but overall it seems to
make sense. I added this as a separate commit.

> ---
> It would be better to enhance psql's \dRs command to show the server
> name specified in the subscription.

Good idea, done.

Regards,
	Jeff Davis

  54. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-02T21:34:23Z

    On Thu, 2026-02-26 at 11:12 -0800, Jeff Davis wrote:
> On Wed, 2026-02-04 at 13:53 +0900, Masahiko Sawada wrote:
> > I've reviewed the latest patch set. I understand the motivation
> > behind
> > this proposal and find it useful.
> 
> Thank you, that's important feedback.

Attached v18:

  * rebase
  * Changed ForeignServerConnectionString() to use a local variable
rather than a static. It's not very performance-sensitive, so it's OK
to create a memory context for each invocation, which will be deleted.
I'm not aware of an actual problem in the previous code, but it seemed
a bit less safe.

I plan to commit the main patch (v18-0001) soon, after rechecking some
details (like the postgres_fdw upgrade). v18-0002 could use some review
first. 

Regards,
	Jeff Davis

  55. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Masahiko Sawada <sawada.mshk@gmail.com> — 2026-03-03T18:19:54Z

    On Mon, Mar 2, 2026 at 1:34 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Thu, 2026-02-26 at 11:12 -0800, Jeff Davis wrote:
> > On Wed, 2026-02-04 at 13:53 +0900, Masahiko Sawada wrote:
> > > I've reviewed the latest patch set. I understand the motivation
> > > behind
> > > this proposal and find it useful.
> >
> > Thank you, that's important feedback.
>
> Attached v18:
>
>   * rebase
>   * Changed ForeignServerConnectionString() to use a local variable
> rather than a static. It's not very performance-sensitive, so it's OK
> to create a memory context for each invocation, which will be deleted.
> I'm not aware of an actual problem in the previous code, but it seemed
> a bit less safe.
>
> I plan to commit the main patch (v18-0001) soon, after rechecking some
> details (like the postgres_fdw upgrade).

I have a few minor comments:

+   Oid         subserver;      /* Set if connecting with server */
+

Do we want to add BKI_LOOKUP(pg_foreign_data_wrapper) here?

---
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+

Need to update the copyright year.

The rest looks good to me.

> v18-0002 could use some review
> first.

Thank you for making this patch. I'll look at this patch too.

FYI interestingly, dblink_fdw can also be used for subscription
connections like postgres_fdw. It made me think that it might be
interesting to implement a FDW that supports only the libpq connection
(i.e., NO HANDLER, NO VALIDATOR, and CONNECTION) as it provides the
connection management capability useful for subscriptions while users
can avoid any security risks in postgres_fdw that users might be
concerned about.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  56. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Amit Kapila <amit.kapila16@gmail.com> — 2026-03-05T03:51:53Z

    On Tue, Mar 3, 2026 at 3:04 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> Attached v18:
>

I haven't checked the details but while glancing at the patch, I have
few observations:
1.
@@ -92,9 +92,11 @@
CATALOG(pg_subscription,6100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW
  * exceeded max_retention_duration, when
  * defined */

+ Oid subserver; /* Set if connecting with server */
+
 #ifdef CATALOG_VARLEN /* variable-length fields start here */
  /* Connection string to the publisher */
- text subconninfo BKI_FORCE_NOT_NULL;
+ text subconninfo; /* Set if connecting with connection string */

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 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, subenabled,
              subbinary, substream, subtwophasestate, subdisableonerr,
  subpasswordrequired, subrunasowner, subfailover,
              subretaindeadtuples, submaxretention, subretentionactive,
              subslotname, subsynccommit, subpublications, suborigin)
    ON pg_subscription TO public;

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

-- 
With Regards,
Amit Kapila.

  57. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-05T08:52:57Z

    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.

Regards,
	Jeff Davis

  58. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-05T19:00:43Z

    On Tue, 2026-03-03 at 10:19 -0800, Masahiko Sawada wrote:
> It made me think that it might be
> interesting to implement a FDW that supports only the libpq
> connection
> (i.e., NO HANDLER, NO VALIDATOR, and CONNECTION) as it provides the
> connection management capability useful for subscriptions while users
> can avoid any security risks in postgres_fdw that users might be
> concerned about.

We discussed some similar ideas earlier in the thread. My initial
proposal used special "FOR CONNECTION ONLY" syntax and had special
cases for the FDW.

Tom didn't like the special syntax and catalog work[1], and suggested a
dummy FDW. Ashutosh expressed concern about the scope of a dummy
FDW[2], and suggested that we just rely on postgres_fdw, which is the
current patch. I agree and think it's the right direction.

A new special-purpose FDW might be useful, but I don't understand the
exact use case yet, so I think it's better to leave it to the user to
implement a new FDW that does what they want. (Perhaps special safety
rules, etc.)

Regards,
	Jeff Davis

[1]
https://www.postgresql.org/message-id/172273.1693403385%40sss.pgh.pa.us
[2]
https://www.postgresql.org/message-id/CAExHW5uCzS-VeSYQHTHxFSdQik-f_O892xmzrzm2fuO+ro+otA@mail.gmail.com

  59. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-03-06T16:19:00Z

    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

  60. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Amit Kapila <amit.kapila16@gmail.com> — 2026-03-07T07:01:35Z

    On Thu, Mar 5, 2026 at 2:23 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> 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 also can't think of any straight-forward solution for it. I've not
thought in detail but can a new type of dependency be required to
solve this problem? I am not aware if we are doing something similar
in any other CASCADE operation, so even if we want to go with giving
ERROR for this case, it may be better to get somewhat wider acceptance
for the same unless few other people respond here and consider this as
an acceptable solution.

Few other minor comments:
======================
1.
+# Replicate the changes without columns
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");

I don't see a subscriber-side table or verification code to verify the
above test.

2.
+ Oid subserver BKI_LOOKUP_OPT(pg_foreign_server); /* If connection uses
+ * server */
+

Isn't it better to keep this along with other oids in the beginning of
the catalog, say after subowner? It will also avoid padding before
subserver field.

-- 
With Regards,
Amit Kapila.

  61. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Amit Kapila <amit.kapila16@gmail.com> — 2026-03-07T07:05:32Z

    On Fri, Mar 6, 2026 at 9:49 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> 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:
> >
> > 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?
>

It is because the launcher process needs to traverse all subscriptions
to start workers.

-- 
With Regards,
Amit Kapila.

  62. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Amit Kapila <amit.kapila16@gmail.com> — 2026-03-09T06:23:56Z

    On Sat, Mar 7, 2026 at 12:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Mar 5, 2026 at 2:23 PM Jeff Davis <pgsql@j-davis.com> wrote:
> >
>
> Few other minor comments:
> ======================
> 1.
> +# Replicate the changes without columns
> +$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
> +$node_publisher->safe_psql('postgres',
> + "INSERT INTO tab_no_col default VALUES");
>
> I don't see a subscriber-side table or verification code to verify the
> above test.
>

I see that the committed version (8185bb5347) has this part of the
test, isn't that test incomplete, if not, tell me what am I missing?
It seems I have sent this message after you have committed the last
version.

> 2.
> + Oid subserver BKI_LOOKUP_OPT(pg_foreign_server); /* If connection uses
> + * server */
> +
>
> Isn't it better to keep this along with other oids in the beginning of
> the catalog, say after subowner? It will also avoid padding before
> subserver field.
>

We can probably consider this one as well though there is no
correctness issue as such.

-- 
With Regards,
Amit Kapila.

  63. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-10T14:23:46Z

    On Mon, 2026-03-09 at 11:53 +0530, Amit Kapila wrote:
> > +# Replicate the changes without columns
> > +$node_publisher->safe_psql('postgres', "CREATE TABLE
> > tab_no_col()");
> > +$node_publisher->safe_psql('postgres',
> > + "INSERT INTO tab_no_col default VALUES");
> > 
> > I don't see a subscriber-side table or verification code to verify
> > the
> > above test.
> > 
> 
> I see that the committed version (8185bb5347) has this part of the
> test, isn't that test incomplete, if not, tell me what am I missing?

In 8185bb5347, contrib/postgres_fdw/t/010_subscription.pl has:

  ...
  # Setup structure on subscriber
  $node_subscriber->safe_psql('postgres', "CREATE EXTENSION
postgres_fdw");
  $node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int,
b int)");
  ...
  $result =
  $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM (SELECT
f.b = l.b as match FROM tab_ins l, f_tab_ins f WHERE l.a = f.a) WHERE
match");
  is($result, qq(1050), 'check that inserted data was copied to
subscriber');
  ...

which creates the subscriber-side table and verifies the result. If I
change 1050 -> 1051, then the test fails, so I think it's functioning. 

Perhaps I don't understand the question?

> It seems I have sent this message after you have committed the last
> version.

Yes, thank you, I will address those shortly.

Regards,
	Jeff Davis

  64. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-14T03:02:04Z

    On Sat, 2026-03-07 at 12:31 +0530, Amit Kapila wrote:
> Isn't it better to keep this along with other oids in the beginning
> of
> the catalog, say after subowner? It will also avoid padding before
> subserver field.

Right now it's close to conninfo, which seems reasonable. The other
fields don't seem very concerned about alignment, but if it's an issue
we can rearrange subserver along with submaxretention.

Regards,
	Jeff Davis

  65. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Amit Kapila <amit.kapila16@gmail.com> — 2026-03-14T09:44:27Z

    On Tue, Mar 10, 2026 at 7:53 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2026-03-09 at 11:53 +0530, Amit Kapila wrote:
> > > +# Replicate the changes without columns
> > > +$node_publisher->safe_psql('postgres', "CREATE TABLE
> > > tab_no_col()");
> > > +$node_publisher->safe_psql('postgres',
> > > + "INSERT INTO tab_no_col default VALUES");
> > >
> > > I don't see a subscriber-side table or verification code to verify
> > > the
> > > above test.
> > >
> >
> > I see that the committed version (8185bb5347) has this part of the
> > test, isn't that test incomplete, if not, tell me what am I missing?
>
> In 8185bb5347, contrib/postgres_fdw/t/010_subscription.pl has:
>
>   ...
>   # Setup structure on subscriber
>   $node_subscriber->safe_psql('postgres', "CREATE EXTENSION
> postgres_fdw");
>   $node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int,
> b int)");
>   ...
>   $result =
>   $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM (SELECT
> f.b = l.b as match FROM tab_ins l, f_tab_ins f WHERE l.a = f.a) WHERE
> match");
>   is($result, qq(1050), 'check that inserted data was copied to
> subscriber');
>   ...
>
> which creates the subscriber-side table and verifies the result.
>

I am talking about a table with the name tab_no_col whereas you are
talking about a table with the name tab_ins. The test doesn't create a
table with the name tab_no_col on the subscriber-side which makes it
redundant, am I missing something?

-- 
With Regards,
Amit Kapila.

  66. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-14T22:55:29Z

    On Fri, 2026-03-06 at 21:49 +0530, Ashutosh Bapat wrote:

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

Done.

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

It seems that foreign_data.sql expects there to be zero FDWs, servers,
and user mappings, so it's not quite that simple. I'm not entirely sure
why that is, but I suppose it's meant to be tested in 002_pg_dump.pl
instead.

I wrote the tests there (attached), which revealed that CREATE FOREIGN
DATA WRAPPER ... CONNECTION wasn't being dumped properly. I attached a
separate fix for that.

Unfortunately I don't think we can rely on regress.so being available
when 002_pg_dump.pl runs. Do you have an idea how I can effectively
test the FDW (which is needed to test the server and subscription)? I
suppose I could make it a built-in function, and that wouldn't be so
bad, but not ideal. Right now this test is failing for CI on debian
autoconf.

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

Done.

Attached series including patches to address Andres's and Amit's
comments, too.

Thank you!

Regards,
	Jeff Davis

  67. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-14T22:57:50Z

    On Sat, 2026-03-14 at 15:14 +0530, Amit Kapila wrote:
> I am talking about a table with the name tab_no_col whereas you are
> talking about a table with the name tab_ins. The test doesn't create
> a
> table with the name tab_no_col on the subscriber-side which makes it
> redundant, am I missing something?

I misread your message, sorry. You're right, there are some copy+paste
issues. Posted a patch over here:

http://postgr.es/m/7eb0c03b4312b32cb76d340023b39a751745a1f9.camel@j-davis.com

to clean it up.

Regards,
	Jeff Davis

  68. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Amit Kapila <amit.kapila16@gmail.com> — 2026-03-16T05:38:49Z

    On Sun, Mar 15, 2026 at 4:25 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> Attached series including patches to address Andres's and Amit's
> comments, too.
>

0001 LGTM.

0003:
@@ -5056,8 +5058,15 @@ maybe_reread_subscription(void)
  started_tx = true;
  }

- /* Ensure allocations in permanent context. */
- oldctx = MemoryContextSwitchTo(ApplyContext);
+ newctx = AllocSetContextCreate(ApplyContext,
+    "Subscription Context",
+    ALLOCSET_SMALL_SIZES);
+

Won't it be sufficient if we just reset MySubscriptionCtx here or in
callback subscription_change_cb()?

-- 
With Regards,
Amit Kapila.

  69. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-16T16:26:41Z

    On Mon, 2026-03-16 at 11:08 +0530, Amit Kapila wrote:
> Won't it be sufficient if we just reset MySubscriptionCtx here or in
> callback subscription_change_cb()?

The old and new subscriptions are compared against eachother (to see
whether to restart the worker or not), so they both have to exist at
the same time. If we put them in the same context, then we can't reset
it.

I suppose we could have just two contexts and switch back and forth
between them, resetting the last one. But that doesn't seem to be worth
the trouble.

Regards,
	Jeff Davis

  70. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Amit Kapila <amit.kapila16@gmail.com> — 2026-03-17T05:59:40Z

    On Mon, Mar 16, 2026 at 9:56 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2026-03-16 at 11:08 +0530, Amit Kapila wrote:
> > Won't it be sufficient if we just reset MySubscriptionCtx here or in
> > callback subscription_change_cb()?
>
> The old and new subscriptions are compared against eachother (to see
> whether to restart the worker or not), so they both have to exist at
> the same time. If we put them in the same context, then we can't reset
> it.
>
> I suppose we could have just two contexts and switch back and forth
> between them, resetting the last one. But that doesn't seem to be worth
> the trouble.
>

Yeah, or the other possibility could be to let the newsub information
get allocated in the current transaction context and reset the
subscription context if we decide not to exit from the worker. Then
copy/get the subscription info in subscription context but not sure if
that is worth it. The minor oddity in the proposed approach is that
the worker will exit in many cases after allocating the new context
but that may be the best we can do here.

-- 
With Regards,
Amit Kapila.

  71. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-17T16:56:24Z

    On Tue, 2026-03-17 at 11:29 +0530, Amit Kapila wrote:
> Yeah, or the other possibility could be to let the newsub information
> get allocated in the current transaction context and reset the
> subscription context if we decide not to exit from the worker. Then
> copy/get the subscription info in subscription context but not sure
> if
> that is worth it.

Then we have to invent a deep copy for the Subscription, and we've
already seen that the FreeSubscrpition() method was not being
maintained properly.

> The minor oddity in the proposed approach is that
> the worker will exit in many cases after allocating the new context
> but that may be the best we can do here.

Agreed.

Regards,
	Jeff Davis

  72. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-18T09:39:34Z

    On 2026-03-17, Jeff Davis wrote:

> Then we have to invent a deep copy for the Subscription, and we've
> already seen that the FreeSubscrpition() method was not being
> maintained properly.

Maybe another possibility would be to use a separate memory context for each subscription, initially making it a child of the transaction context, and then reparenting it as appropriate.

-- 
Álvaro Herrera

  73. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-18T18:01:43Z

    On 2026-Mar-18, Álvaro Herrera wrote:

> On 2026-03-17, Jeff Davis wrote:
> 
> > Then we have to invent a deep copy for the Subscription, and we've
> > already seen that the FreeSubscrpition() method was not being
> > maintained properly.
> 
> Maybe another possibility would be to use a separate memory context
> for each subscription, initially making it a child of the transaction
> context, and then reparenting it as appropriate.

I mean something like this on top of your 0003.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I love the Postgres community. It's all about doing things _properly_. :-)"
(David Garamond)

  74. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-18T19:06:48Z

    On Sat, 2026-03-14 at 15:55 -0700, Jeff Davis wrote:
> Attached series including patches to address Andres's and Amit's
> comments, too.

Committed two patches.

New patch 0004: fixes missing dependencies from the FDW to the
connection function. There's a related pre-existing issue with the
dependency from the FDW to the handler function, which I will post as a
separate backportable bugfix.

I'd still like to find a good way to add pg_dump tests. The only idea I
have now is to build the test function into core postgres (without
pg_proc entry), which might be worthwhile.

Regards,
	Jeff Davis

  75. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Amit Kapila <amit.kapila16@gmail.com> — 2026-03-21T10:55:06Z

    On Wed, Mar 18, 2026 at 11:31 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2026-Mar-18, Álvaro Herrera wrote:
>
> > On 2026-03-17, Jeff Davis wrote:
> >
> > > Then we have to invent a deep copy for the Subscription, and we've
> > > already seen that the FreeSubscrpition() method was not being
> > > maintained properly.
> >
> > Maybe another possibility would be to use a separate memory context
> > for each subscription, initially making it a child of the transaction
> > context, and then reparenting it as appropriate.
>
> I mean something like this on top of your 0003.
>

+1. This approach and patch looks like a better way to deal with this issue.

-- 
With Regards,
Amit Kapila.

  76. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-03-24T22:47:47Z

    On Sat, 2026-03-21 at 16:25 +0530, Amit Kapila wrote:
> > > Maybe another possibility would be to use a separate memory
> > > context
> > > for each subscription, initially making it a child of the
> > > transaction
> > > context, and then reparenting it as appropriate.
> > 
> > I mean something like this on top of your 0003.
> > 
> 
> +1. This approach and patch looks like a better way to deal with this
> issue.

Thank you, pushed.

A couple minor adjustments: in GetSubscription(), I create the context
after the early return, in case the subscription isn't found. Also, I
combined the:

   if (newsub)
      ...

   if (!newsub)
      ...

into if/else.

The only remaining issue in this thread is how to make
test_fdw_connect() available during the pg_dump tests without polluting
pg_proc. Is there a reasonable way to do that?

Regards,
	Jeff Davis

  77. RE: [19] CREATE SUBSCRIPTION ... SERVER

    Shinoda, Noriyoshi <noriyoshi.shinoda@hpe.com> — 2026-04-10T02:00:58Z

    Hi,
Thanks for developing this great feature.

> Committed two patches.
The commit of 0004 patch added the `fdwconnection` column to the pg_foreign_data_wrapper catalog. 
However, it seems the documentation is missing the definition for this column. The small patch attached adds the information for this column to catalog.sgml. There might be a better phrasing for the description text.

Regards,
Noriyoshi Shinoda
-----Original Message-----
From: Jeff Davis <pgsql@j-davis.com> 
Sent: Thursday, March 19, 2026 4:07 AM
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Joe Conway <mail@joeconway.com>; pgsql-hackers@postgresql.org
Subject: Re: [19] CREATE SUBSCRIPTION ... SERVER

On Sat, 2026-03-14 at 15:55 -0700, Jeff Davis wrote:
> Attached series including patches to address Andres's and Amit's 
> comments, too.

Committed two patches.

New patch 0004: fixes missing dependencies from the FDW to the connection function. There's a related pre-existing issue with the dependency from the FDW to the handler function, which I will post as a separate backportable bugfix.

I'd still like to find a good way to add pg_dump tests. The only idea I have now is to build the test function into core postgres (without pg_proc entry), which might be worthwhile.

Regards,
	Jeff Davis

  78. Re: [19] CREATE SUBSCRIPTION ... SERVER

    Jeff Davis <pgsql@j-davis.com> — 2026-04-10T03:33:49Z

    On Fri, 2026-04-10 at 02:00 +0000, Shinoda, Noriyoshi (PSD Japan FSI)
wrote:
> However, it seems the documentation is missing the definition for
> this column. The small patch attached adds the information for this
> column to catalog.sgml. There might be a better phrasing for the
> description text.

Thank you, committed with expanded wording. The
pg_subscription.subserver field was also missing documentation, and I
fixed that too.

Regards,
	Jeff Davis