Re: logical streaming of xacts via test_decoding is broken

On Thu, Nov 12, 2020 at 3:10 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> 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:
>
> > 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?

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.

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.

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?

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.

--
With Regards,
Amit Kapila.