Re: logical streaming of xacts via test_decoding is broken

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.

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?

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?

--
With Regards,
Amit Kapila.