Обсуждение: 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