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
