Horton

Re: Handle infinite recursion in logical replication setup

vignesh C <vignesh21@gmail.com>

View thread View on postgr.es
From: vignesh C <vignesh21@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-05-20T09:37:47Z
Lists: pgsql-hackers

Attachments

On Wed, May 18, 2022 at 10:29 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, May 4, 2022 at 12:17 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v13 patch has the changes for the same.
> >
>
> Few comments on v13-0001
> ======================
> 1.
> + *
> + * FIXME: LOGICALREP_PROTO_LOCALONLY_VERSION_NUM needs to be bumped to 4 in
> + * PG16.
> ...
> @@ -477,6 +489,12 @@ pgoutput_startup(LogicalDecodingContext *ctx,
> OutputPluginOptions *opt,
>   else
>   ctx->twophase_opt_given = true;
>
> + if (data->local_only && data->protocol_version <
> LOGICALREP_PROTO_LOCALONLY_VERSION_NUM)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("requested proto_version=%d does not support local_only, need
> %d or higher",
> + data->protocol_version, LOGICALREP_PROTO_LOCALONLY_VERSION_NUM)));
>
> What is the need to change the protocol version for this parameter? As
> per my understanding, we only need a different protocol version when
> we are sending some new message or some additional information in an
> existing message as we have done for the streaming/two_phase options
> which doesn't seem to be the case here.

Modified

> 2.
> @@ -29,6 +29,7 @@ typedef struct PGOutputData
>   bool streaming;
>   bool messages;
>   bool two_phase;
> + bool local_only;
>  } PGOutputData;
>
> It seems we already have a similar option named 'only_local' in
> TestDecodingData which has the exactly same functionality. So, isn't
> it better to name this as 'only_local' instead of 'local_only'? Also,
> let's add a test case for 'only_local' option of test_decoding.

Modified and added test for only_local option of test_decoding.

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

Regards,
Vignesh

Commits

  1. Improved CREATE SUBSCRIPTION message for clarity

  2. Fix the test case introduced by commit 8756930190.

  3. Raise a warning if there is a possibility of data from multiple origins.

  4. Add wait_for_subscription_sync for TAP tests.

  5. Bump catversion for commit d8cd0c6c95c0120168df93aae095df4e0682a08a.

  6. Allow users to skip logical replication of data having origin.

  7. Improve two comments related to a boolean DefElem's value

  8. doc: Fix man page whitespace issues