Re: logical streaming of xacts via test_decoding is broken

On Tue, Nov 10, 2020 at 7:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Nov 10, 2020 at 2:25 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Tue, Nov 10, 2020 at 11:18 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Nov 10, 2020 at 10:52 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > For this case, users can use skip_empty_xacts = true and
> > > > skip_empty_streams = false. I am just asking if the user has only used
> > > > skip_empty_xacts = true and didn't use the 'skip_empty_streams'
> > > > option.
> > >
> > > Ok, thanks for the clarification.
> > >
> >
> > I have prepared a patch for the same.
> >
>
> Few comments:
> 1.
> + else if (strcmp(elem->defname, "skip-empty-streams") == 0)
> + {
> + if (elem->arg == NULL)
> + data->skip_empty_streams = true;
> + else if (!parse_bool(strVal(elem->arg), &data->skip_empty_streams))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("could not parse value \"%s\" for parameter \"%s\"",
> + strVal(elem->arg), elem->defname)));
> + if (!data->skip_empty_xacts && data->skip_empty_streams)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("the skip-empty-streams can not be true if skip-empty-xacts
> is false")));
> }
>
> You can probably add a comment as to why we are disallowing this case.
> I thought of considering 'stream-changes' parameter here because it
> won't make sense to give this parameter without it, however, it seems
> that is not necessary but maybe adding a comment
> here in that regard would be a good idea.

Should we also consider the case that if the user just passed
skip_empty_streams to true then we should automatically set
skip_empty_xacts to true?
And we will allow the 'skip_empty_streams' parameter only if
stream-changes' is true.

> 2.
> pg_decode_begin_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
> {
> TestDecodingData *data = ctx->output_plugin_private;
> + TestDecodingTxnData *txndata =
> + MemoryContextAllocZero(ctx->context, sizeof(TestDecodingTxnData));
> +
>
> Shall we free this memory at commit time for the sake of consistency,
> otherwise also it would be freed with decoding context?

Yeah, actually I have freed in the stream commit but missed here. I
will do that.

> 3. Can you please prepare a separate patch for test case changes so
> that it would be easier to verify that it fails without the patch and
> passed after the patch?

ok

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com