Обсуждение: Re: [18] CREATE SUBSCRIPTION ... SERVER
On Mon, 2024-12-16 at 20:05 -0800, Jeff Davis wrote:
> On Wed, 2024-10-30 at 08:08 -0700, Jeff Davis wrote:
>
Rebased v14.
The approach has changed multiple times. It starte off with more in-
core code, but in response to review feedback, has become more
decoupled from core and more coupled to postgres_fdw.
But the patch has been about the same (just rebases) since March of
last year, and hasn't gotten feedback since. I still think it's a nice
feature, but I'd like some feedback on the externals of the feature.
As a note, this will require a version bump for postgres_fdw for the
new connection method.
Regards,
Jeff Davis
Вложения
On Sat, 1 Mar 2025 at 04:35, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2024-12-16 at 20:05 -0800, Jeff Davis wrote:
> > On Wed, 2024-10-30 at 08:08 -0700, Jeff Davis wrote:
> >
>
> Rebased v14.
>
> The approach has changed multiple times. It starte off with more in-
> core code, but in response to review feedback, has become more
> decoupled from core and more coupled to postgres_fdw.
>
> But the patch has been about the same (just rebases) since March of
> last year, and hasn't gotten feedback since. I still think it's a nice
> feature, but I'd like some feedback on the externals of the feature.
+1 for this feature.
I started having a look at the patch, here are some initial comments:
1) The hint given here does not help anymore as subscription is global object:
postgres=# drop server myserver ;
ERROR: cannot drop server myserver because other objects depend on it
DETAIL: user mapping for vignesh on server myserver depends on server myserver
subscription tap_sub depends on server myserver
HINT: Use DROP ... CASCADE to drop the dependent objects too.
postgres=# drop server myserver cascade;
NOTICE: drop cascades to 2 other objects
DETAIL: drop cascades to user mapping for vignesh on server myserver
drop cascades to subscription tap_sub
ERROR: global objects cannot be deleted by doDeletion
Should we do anything about this?
2) I felt this change is not required as TAP_TESTS is already defined:
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index adfbd2ef758..59b805656c1 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -19,6 +19,8 @@ DATA = postgres_fdw--1.0.sql
postgres_fdw--1.0--1.1.sql postgres_fdw--1.1--1.2.s
REGRESS = postgres_fdw query_cancel
TAP_TESTS = 1
+TAP_TESTS = 1
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
3) Copyright year to be updated:
diff --git a/contrib/postgres_fdw/t/010_subscription.pl
b/contrib/postgres_fdw/t/010_subscription.pl
new file mode 100644
index 00000000000..a39e8fdbba4
--- /dev/null
+++ b/contrib/postgres_fdw/t/010_subscription.pl
@@ -0,0 +1,71 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Basic logical replication test
4) I'm not sure if so many records are required, may be 10 records is enough:
+# Create some preexisting content on publisher
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab_ins AS SELECT a, a + 1 as b FROM
generate_series(1,1002) AS a");
+
5) Should subscription be server and user mapping here in the comments?
+ /* Keep us informed about subscription changes. */
+ CacheRegisterSyscacheCallback(FOREIGNSERVEROID,
+
subscription_change_cb,
+ (Datum) 0);
+ /* Keep us informed about subscription changes. */
+ CacheRegisterSyscacheCallback(USERMAPPINGOID,
+
subscription_change_cb,
+ (Datum) 0);
6) Should "initial data" be "incremental data" here:
+$node_publisher->wait_for_catchup('tap_sub');
+
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM
(SELECT f.b = l.b as match FROM tab_ins l, f_tab_ins f WHERE l.a =
f.a) WHERE match");
+is($result, qq(1050), 'check initial data was copied to subscriber');
Regards,
Vignesh
On Sat, 1 Mar 2025 at 04:35, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2024-12-16 at 20:05 -0800, Jeff Davis wrote:
> > On Wed, 2024-10-30 at 08:08 -0700, Jeff Davis wrote:
> >
>
> Rebased v14.
>
> The approach has changed multiple times. It starte off with more in-
> core code, but in response to review feedback, has become more
> decoupled from core and more coupled to postgres_fdw.
>
> But the patch has been about the same (just rebases) since March of
> last year, and hasn't gotten feedback since. I still think it's a nice
> feature, but I'd like some feedback on the externals of the feature.
Few comments:
1) \dRs+ sub does not include the server info:
postgres=# \dRs+ sub*
List of subscriptions
Name | Owner | Enabled | Publication | Binary | Streaming |
Two-phase commit | Disable on error | Origin | Password required | Run
as owner? | Failover | Synchronous commit |
Conninfo | Skip LSN
------+---------+---------+-------------+--------+-----------+------------------+------------------+--------+-------------------+---------------+----------+--------------------+-------------
-----------------------------+----------
sub | vignesh | t | {pub1} | f | parallel | d
| f | any | t | f
| f | off |
| 0/0
2) Tab completion for alter subscription also should include server:
+++ b/src/bin/psql/tab-complete.in.c
@@ -3704,7 +3704,7 @@ match_previous_words(int pattern_id,
/* CREATE SUBSCRIPTION */
else if (Matches("CREATE", "SUBSCRIPTION", MatchAny))
- COMPLETE_WITH("CONNECTION");
+ COMPLETE_WITH("SERVER", "CONNECTION");
postgres=# alter subscription sub3
ADD PUBLICATION DISABLE ENABLE REFRESH
PUBLICATION SET
CONNECTION DROP PUBLICATION OWNER TO RENAME
TO SKIP (
3) In case of binary mode, pg_dump creates subscription using server
option, but not in normal mode:
+ if (dopt->binary_upgrade && fout->remoteVersion >= 180000)
+ appendPQExpBufferStr(query, " fs.srvname AS subservername,\n"
+ "
o.remote_lsn AS suboriginremotelsn,\n"
+ " s.subenabled,\n"
+ " s.subfailover\n");
+ else
+ appendPQExpBufferStr(query, " NULL AS subservername,\n"
+ " NULL AS
suboriginremotelsn,\n"
+ " false AS
subenabled,\n"
+ " false AS
subfailover\n");
If there is some specific reason, we should at least add some comments.
Regards,
Vignesh
On Sat, 1 Mar 2025 at 04:35, Jeff Davis <pgsql@j-davis.com> wrote: > > On Mon, 2024-12-16 at 20:05 -0800, Jeff Davis wrote: > > On Wed, 2024-10-30 at 08:08 -0700, Jeff Davis wrote: > > > > Rebased v14. > > The approach has changed multiple times. It starte off with more in- > core code, but in response to review feedback, has become more > decoupled from core and more coupled to postgres_fdw. > > But the patch has been about the same (just rebases) since March of > last year, and hasn't gotten feedback since. I still think it's a nice > feature, but I'd like some feedback on the externals of the feature. > > As a note, this will require a version bump for postgres_fdw for the > new connection method. > Hi Jeff, I reviewed the patch and I have a comment: If version is >=18, the query will have 'suboriginremotelsn', 'subenabled', 'subfailover' twice. if (fout->remoteVersion >= 170000) appendPQExpBufferStr(query, - " s.subfailover\n"); + " s.subfailover,\n"); else appendPQExpBuffer(query, - " false AS subfailover\n"); + " false AS subfailover,\n"); + + if (dopt->binary_upgrade && fout->remoteVersion >= 180000) + appendPQExpBufferStr(query, " fs.srvname AS subservername,\n" + " o.remote_lsn AS suboriginremotelsn,\n" + " s.subenabled,\n" + " s.subfailover\n"); + else + appendPQExpBufferStr(query, " NULL AS subservername,\n" + " NULL AS suboriginremotelsn,\n" + " false AS subenabled,\n" + " false AS subfailover\n"); query formed is something like: "SELECT s.tableoid, s.oid, s.subname,\n s.subowner,\n s.subconninfo, s.subslotname, s.subsynccommit,\n s.subpublications,\n s.subbinary,\n s.substream,\n s.subtwophasestate,\n s.subdisableonerr,\n s.subpasswordrequired,\n s.subrunasowner,\n s.suborigin,\n NULL AS suboriginremotelsn,\n false AS subenabled,\n s.subfailover,\n NULL AS subservername,\n NULL AS suboriginremotelsn,\n false AS subenabled,\n false AS subfailover\nFROM pg_subscription s\nWHERE s.subdbid = (SELECT oid FROM pg_database\n.." is it expected? Thanks and Regards, Shlok Kyal
On Wed, 2025-04-02 at 17:58 +0530, Shlok Kyal wrote:
> I reviewed the patch and I have a comment:
Thank you and vignesh for the feedback. This patch didn't quite make it
for v18, but I will address it for the next CF.
Regards,
Jeff Davis
On Wed, 2025-04-02 at 17:58 +0530, Shlok Kyal wrote:
> I reviewed the patch and I have a comment:
>
> If version is >=18, the query will have 'suboriginremotelsn',
> 'subenabled', 'subfailover' twice.
Thank you. Fixed and rebased.
Note that this patch will require a postgres_fdw version bump.
Regards,
Jeff Davis
Вложения
On Fri, 2025-12-26 at 13:52 -0800, Jeff Davis wrote:
> On Wed, 2025-04-02 at 17:58 +0530, Shlok Kyal wrote:
> > I reviewed the patch and I have a comment:
> >
> > If version is >=18, the query will have 'suboriginremotelsn',
> > 'subenabled', 'subfailover' twice.
>
> Thank you. Fixed and rebased.
Attached new version with significant changes:
- fixed several issues (including some improper merges in the last
rebase)
- refactored to share code between postgres_fdw_connection() and
connect_pg_server()
- added docs in postgres_fdw
- added tests in core
- bumped postgres_fdw version to 1.3
Regards,
Jeff Davis
Вложения
On Sat, Jan 10, 2026 at 8:55 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2025-12-26 at 13:52 -0800, Jeff Davis wrote:
> > On Wed, 2025-04-02 at 17:58 +0530, Shlok Kyal wrote:
> > > I reviewed the patch and I have a comment:
> > >
> > > If version is >=18, the query will have 'suboriginremotelsn',
> > > 'subenabled', 'subfailover' twice.
> >
> > Thank you. Fixed and rebased.
>
> Attached new version with significant changes:
>
> - fixed several issues (including some improper merges in the last
> rebase)
> - refactored to share code between postgres_fdw_connection() and
> connect_pg_server()
> - added docs in postgres_fdw
> - added tests in core
> - bumped postgres_fdw version to 1.3
>
I've reviewed the latest patch set. I understand the motivation behind
this proposal and find it useful. Here are some comments:
@@ -5580,6 +5580,8 @@ fdw_option:
| NO HANDLER { $$ =
makeDefElem("handler", NULL, @1); }
| VALIDATOR handler_name { $$ =
makeDefElem("validator", (Node *) $2, @1); }
| NO VALIDATOR { $$ =
makeDefElem("validator", NULL, @1); }
+ | CONNECTION handler_name { $$ =
makeDefElem("connection", (Node *) $2, @1); }
+ | NO CONNECTION { $$ =
makeDefElem("connection", NULL, @1); }
;
The documentation for ALTER FOREIGN DATA WRAPPER needs to be updated.
---
The security section[1] of logical replication chapter would also need
to be updated. Currently we have:
To create a subscription, the user must have the privileges of the
pg_create_subscription role, as well as CREATE privileges on the
database.
IIUC if the user uses the SERVER clause, they must have the USAGE
privilege on the foreign server too.
---
We might want to mention in the documentation of CREATE SERVER[2] that
a foreign server's name can be used to connect publication in CREATE
SUBSCRIPTION as we have a similar description for dblink_connect():
When using the dblink module, a foreign server's name can be used as
an argument of the dblink_connect function to indicate the connection
parameters. It is necessary to have the USAGE privilege on the foreign
server to be able to use it in this way.
---
dblink_connect() function can retrieve the connection string from a
foreign server specified in the second argument, which is a very
similar use case to CREATE SUBSCRIPTION. Should we make dblink use the
new function ForeignServerConnectionString() to get the connection
string (in get_connect_string())?
---
It would be better to enhance psql's \dRs command to show the server
name specified in the subscription.
Regards,
[1] https://www.postgresql.org/docs/devel/logical-replication-security.html
[2] https://www.postgresql.org/docs/devel/sql-createserver.html
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Wed, 2026-02-04 at 13:53 +0900, Masahiko Sawada wrote:
> I've reviewed the latest patch set. I understand the motivation
> behind
> this proposal and find it useful.
Thank you, that's important feedback.
> The documentation for ALTER FOREIGN DATA WRAPPER needs to be updated.
Done.
> ---
> The security section[1] of logical replication chapter would also
> need
> to be updated.
Done.
> We might want to mention in the documentation of CREATE SERVER[2]
> that
> a foreign server's name can be used to connect publication in CREATE
> SUBSCRIPTION as we have a similar description for dblink_connect():
Done.
> ---
> dblink_connect() function can retrieve the connection string from a
> foreign server specified in the second argument, which is a very
> similar use case to CREATE SUBSCRIPTION. Should we make dblink use
> the
> new function ForeignServerConnectionString() to get the connection
> string (in get_connect_string())?
ForeignServerConnectionString() goes through the new FDW
connection_function, whereas dblink builds the string itself.
Technically, changing that could break things, but overall it seems to
make sense. I added this as a separate commit.
> ---
> It would be better to enhance psql's \dRs command to show the server
> name specified in the subscription.
Good idea, done.
Regards,
Jeff Davis
Вложения
On Thu, 2026-02-26 at 11:12 -0800, Jeff Davis wrote:
> On Wed, 2026-02-04 at 13:53 +0900, Masahiko Sawada wrote:
> > I've reviewed the latest patch set. I understand the motivation
> > behind
> > this proposal and find it useful.
>
> Thank you, that's important feedback.
Attached v18:
* rebase
* Changed ForeignServerConnectionString() to use a local variable
rather than a static. It's not very performance-sensitive, so it's OK
to create a memory context for each invocation, which will be deleted.
I'm not aware of an actual problem in the previous code, but it seemed
a bit less safe.
I plan to commit the main patch (v18-0001) soon, after rechecking some
details (like the postgres_fdw upgrade). v18-0002 could use some review
first.
Regards,
Jeff Davis
Вложения
On Mon, Mar 2, 2026 at 1:34 PM Jeff Davis <pgsql@j-davis.com> wrote: > > On Thu, 2026-02-26 at 11:12 -0800, Jeff Davis wrote: > > On Wed, 2026-02-04 at 13:53 +0900, Masahiko Sawada wrote: > > > I've reviewed the latest patch set. I understand the motivation > > > behind > > > this proposal and find it useful. > > > > Thank you, that's important feedback. > > Attached v18: > > * rebase > * Changed ForeignServerConnectionString() to use a local variable > rather than a static. It's not very performance-sensitive, so it's OK > to create a memory context for each invocation, which will be deleted. > I'm not aware of an actual problem in the previous code, but it seemed > a bit less safe. > > I plan to commit the main patch (v18-0001) soon, after rechecking some > details (like the postgres_fdw upgrade). I have a few minor comments: + Oid subserver; /* Set if connecting with server */ + Do we want to add BKI_LOOKUP(pg_foreign_data_wrapper) here? --- + +# Copyright (c) 2021-2024, PostgreSQL Global Development Group + Need to update the copyright year. The rest looks good to me. > v18-0002 could use some review > first. Thank you for making this patch. I'll look at this patch too. FYI interestingly, dblink_fdw can also be used for subscription connections like postgres_fdw. It made me think that it might be interesting to implement a FDW that supports only the libpq connection (i.e., NO HANDLER, NO VALIDATOR, and CONNECTION) as it provides the connection management capability useful for subscriptions while users can avoid any security risks in postgres_fdw that users might be concerned about. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com