Re: Handle infinite recursion in logical replication setup - Mailing list pgsql-hackers

Here are my review comments for v8-0001.

======

1. Commit message

CREATE SUBSCRIPTION sub1 CONNECTION 'dbname =postgres port=9999'
PUBLICATION pub1 with (local_only = true);

The spaces in the CONNECTION string are a bit strange.

~~~

2. src/backend/catalog/pg_subscription.

@@ -71,6 +71,7 @@ GetSubscription(Oid subid, bool missing_ok)
  sub->stream = subform->substream;
  sub->twophasestate = subform->subtwophasestate;
  sub->disableonerr = subform->subdisableonerr;
+ sub->local_only = subform->sublocal;

Maybe call it subform->sublocalonly;

~~~

3. src/backend/catalog/system_views.sql

@@ -1290,8 +1290,8 @@ REVOKE ALL ON pg_replication_origin_status FROM public;
 -- 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, subslotname,
-              subsynccommit, subpublications)
+              subbinary, substream, subtwophasestate, subdisableonerr,
+              sublocal, subslotname, subsynccommit, subpublications)

Maybe call the column sublocalonly

~~~

4. .../libpqwalreceiver/libpqwalreceiver.c

@@ -453,6 +453,10 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
  PQserverVersion(conn->streamConn) >= 150000)
  appendStringInfoString(&cmd, ", two_phase 'on'");

+ if (options->proto.logical.local_only &&
+ PQserverVersion(conn->streamConn) >= 150000)
+ appendStringInfoString(&cmd, ", local_only 'on'");

Add a FIXME comment here as a reminder that this condition needs to
change for PG16.

~~~

5. src/bin/pg_dump/pg_dump.c

@@ -4361,6 +4361,7 @@ getSubscriptions(Archive *fout)
  int i_subsynccommit;
  int i_subpublications;
  int i_subbinary;
+ int i_sublocal;
  int i,
  ntups;

Maybe call this member i_sublocalonly;

~~~

6. src/bin/pg_dump/pg_dump.c
+ if (fout->remoteVersion >= 150000)
+ appendPQExpBufferStr(query, " s.sublocal\n");
+ else
+ appendPQExpBufferStr(query, " false AS sublocal\n");
+

6a. Add a FIXME comment as a reminder that this condition needs to
change for PG16.

6b. Change the column name to sublocalonly.

~~~

7. src/bin/pg_dump/pg_dump.h

@@ -661,6 +661,7 @@ typedef struct _SubscriptionInfo
  char    *subdisableonerr;
  char    *subsynccommit;
  char    *subpublications;
+ char    *sublocal;
 } SubscriptionInfo;

Change the member name to sublocalonly

~~~

8. src/bin/psql/describe.c

@@ -6241,6 +6241,12 @@ describeSubscriptions(const char *pattern, bool verbose)
    gettext_noop("Two phase commit"),
    gettext_noop("Disable on error"));

+ /* local_only is supported only in v15 and higher */
+ if (pset.sversion >= 150000)
+ appendPQExpBuffer(&buf,
+   ", sublocal AS \"%s\"\n",
+   gettext_noop("Local only"));

7a. The comment is wrong to mention v15.

7b. Add a FIXME comment as a reminder that this condition needs to
change for PG16.

7c. Change the column name to sublocalonly.

~~~

9. src/include/catalog/pg_subscription.h

@@ -70,6 +70,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW

  bool substream; /* Stream in-progress transactions. */

+ bool sublocal; /* skip copying of remote origin data */

Change the member name to sublocalonly

~~~

10. src/include/replication/logicalproto.h

@@ -32,12 +32,17 @@
  *
  * LOGICALREP_PROTO_TWOPHASE_VERSION_NUM is the minimum protocol version with
  * support for two-phase commit decoding (at prepare time). Introduced in PG15.
+ *
+ * LOGICALREP_PROTO_LOCALONLY_VERSION_NUM is the minimum protocol version with
+ * support for sending only locally originated data from the publisher.
+ * Introduced in PG16.

Add a FIXME comment here as a reminder that the proto version number
needs to be bumped to 4 in PG16.

~~~

11. src/test/subscription/t/032_circular.pl

Perhaps there should be another test using a third "Node_C" which
publishes some data to Node_B. Then you can ensure that by using
local_only (when Node_A is subscribing to Node_A) that nothing from
the Node_C can find its way onto Node_A. But then the test name
"circular" is a bit misleading. Maybe it should be "032_localonly"?

------
Kind Regards,
Peter Smith.
Fujitsu Australia