Re: logical streaming of xacts via test_decoding is broken

On Fri, Nov 13, 2020 at 3:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Nov 12, 2020 at 3:10 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, Nov 10, 2020 at 7:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Nov 10, 2020 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > > 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
> >
>
> Few comments:
> =================
> 1.
>    -- consume DDL
>    SELECT data FROM pg_logical_slot_get_changes('isolation_slot',
> NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> -  CREATE OR REPLACE FUNCTION large_val() RETURNS TEXT LANGUAGE SQL AS
> 'select array_agg(md5(g::text))::text from generate_series(1, 80000)
> g';
> +  CREATE OR REPLACE FUNCTION large_val() RETURNS TEXT LANGUAGE SQL AS
> 'select array_agg(md5(g::text))::text from generate_series(1, 60000)
> g';
>  }
>
>
> Is there a reason for this change? I think probably here a lesser
> number of rows are sufficient to serve the purpose of the test but I
> am not sure if it is related to this patch or there is any other
> reason behind this change?

I think I changed for some experiment and got included in the patch so
reverted this.

> 2.
> +typedef struct
> +{
> + bool xact_wrote_changes;
> + bool stream_wrote_changes;
> +} TestDecodingTxnData;
> +
>
> I think here a comment explaining why we need this as a separate
> structure would be better and probably explain why we need two
> different members.

Done

> 3.
> pg_decode_commit_txn()
> {
> ..
> - if (data->skip_empty_xacts && !data->xact_wrote_changes)
> + pfree(txndata);
> + txn->output_plugin_private = false;
> +
>
> Here, don't we need to set txn->output_plugin_private as NULL as it is
> a pointer and we do explicitly test it for being NULL at other places?
> Also, change at other places where it is set as false.

Fixed

> 4.
> @@ -592,10 +610,24 @@ pg_decode_stream_start(LogicalDecodingContext *ctx,
>      ReorderBufferTXN *txn)
>  {
>   TestDecodingData *data = ctx->output_plugin_private;
> + TestDecodingTxnData *txndata = txn->output_plugin_private;
>
> - data->xact_wrote_changes = false;
> + /*
> + * If this is the first stream for the txn then allocate the txn plugin
> + * data and set the xact_wrote_changes to false.
> + */
> + if (txndata == NULL)
> + {
> + txndata =
>
> As we are explicitly testing for NULL here, isn't it better to
> explicitly initialize 'output_plugin_private' with NULL in
> ReorderBufferGetTXN?

Done

> 5.
> @@ -633,8 +666,18 @@ pg_decode_stream_abort(LogicalDecodingContext *ctx,
>      XLogRecPtr abort_lsn)
>  {
>   TestDecodingData *data = ctx->output_plugin_private;
> + ReorderBufferTXN *toptxn = txn->toptxn ? txn->toptxn : txn;
> + TestDecodingTxnData *txndata = toptxn->output_plugin_private;
> + bool xact_wrote_changes = txndata->xact_wrote_changes;
>
> - if (data->skip_empty_xacts && !data->xact_wrote_changes)
> + if (txn->toptxn == NULL)
> + {
> + Assert(txn->output_plugin_private != NULL);
> + pfree(txndata);
> + txn->output_plugin_private = false;
> + }
> +
>
> Here, if we are expecting 'output_plugin_private' to be set only for
> toptxn then the Assert and reset should happen for toptxn? I find the
> changes in this function a bit unclear so probably adding a comment
> here could help.

I have added the comments.


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