From: | vignesh C <vignesh21(at)gmail(dot)com> |
---|---|
To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Handle infinite recursion in logical replication setup |
Date: | 2022-04-16 14:18:22 |
Message-ID: | CALDaNm2Fe=g4Tx-DhzwD6NU0VRAfaPedXwWO01maNU7_OfS8fw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v9-0002-Support-force-option-for-copy_data-check-and-thro.patch | text/x-patch | 42.8 KB |
v9-0001-Skip-replication-of-non-local-data.patch | text/x-patch | 53.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Justin Pryzby | 2022-04-16 14:44:54 | Re: convert libpq uri-regress tests to tap test |
Previous Message | Nikolay Shaplov | 2022-04-16 14:15:34 | Re: [PATCH] New [relation] option engine |