On Tue, Nov 10, 2020 at 7:20 PM Amit Kapila <[email protected]> wrote: > > On Tue, Nov 10, 2020 at 2:25 PM Dilip Kumar <[email protected]> wrote: > > > > On Tue, Nov 10, 2020 at 11:18 AM Dilip Kumar <[email protected]> wrote: > > > > > > On Tue, Nov 10, 2020 at 10:52 AM Amit Kapila <[email protected]> > > > 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.
As per our latest discussion, I have removed the extra input parameter
so this comment is not needed now.
> 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?
Done
> 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?
Done
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
v2-0001-Bug-fix-skip-empty-xacts-in-streaming-mode.patch
Description: Binary data
v2-0002-Test-case-to-test-the-interleaved-empty-transacti.patch
Description: Binary data
