Обсуждение: [17] CREATE SUBSCRIPTION ... SERVER

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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