Re: Handle infinite recursion in logical replication setup

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