Re: [18] CREATE SUBSCRIPTION ... SERVER

On Sat, 1 Mar 2025 at 04:35, Jeff Davis <pgsql(at)j-davis(dot)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