Re: Bug in logical decoding of in-progress transactions

On Thu, Sep 10, 2020 at 2:48 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Thu, Sep 10, 2020 at 12:00 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> wrote:
> >
> > On Thu, Sep 10, 2020 at 11:53 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> wrote:
> >>
> >>> >
> >>> > I have written a test case to reproduce the same.
> >
>
> Can we write an isolation test for this scenario? See some similar
> tests in contrib/test_decoding/specs. If that is possible then we can
> probably remove the test which failed and instead write an isolation
> test involving three transactions as shown by you. Also, please
> prepare two separate patches (one for test and other for code) if you
> are able to convert existing test to an isolation test as that will
> make it easier to test the fix.
>

I have written a test in isolation test. IMHO, we should not try to merge
stream.sql to this isolation test mainly for two reasons a) this isolation
test is very specific that while we are trying to stream we must have the
incomplete changes so if we try to put more operation like
message/truncate/abort then it will become unpredictable. Currently, I
have kept it with just one big tuple so it is a guarantee that whenever the
the logical_decoding_work_mem exceed then it will have the partial
changes. b) we can add another operation in the transaction and cover the
stream changes but then those are not very specific to the isolation test.
So I feel it is better to put only the specific scenario in the isolation
test.

>
> >
> > I have removed some comments which are not valid after this patch.
> >
>
> Few comments:
> =============
> 1. We need to set xact_wrote_changes in pg_decode_stream_truncate() as
> well along with the APIs in which you have set it.
> 2.
> +static void
> +pg_output_stream_start(LogicalDecodingContext *ctx, TestDecodingData
> *data, ReorderBufferTXN *txn, bool last_write)
> +{
> OutputPluginPrepareWrite(ctx, true);
> if (data->include_xids)
> appendStringInfo(ctx->out, "opening a streamed block for transaction
> TXN %u", txn->xid);
> @@ -601,16 +610,15 @@ pg_decode_stream_start(LogicalDecodingContext *ctx,
> OutputPluginWrite(ctx, true);
>
> In this API, we need to use 'last_write' in OutputPluginPrepareWrite()
> and OutputPluginWrite().
>
> The attached patch fixes both these comments.
>

Okay, there is some change in stream.out so I have included that in the
first patch.

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