Re: [17] CREATE SUBSCRIPTION ... SERVER
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
From 88fa1333ace4d15d72534d20d2cccb37748277f2 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Tue, 2 Jan 2024 13:42:48 -0800
Subject: [PATCH v11] CREATE SUSBCRIPTION ... SERVER.
Allow specifying a foreign server for CREATE SUBSCRIPTION, rather than
a raw connection string with CONNECTION.
Using a foreign server as a layer of indirection improves management
of multiple subscriptions to the same server. It also provides
integration with user mappings in case different subscriptions have
different owners or a subscription changes owners.
Discussion: https://postgr.es/m/61831790a0a937038f78ce09f8dd4cef7de7456a.ca...@j-davis.com
Reviewed-by: Ashutosh Bapat
---
contrib/postgres_fdw/Makefile | 4 +-
contrib/postgres_fdw/connection.c | 74
.../postgres_fdw/expected/postgres_fdw.out| 8 +
contrib/postgres_fdw/meson.build | 6 +
.../postgres_fdw/postgres_fdw--1.1--1.2.sql | 11 ++
contrib/postgres_fdw/postgres_fdw.control | 2 +-
contrib/postgres_fdw/sql/postgres_fdw.sql | 7 +
contrib/postgres_fdw/t/010_subscription.pl| 71
doc/src/sgml/ref/alter_subscription.sgml | 18 +-
doc/src/sgml/ref/create_subscription.sgml | 11 +-
src/backend/catalog/pg_subscription.c | 38 +++-
src/backend/commands/foreigncmds.c| 57 +-
src/backend/commands/subscriptioncmds.c | 167 --
src/backend/foreign/foreign.c | 42 +
src/backend/parser/gram.y | 22 +++
src/backend/replication/logical/worker.c | 16 +-
src/bin/pg_dump/pg_dump.c | 27 ++-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/psql/tab-complete.c | 2 +-
src/include/catalog/pg_foreign_data_wrapper.h | 3 +
src/include/catalog/pg_subscription.h | 7 +-
src/include/foreign/foreign.h | 3 +
src/include/nodes/parsenodes.h| 3 +
src/test/regress/expected/oidjoins.out| 1 +
24 files changed, 563 insertions(+), 38 deletions(-)
create mode 100644 contrib/postgres_fdw/postgres_fdw--1.1--1.2.sql
create mode 100644 contrib/postgres_fdw/t/010_subscription.pl
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index c1b0cad453..995a30c297 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -14,10 +14,12 @@ PG_CPPFLAGS = -I$(libpq_srcdir)
SHLIB_LINK_INTERNAL = $(libpq)
EXTENSION = postgres_fdw
-DATA = postgres_fdw--1.0.sql postgres_fdw--1.0--1.1.sql
+DATA = postgres_fdw--1.0.sql postgres_fdw--1.0--1.1.sql postgres_fdw--1.1--1.2.sql
REGRESS = postgres_fdw
+TAP_TESTS = 1
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 4931ebf591..a011e6df5f 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -113,6 +113,7 @@ static uint32 pgfdw_we_get_result = 0;
PG_FUNCTION_INFO_V1(postgres_fdw_get_connections);
PG_FUNCTION_INFO_V1(postgres_fdw_disconnect);
PG_FUNCTION_INFO_V1(postgres_fdw_disconnect_all);
+PG_FUNCTION_INFO_V1(postgres_fdw_connection);
/* prototypes of private functions */
static void make_new_connection(ConnCacheEntry *entry, UserMapping *user);
@@ -1972,6 +1973,79 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
}
}
+/*
+ * Values in connection strings must be enclosed in single quotes. Single
+ * quotes and backslashes must be escaped with backslash. NB: these rules are
+ * different from the rules for escaping a SQL literal.
+ */
+static void
+appendEscapedValue(StringInfo str, const char *val)
+{
+ appendStringInfoChar(str, '\'');
+ for (int i = 0; val[i] != '\0'; i++)
+ {
+ if (val[i] == '\\' || val[i] == '\'')
+ appendStringInfoChar(str, '\\');
+ appendStringInfoChar(str, val[i]);
+ }
+ appendStringInfoChar(str, '\'');
+}
+
+Datum
+postgres_fdw_connection(PG_FUNCTION_ARGS)
+{
+ /* TODO: consider memory usage */
+ Oid userid = PG_GETARG_OID(0);
+ Oid serverid = PG_GETARG_OID(1);
+ ForeignServer
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Wed, Jan 31, 2024 at 2:16 AM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Wed, Jan 24, 2024 at 7:15 AM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Mon, Jan 29, 2024 at 11:11 PM Bharath Rupireddy
wrote:
>
> On Wed, Jan 24, 2024 at 7:15 AM Jeff Davis 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
Remove-deprecated-postgresql_fdw_validator.diff
Description: Binary data
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Wed, Jan 24, 2024 at 7:15 AM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Tue, Jan 23, 2024 at 12:33 AM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
Hi Jeff,
On Tue, Jan 16, 2024 at 7:25 AM Jeff Davis 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
diff --git a/contrib/postgres_fdw/t/010_subscription.pl
b/contrib/postgres_fdw/t/010_subscription.pl
index d1d80d0679..3ae2b6da4a 100644
--- a/contrib/postgres_fdw/t/010_subscription.pl
+++ b/contrib/postgres_fdw/t/010_subscription.pl
@@ -20,7 +20,7 @@ $node_subscriber->start;
# Create some preexisting content on publisher
$node_publisher->safe_psql('postgres',
- "CREATE TABLE tab_ins AS SELECT generate_series(1,1002) AS a");
+ "CREATE TABLE tab_ins AS SELECT a, a + 1 as b FROM
generate_series(1,1002) AS a");
# Replicate the changes without columns
$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
@@ -29,7 +29,7 @@ $node_publisher->safe_psql('postgres',
# Setup structure on subscriber
$node_subscriber->safe_psql('postgres', "CREATE EXTENSION postgres_fdw");
-$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int)");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int, b int)");
# Setup logical replication
my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
@@ -45,6 +45,9 @@ $node_subscriber->safe_psql('postgres',
"CREATE USER MAPPING FOR PUBLIC SERVER tap_server"
);
+$node_subscriber->safe_psql('postgres',
+ "CREATE FOREIGN TABLE f_tab_ins (a int, b int) SERVER tap_server
OPTIONS(table_name 'tab_ins')"
+);
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION tap_sub SERVER tap_server PUBLICATION tap_pub WITH
(password_required=false)"
);
@@ -53,16 +56,16 @@ $node_subscriber->safe_psql('postgres',
$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub');
my $result =
- $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_ins");
+ $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(1002), 'check initial data was copied to subscriber');
$node_publisher->safe_psql('postgres',
- "INSERT INTO tab_ins SELECT generate_series(1,50)");
+ "INSERT INTO tab_ins SELECT a, a + 1 FROM generate_series(1003,1050)
a");
$node_publisher->wait_for_catchup('tap_sub');
$result =
- $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_ins");
-is($result, qq(1052), 'check initial data was copied to subscriber');
+ $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');
done_testing();
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Tue, Jan 16, 2024 at 7:25 AM Jeff Davis 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;
boolissecret;/* is option for a password? */
boolisdebug;/* 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Fri, Jan 5, 2024 at 1:34 PM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Fri, Jan 5, 2024 at 6:26 AM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Mon, Jan 1, 2024 at 12:29 AM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
From 0b8cb23157b86909d38cc10723f19d94787efed2 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Wed, 23 Aug 2023 10:31:16 -0700
Subject: [PATCH v4] CREATE SUBSCRIPTION ... SERVER.
---
doc/src/sgml/ref/alter_subscription.sgml | 18 +-
doc/src/sgml/ref/create_subscription.sgml | 16 +-
doc/src/sgml/user-manag.sgml | 12 +-
src/backend/catalog/Makefile | 1 +
src/backend/catalog/pg_subscription.c | 17 +-
src/backend/catalog/system_functions.sql | 2 +
src/backend/commands/subscriptioncmds.c | 197 ++--
src/backend/foreign/foreign.c | 214 ++
src/backend/parser/gram.y | 20 ++
src/backend/replication/logical/worker.c | 12 +-
src/bin/pg_dump/pg_dump.c | 63 +-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/psql/describe.c | 2 +-
src/bin/psql/tab-complete.c | 2 +-
src/include/catalog/meson.build | 1 +
src/include/catalog/pg_authid.dat | 5 +
.../catalog/pg_foreign_data_wrapper.dat | 22 ++
src/include/catalog/pg_proc.dat | 8 +
src/include/catalog/pg_subscription.h | 5 +-
src/include/foreign/foreign.h | 1 +
src/include/nodes/parsenodes.h| 3 +
src/test/regress/expected/foreign_data.out| 60 -
src/test/regress/expected/subscription.out| 38
src/test/regress/sql/foreign_data.sql | 41 +++-
src/test/regress/sql/subscription.sql | 39
src/test/subscription/t/001_rep_changes.pl| 57 +
26 files changed, 804 insertions(+), 53 deletions(-)
create mode 100644 src/include/catalog/pg_foreign_data_wrapper.dat
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 6d36ff0dc9..6d219145a9 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -21,6 +21,7 @@ PostgreSQL documentation
+ALTER SUBSCRIPTION name SERVER servername
ALTER SUBSCRIPTION name CONNECTION 'conninfo'
ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
@@ -94,13 +95,24 @@ ALTER SUBSCRIPTION name RENAME TO <
+
+SERVER servername
+
+
+ This clause replaces the foreign server or connection string originally
+ set by with the foreign server
+ servername.
+
+
+
+
CONNECTION 'conninfo'
- This clause replaces the connection string originally set by
- . See there for more
- information.
+ This clause replaces the foreign server or connection string originally
+ set by with the connection
+ string conninfo.
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index f1c20b3a46..8cf67516cf 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
CREATE SUBSCRIPTION subscription_name
-CONNECTION 'conninfo'
+{ SERVER servername | CONNECTION 'conninfo' }
PUBLICATION publication_name [, ...]
[ WITH ( subscription_parameter [= value] [, ... ] ) ]
@@ -77,6 +77,15 @@ CREATE SUBSCRIPTION subscription_name
+
+SERVER servername
+
+
+ A foreign server to use for the connection.
+
+
+
+
CONNECTION 'conninfo'
@@ -363,6 +372,11 @@ CREATE SUBSCRIPTION subscription_nametrue. Only superusers can set
this value to false.
+
+ Only allowed when using a connection string. If using a foreign
+ server, specify password_required as part of the
+ user mapping for the foreign server, instead.
+
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 92a299d2d3..4f4c20ba3c 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -687,11 +687,19 @@ DROP ROLE doomed_role;
Allow use of connection slots reserved via
.
+
+ pg_create_connection
+ Allow users to specify a connection string directly in CREATE
+ SUBSCRIPTION.
+
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
From 776cd8e5e1541c56b1767aa595fc609fdeffa5e3 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Wed, 23 Aug 2023 10:31:16 -0700
Subject: [PATCH v3] CREATE SUBSCRIPTION ... SERVER.
---
doc/src/sgml/ref/alter_subscription.sgml | 7 +-
doc/src/sgml/ref/create_subscription.sgml | 15 +-
doc/src/sgml/user-manag.sgml | 21 +-
src/backend/catalog/Makefile | 1 +
src/backend/catalog/pg_subscription.c | 17 +-
src/backend/catalog/system_functions.sql | 2 +
src/backend/commands/subscriptioncmds.c | 197 ++--
src/backend/foreign/foreign.c | 214 ++
src/backend/parser/gram.y | 20 ++
src/backend/replication/logical/worker.c | 12 +-
src/bin/pg_dump/pg_dump.c | 59 -
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/psql/describe.c | 2 +-
src/bin/psql/tab-complete.c | 2 +-
src/include/catalog/meson.build | 1 +
src/include/catalog/pg_authid.dat | 5 +
.../catalog/pg_foreign_data_wrapper.dat | 22 ++
src/include/catalog/pg_proc.dat | 8 +
src/include/catalog/pg_subscription.h | 5 +-
src/include/foreign/foreign.h | 1 +
src/include/nodes/parsenodes.h| 3 +
src/test/regress/expected/foreign_data.out| 60 -
src/test/regress/expected/subscription.out| 38
src/test/regress/sql/foreign_data.sql | 41 +++-
src/test/regress/sql/subscription.sql | 39
src/test/subscription/t/001_rep_changes.pl| 57 +
26 files changed, 799 insertions(+), 51 deletions(-)
create mode 100644 src/include/catalog/pg_foreign_data_wrapper.dat
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 6d36ff0dc9..f2235061bb 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -21,6 +21,7 @@ PostgreSQL documentation
+ALTER SUBSCRIPTION name SERVER servername
ALTER SUBSCRIPTION name CONNECTION 'conninfo'
ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
@@ -98,9 +99,9 @@ ALTER SUBSCRIPTION name RENAME TO <
CONNECTION 'conninfo'
- This clause replaces the connection string originally set by
- . See there for more
- information.
+ This clause replaces the foreign server or connection string originally
+ set by with the connection
+ string conninfo.
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index f1c20b3a46..cd76b2e32d 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
CREATE SUBSCRIPTION subscription_name
-CONNECTION 'conninfo'
+{ SERVER servername | CONNECTION 'conninfo' }
PUBLICATION publication_name [, ...]
[ WITH ( subscription_parameter [= value] [, ... ] ) ]
@@ -77,6 +77,15 @@ CREATE SUBSCRIPTION subscription_name
+
+SERVER servername
+
+
+ A foreign server to use for the connection.
+
+
+
+
CONNECTION 'conninfo'
@@ -363,6 +372,10 @@ CREATE SUBSCRIPTION subscription_nametrue. Only superusers can set
this value to false.
+
+ Only allowed when CONNECTION is
+ specified. Otherwise, see .
+
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 92a299d2d3..d63a33a4b3 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -687,11 +687,20 @@ DROP ROLE doomed_role;
Allow use of connection slots reserved via
.
+
+ pg_create_connection
+ Allow users with CREATE permission on the
+ database to issue CREATE
+ SERVER if FOR CONNECTION ONLY is
+ specified.
+
pg_create_subscription
Allow users with CREATE permission on the
- database to issue
- CREATE SUBSCRIPTION.
+ database to issue CREATE
+ SUBSCRIPTION. This role is a member of
+ pg_create_connection.
@@ -737,6 +746,14 @@ DROP ROLE doomed_role;
great care should be taken when granting these roles to
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Sat, Sep 2, 2023 at 1:41 AM Robert Haas wrote: > > On Fri, Sep 1, 2023 at 4:04 PM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Sat, Sep 2, 2023 at 12:24 AM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Fri, Sep 1, 2023 at 4:04 PM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Fri, Sep 1, 2023 at 2:47 AM Joe Conway 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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.ca...@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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Wed, Aug 30, 2023 at 1:19 PM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Wed, Aug 30, 2023 at 9:00 PM Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
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
Re: [17] CREATE SUBSCRIPTION ... SERVER
Jeff Davis 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
Re: [17] CREATE SUBSCRIPTION ... SERVER
Hi Jeff, On Wed, Aug 30, 2023 at 2:12 PM Jeff Davis 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
[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 From 422114a0bc1d928d257505bf31e99397cb8a6a8c Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Wed, 23 Aug 2023 10:31:16 -0700 Subject: [PATCH v1] CREATE SUBSCRIPTION ... SERVER. --- contrib/dblink/dblink.c | 17 +- contrib/dblink/expected/dblink.out
