Обсуждение: 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
On Tue, Mar 3, 2026 at 3:04 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> Attached v18:
>
I haven't checked the details but while glancing at the patch, I have
few observations:
1.
@@ -92,9 +92,11 @@
CATALOG(pg_subscription,6100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW
* exceeded max_retention_duration, when
* defined */
+ Oid subserver; /* Set if connecting with server */
+
#ifdef CATALOG_VARLEN /* variable-length fields start here */
/* Connection string to the publisher */
- text subconninfo BKI_FORCE_NOT_NULL;
+ text subconninfo; /* Set if connecting with connection string */
We revoke view rights on subconninfo from the public. See below [A] in
system_views.sql. Do we want to do the same for subserver or is it
okay for users to see it? I think the following comment and some place
in docs needs to be updated.
[A]
-- All columns of pg_subscription except subconninfo are publicly readable.
REVOKE ALL ON pg_subscription FROM public;
GRANT SELECT (oid, subdbid, subskiplsn, subname, subowner, subenabled,
subbinary, substream, subtwophasestate, subdisableonerr,
subpasswordrequired, subrunasowner, subfailover,
subretaindeadtuples, submaxretention, subretentionactive,
subslotname, subsynccommit, subpublications, suborigin)
ON pg_subscription TO public;
2. We may want to update the following text in pg_dump docs about the
new way of connecting to hosts. See [B] (When dumping logical
replication subscriptions, pg_dump will generate CREATE SUBSCRIPTION
commands that use the connect = false option, so that restoring the
subscription does not make remote connections for creating a
replication slot or for initial table copy. That way, the dump can be
restored without requiring network access to the remote servers. It is
then up to the user to reactivate the subscriptions in a suitable way.
If the involved hosts have changed, the connection information might
have to be changed.)
[B] - https://www.postgresql.org/docs/devel/app-pgdump.html
--
With Regards,
Amit Kapila.
On Thu, 2026-03-05 at 09:21 +0530, Amit Kapila wrote: > We revoke view rights on subconninfo from the public. See below [A] > in > system_views.sql. Do we want to do the same for subserver or is it > okay for users to see it? I can't think of a reason that the server name should be secret, but let me know if you think so. > I think the following comment and some place > in docs needs to be updated. > [A] > -- All columns of pg_subscription except subconninfo are publicly > readable. > REVOKE ALL ON pg_subscription FROM public; > GRANT SELECT (oid, subdbid, subskiplsn, subname, subowner, Good catch! Thank you. > 2. We may want to update the following text in pg_dump docs about the > new way of connecting to hosts. See [B] (When dumping logical > replication subscriptions, pg_dump will generate CREATE SUBSCRIPTION > commands that use the connect = false option, so that restoring the > subscription does not make remote connections for creating a > replication slot or for initial table copy. That way, the dump can be > restored without requiring network access to the remote servers. It > is > then up to the user to reactivate the subscriptions in a suitable > way. > If the involved hosts have changed, the connection information might > have to be changed.) > > [B] - https://www.postgresql.org/docs/devel/app-pgdump.html > I think the above comment is still correct -- it would be a bit easier to deal with servers rather than raw connection strings, but the comment already says "...might have to be changed" which is just a reminder to look. Attached a new patch that also addressed the review comments from here: https://www.postgresql.org/message-id/CAD21AoCpr8UfmOngKZ92hZ-78cr2H+3Tbs9QLveYoWnWBfxrxw@mail.gmail.com Additionally, I ran into a problem that's worth highlighting: DROP SERVER ... CASCADE was broken, because the subscription is dependent on it but that's in a global catalog, which is not handled by doDeletion(). The subscription is conceptually a per-database object, but it's in a shared catalog with a subdbid field. I solved that problem by adding a guard to findDependentObjects() to check for the referenced object belonging to a shared catalog, and if so it just throws an error (so CASCADE is not supported for servers used in subscriptions). That's a simple but not a very satisfying solution, so let me know if you see a problem with that. Regards, Jeff Davis
Вложения
On Tue, 2026-03-03 at 10:19 -0800, Masahiko Sawada wrote:
> It made me think that it might be
> interesting to implement a FDW that supports only the libpq
> connection
> (i.e., NO HANDLER, NO VALIDATOR, and CONNECTION) as it provides the
> connection management capability useful for subscriptions while users
> can avoid any security risks in postgres_fdw that users might be
> concerned about.
We discussed some similar ideas earlier in the thread. My initial
proposal used special "FOR CONNECTION ONLY" syntax and had special
cases for the FDW.
Tom didn't like the special syntax and catalog work[1], and suggested a
dummy FDW. Ashutosh expressed concern about the scope of a dummy
FDW[2], and suggested that we just rely on postgres_fdw, which is the
current patch. I agree and think it's the right direction.
A new special-purpose FDW might be useful, but I don't understand the
exact use case yet, so I think it's better to leave it to the user to
implement a new FDW that does what they want. (Perhaps special safety
rules, etc.)
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/172273.1693403385%40sss.pgh.pa.us
[2]
https://www.postgresql.org/message-id/CAExHW5uCzS-VeSYQHTHxFSdQik-f_O892xmzrzm2fuO+ro+otA@mail.gmail.com
On Thu, Mar 5, 2026 at 2:23 PM Jeff Davis <pgsql@j-davis.com> wrote: > > On Thu, 2026-03-05 at 09:21 +0530, Amit Kapila wrote: > > We revoke view rights on subconninfo from the public. See below [A] > > in > > system_views.sql. Do we want to do the same for subserver or is it > > okay for users to see it? > > I can't think of a reason that the server name should be secret, but > let me know if you think so. > > > I think the following comment and some place > > in docs needs to be updated. > > [A] > > -- All columns of pg_subscription except subconninfo are publicly > > readable. > > REVOKE ALL ON pg_subscription FROM public; > > GRANT SELECT (oid, subdbid, subskiplsn, subname, subowner, > > Good catch! Thank you. > > > 2. We may want to update the following text in pg_dump docs about the > > new way of connecting to hosts. See [B] (When dumping logical > > replication subscriptions, pg_dump will generate CREATE SUBSCRIPTION > > commands that use the connect = false option, so that restoring the > > subscription does not make remote connections for creating a > > replication slot or for initial table copy. That way, the dump can be > > restored without requiring network access to the remote servers. It > > is > > then up to the user to reactivate the subscriptions in a suitable > > way. > > If the involved hosts have changed, the connection information might > > have to be changed.) > > > > [B] - https://www.postgresql.org/docs/devel/app-pgdump.html > > > > I think the above comment is still correct -- it would be a bit easier > to deal with servers rather than raw connection strings, but the > comment already says "...might have to be changed" which is just a > reminder to look. > > > Attached a new patch that also addressed the review comments from here: > > https://www.postgresql.org/message-id/CAD21AoCpr8UfmOngKZ92hZ-78cr2H+3Tbs9QLveYoWnWBfxrxw@mail.gmail.com > > Additionally, I ran into a problem that's worth highlighting: > > DROP SERVER ... CASCADE was broken, because the subscription is > dependent on it but that's in a global catalog, which is not handled by > doDeletion(). The subscription is conceptually a per-database object, > but it's in a shared catalog with a subdbid field. I solved that > problem by adding a guard to findDependentObjects() to check for the > referenced object belonging to a shared catalog, and if so it just > throws an error (so CASCADE is not supported for servers used in > subscriptions). That's a simple but not a very satisfying solution, so > let me know if you see a problem with that. I shared the awkwardness, but don't have any better ideas. However, it does raise a question as to why do we need an FDW to be database specific or for that matter a SERVER database specific. That might be because it requires an extension which is database specific. Probably we should support extensions which are database agnostic. However that's way beyond the scope of this patch. Other way around why do we need subscriptions to be shared objects? Again probably beyond the scope of this patch. I also see some code duplicated across Create and Alter subscription code paths. Even without this patch the code was duplicated, but with this patch the amount of duplication has increased. Can we deduplicate some of the code? I don't think we need a separate ForeignServerName function. In AlterSubscription() we already have ForeignSever object which has server name in it. Other two callers invoke ForeignServerConnectionString() which in turn fetches ForeignServer object. Those callers instead may fetch ForeignServer object themselves and pass it to ForeignServerConnectionString() and use it in the error message. The patch has changes to pg_dump.c but there is no corresponding test. But I don't think we need a separate test. If the objects created in regress/*.sql tests are not dropped, 002_pg_upgrade.pl would test dump/restore of subscriptions with server. I think we need tests for testing changes in connection when ALTER SUBSCRIPTION ... SERVER is executed and also those for switching between SERVER and CONNECTION. -- Best Wishes, Ashutosh Bapat
On Thu, Mar 5, 2026 at 2:23 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> Additionally, I ran into a problem that's worth highlighting:
>
> DROP SERVER ... CASCADE was broken, because the subscription is
> dependent on it but that's in a global catalog, which is not handled by
> doDeletion(). The subscription is conceptually a per-database object,
> but it's in a shared catalog with a subdbid field. I solved that
> problem by adding a guard to findDependentObjects() to check for the
> referenced object belonging to a shared catalog, and if so it just
> throws an error (so CASCADE is not supported for servers used in
> subscriptions). That's a simple but not a very satisfying solution, so
> let me know if you see a problem with that.
>
I also can't think of any straight-forward solution for it. I've not
thought in detail but can a new type of dependency be required to
solve this problem? I am not aware if we are doing something similar
in any other CASCADE operation, so even if we want to go with giving
ERROR for this case, it may be better to get somewhat wider acceptance
for the same unless few other people respond here and consider this as
an acceptable solution.
Few other minor comments:
======================
1.
+# Replicate the changes without columns
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_no_col default VALUES");
I don't see a subscriber-side table or verification code to verify the
above test.
2.
+ Oid subserver BKI_LOOKUP_OPT(pg_foreign_server); /* If connection uses
+ * server */
+
Isn't it better to keep this along with other oids in the beginning of
the catalog, say after subowner? It will also avoid padding before
subserver field.
--
With Regards,
Amit Kapila.
On Fri, Mar 6, 2026 at 9:49 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > > On Thu, Mar 5, 2026 at 2:23 PM Jeff Davis <pgsql@j-davis.com> wrote: > > > > On Thu, 2026-03-05 at 09:21 +0530, Amit Kapila wrote: > > > > Additionally, I ran into a problem that's worth highlighting: > > > > DROP SERVER ... CASCADE was broken, because the subscription is > > dependent on it but that's in a global catalog, which is not handled by > > doDeletion(). The subscription is conceptually a per-database object, > > but it's in a shared catalog with a subdbid field. I solved that > > problem by adding a guard to findDependentObjects() to check for the > > referenced object belonging to a shared catalog, and if so it just > > throws an error (so CASCADE is not supported for servers used in > > subscriptions). That's a simple but not a very satisfying solution, so > > let me know if you see a problem with that. > > I shared the awkwardness, but don't have any better ideas. However, it > does raise a question as to why do we need an FDW to be database > specific or for that matter a SERVER database specific. That might be > because it requires an extension which is database specific. Probably > we should support extensions which are database agnostic. However > that's way beyond the scope of this patch. Other way around why do we > need subscriptions to be shared objects? > It is because the launcher process needs to traverse all subscriptions to start workers. -- With Regards, Amit Kapila.
On Sat, Mar 7, 2026 at 12:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Mar 5, 2026 at 2:23 PM Jeff Davis <pgsql@j-davis.com> wrote:
> >
>
> Few other minor comments:
> ======================
> 1.
> +# Replicate the changes without columns
> +$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
> +$node_publisher->safe_psql('postgres',
> + "INSERT INTO tab_no_col default VALUES");
>
> I don't see a subscriber-side table or verification code to verify the
> above test.
>
I see that the committed version (8185bb5347) has this part of the
test, isn't that test incomplete, if not, tell me what am I missing?
It seems I have sent this message after you have committed the last
version.
> 2.
> + Oid subserver BKI_LOOKUP_OPT(pg_foreign_server); /* If connection uses
> + * server */
> +
>
> Isn't it better to keep this along with other oids in the beginning of
> the catalog, say after subowner? It will also avoid padding before
> subserver field.
>
We can probably consider this one as well though there is no
correctness issue as such.
--
With Regards,
Amit Kapila.