Re: Handle infinite recursion in logical replication setup

On Thu, Apr 14, 2022 at 1:53 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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.

Modified

> ~~~
>
> 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;
>

Modified

> ~~~
>
> 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
>

Modified

> ~~~
>
> 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.
>

Modified

> ~~~
>
> 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;
>

Modified

> ~~~
>
> 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.
>

Modified

> 6b. Change the column name to sublocalonly.
>

Modified

> ~~~
>
> 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
>

Modified

> ~~~
>
> 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.
>

I have removed this comment and added a FIXME. I will add it in once
version change is done to avoid confusion.

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

Modified

> 7c. Change the column name to sublocalonly.
>

Modified

> ~~~
>
> 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
>

Modified

> ~~~
>
> 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.
>

Modified

> ~~~
>
> 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"?

Added the test and changed the file.

Thanks for the comments, attached v9 patch has the changes for the same.

Regards,
Vignesh