On Mon, May 23, 2022 at 10:01 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, May 20, 2022 at 3:08 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Wed, May 18, 2022 at 10:29 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > > 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
> >
>
> It seems you forgot to remove the comments after removing the code
> corresponding to the above. See below.
> + *
> + * LOGICALREP_PROTO_LOCALONLY_VERSION_NUM is the minimum protocol version with
> + * support for sending only locally originated data from the publisher.
> + * Introduced in PG16.
> + *
> + * FIXME: LOGICALREP_PROTO_LOCALONLY_VERSION_NUM needs to be bumped to 4 in
> + * PG16.
> */
>
> Few other comments on 0001
> ========================
> 1.
> + * Return true if data has originated remotely when only_local option is
> + * enabled, false otherwise.
>
> Can we slightly change the comment to:"Return true if the data source
> (origin) is remote and user has requested only local data, false
> otherwise."
>
> 2.
> +SELECT pg_replication_origin_session_reset();
> +SELECT pg_drop_replication_slot('regression_slot_only_local');
> +SELECT pg_replication_origin_drop('regress_test_decoding:
> regression_slot_only_local');
> \ No newline at end of file
>
> At the end of the file, there should be a new line.
>
Thanks for pointing this out, for some reason vim does not show these
new lines at the end of the file, VS Code shows the last new line. I
have added it.
The attached v16 patch has the changes for the same.
Regards,
Vignesh
