Re: [HACKERS] logical decoding of two-phase transactions

On Wed, Oct 7, 2020 at 4:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> All the above comments are genuine and I think it is mostly because
> the author has blindly modified the existing tests without completely
> understanding the intent of the test. I suggest we write a completely
> new regression file (decode_prepared.sql) for these and just copy
> whatever is required from prepared.sql. Once we do that we might also
> want to rename existing prepared.sql to decode_commit_prepared.sql or
> something like that. I think modifying the existing test appears to be
> quite ugly and also it is changing the intent of the existing tests.
>

Updated this. Kept the original prepared.sql untouched and added a new
regression file called two_phase.sql
which is specific to test cases with the new flag two-phase-commit.

> >
> > QUESTION
> > Line 120 - I did not really understand the SQL checking the pg_class.
> > I expected this would be checking table 'test_prepared1' instead. Can
> > you explain it?
> > SELECT 'pg_class' AS relation, locktype, mode
> > FROM pg_locks
> > WHERE locktype = 'relation'
> > AND relation = 'pg_class'::regclass;
> > relation | locktype | mode
> > ----------+----------+------
> > (0 rows)
> >
> > ;
>
> Yes, I also think your expectation is correct and this should be on
> 'test_prepared_1'.

Updated

>
> >
> > QUESTION
> > Line 139 - SET statement_timeout = '1s'; is 1 seconds short enough
> > here for this test, or might it be that these statements would be
> > completed in less than one seconds anyhow?
> >
> > ;
>
> Good question. I think we have to mention the reason why logical
> decoding is not blocked while it needs to acquire a shared lock on the
> table and the previous commands already held an exclusive lock on the
> table. I am not sure if I am missing something but like you, it is not
> clear to me as well what this test intends to do, so surely more
> commentary is required.

Updated.
>
>
> >
> > QUESTION
> > Line 163 - How is this testing a SAVEPOINT? Or is it only to check
> > that the SAVEPOINT command is not part of the replicated changes?
> >
> > ;
>
> It is more of testing that subtransactions will not create a problem
> while decoding.

Updated with a testcase that actually does a rollback to a savepoint

>
> >
> > COMMENT
> > Line 175 - Missing underscore in comment. Code requires also underscore:
> > "nodecode" --> "_nodecode"
> >
>
> makes sense.

Updated.

>
> > ==========
> > Patch V6-0001, File: contrib/test_decoding/test_decoding.c
> > ==========
> >
> > COMMENT
> > Line 43
> > @@ -36,6 +40,7 @@ typedef struct
> > bool skip_empty_xacts;
> > bool xact_wrote_changes;
> > bool only_local;
> > + TransactionId check_xid; /* track abort of this txid */
> > } TestDecodingData;
> >
> > The "check_xid" seems a meaningless name. Check what?
> > IIUC maybe should be something like "check_xid_aborted"

Updated.

> >
> > ;
> >
> > COMMENT
> > Line 105
> > @ -88,6 +93,19 @@ static void
> > pg_decode_stream_truncate(LogicalDecodingContext *ctx,
> > ReorderBufferTXN *txn,
> > int nrelations, Relation relations[],
> > ReorderBufferChange *change);
> > +static bool pg_decode_filter_prepare(LogicalDecodingContext *ctx,
> > + ReorderBufferTXN *txn,
> >
> > Remove extra blank line after these functions
> >
> > ;
>
> The above two sounds reasonable suggestions.

Updated.

> >
> > COMMENT
> > Line 149
> > @@ -116,6 +134,11 @@ _PG_output_plugin_init(OutputPluginCallbacks *cb)
> > cb->stream_change_cb = pg_decode_stream_change;
> > cb->stream_message_cb = pg_decode_stream_message;
> > cb->stream_truncate_cb = pg_decode_stream_truncate;
> > + cb->filter_prepare_cb = pg_decode_filter_prepare;
> > + cb->prepare_cb = pg_decode_prepare_txn;
> > + cb->commit_prepared_cb = pg_decode_commit_prepared_txn;
> > + cb->abort_prepared_cb = pg_decode_abort_prepared_txn;
> > +
> > }
> >
> > There is a confusing mix of terminology where sometimes things are
> > referred as ROLLBACK/rollback and other times apparently the same
> > operation is referred as ABORT/abort. I do not know the root cause of
> > this mixture. IIUC maybe the internal functions and protocol generally
> > use the term "abort", whereas the SQL syntax is "ROLLBACK"... but
> > where those two terms collide in the middle it gets quite confusing.
> >
> > At least I thought the names of the "callbacks" which get exposed to
> > the user (e.g. in the help) might be better if they would match the
> > SQL.
> > "abort_prepared_cb" --> "rollback_prepared_db"
> >
>
> This suggestion sounds reasonable. I think it is to entertain the case
> where due to error we need to rollback the transaction. I think it is
> better if use 'rollback' terminology in the exposed functions. We
> already have a function with the name stream_abort_cb in the code
> which we also might want to rename but that is a separate thing and we
> can deal it with a separate patch.

Changed the call back names from abort_prepared to rollback_prepapred
and stream_abort_prepared to stream_rollback_prepared.

>
> > There are similar review comments like this below where the
> > alternating terms caused me some confusion.
> >
> > ~
> >
> > Also, Remove the extra blank line before the end of the function.
> >
> > ;
> >
> > COMMENT
> > Line 267
> > @ -227,6 +252,42 @@ pg_decode_startup(LogicalDecodingContext *ctx,
> > OutputPluginOptions *opt,
> > errmsg("could not parse value \"%s\" for parameter \"%s\"",
> > strVal(elem->arg), elem->defname)));
> > }
> > + else if (strcmp(elem->defname, "two-phase-commit") == 0)
> > + {
> > + if (elem->arg == NULL)
> > + continue;
> >
> > IMO the "check-xid" code might be better rearranged so the NULL check
> > is first instead of if/else.
> > e.g.
> > if (elem->arg == NULL)
> > ereport(FATAL,
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > errmsg("check-xid needs an input value")));
> > ~
> >
> > Also, is it really supposed to be FATAL instead or ERROR. That is not
> > the same as the other surrounding code.
> >
> > ;
>
> +1.

Updated.

>
> >
> > COMMENT
> > Line 296
> > if (data->check_xid <= 0)
> > ereport(ERROR,
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > errmsg("Specify positive value for parameter \"%s\","
> > " you specified \"%s\"",
> > elem->defname, strVal(elem->arg))));
> >
> > The code checking for <= 0 seems over-complicated. Because conversion
> > was using strtoul() I fail to see how this can ever be < 0. Wouldn't
> > it be easier to simply test the result of the strtoul() function?
> >
> > BEFORE: if (errno == EINVAL || errno == ERANGE)
> > AFTER: if (data->check_xid == 0)
> >
>
> Better to use TransactionIdIsValid(data->check_xid) here.

Updated.

>
> > ~
> >
> > Also, should this be FATAL? Everything else similar is ERROR.
> >
> > ;
>
> It should be an error.

Updated

>
> >
> > COMMENT
> > (general)
> > I don't recall seeing any of these decoding options (e.g.
> > "two-phase-commit", "check-xid") documented anywhere.
> > So how can a user even know these options exist so they can use them?
> > Perhaps options should be described on this page?
> > https://www.postgresql.org/docs/13/functions-admin.html#FUNCTIONS-REPLICATION
> >
> > ;
>
> I think we should do what we are doing for other options, if they are
> not documented then why to document this one separately. I guess we
> can make a case to document all the existing options and write a
> separate patch for that.

I didnt see any of the test_decoding options updated in the
documentation as these seem specific for the test_decoder used in
testing.
https://www.postgresql.org/docs/13/test-decoding.html

>
> >
> > COMMENT
> > (general)
> > "check-xid" is a meaningless option name. Maybe something like
> > "checked-xid-aborted" is more useful?
> > Suggest changing the member, the option, and the error messages to
> > match some better name.

Updated.

> >
> > ;
> >
> > COMMENT
> > Line 314
> > @@ -238,6 +299,7 @@ pg_decode_startup(LogicalDecodingContext *ctx,
> > OutputPluginOptions *opt,
> > }
> >
> > ctx->streaming &= enable_streaming;
> > + ctx->enable_twophase &= enable_2pc;
> > }
> >
> > The "ctx->enable_twophase" is inconsistent naming with the
> > "ctx->streaming" member.
> > "enable_twophase" --> "twophase"
> >
> > ;
>
> +1.

Updated

>
> >
> > COMMENT
> > Line 374
> > @@ -297,6 +359,94 @@ pg_decode_commit_txn(LogicalDecodingContext *ctx,
> > ReorderBufferTXN *txn,
> > OutputPluginWrite(ctx, true);
> > }
> >
> > +
> > +/*
> > + * Filter out two-phase transactions.
> > + *
> > + * Each plugin can implement its own filtering logic. Here
> > + * we demonstrate a simple logic by checking the GID. If the
> > + * GID contains the "_nodecode" substring, then we filter
> > + * it out.
> > + */
> > +static bool
> > +pg_decode_filter_prepare(LogicalDecodingContext *ctx, ReorderBufferTXN *txn,
> >
> > Remove the extra preceding blank line.
> >

Updated.

> > ~
> >
> > I did not find anything in the help about "_nodecode". Should it be
> > there or is this deliberately not documented feature?
> >
> > ;
>
> I guess we can document it along with filter_prepare API, if not
> already documented.
>

Again , this seems to be specific to test_decoder and an example of a
way to create a filter_prepare.

> >
> > QUESTION
> > Line 440
> > +pg_decode_abort_prepared_txn(LogicalDecodingContext *ctx,
> > ReorderBufferTXN *txn,
> >
> > Is this a wrong comment
> > "ABORT PREPARED" --> "ROLLBACK PREPARED" ??
> >
> > ;
> >
> > COMMENT
> > Line 620
> > @@ -455,6 +605,22 @@ pg_decode_change(LogicalDecodingContext *ctx,
> > ReorderBufferTXN *txn,
> > }
> > data->xact_wrote_changes = true;
> >
> > + /* if check_xid is specified */
> > + if (TransactionIdIsValid(data->check_xid))
> > + {
> > + elog(LOG, "waiting for %u to abort", data->check_xid);
> > + while (TransactionIdIsInProgress(dat
> >
> > The check_xid seems a meaningless name, and the comment "/* if
> > check_xid is specified */" was not helpful either.
> > IIUC purpose of this is to check that the nominated xid always is rolled back.
> > So the appropriate name may be more like "check-xid-aborted".
> >
> > ;
>
> Yeah, this part deserves better comments.

Updated.

Other than these first batch of review comments from Peter Smith, I've
also updated new functions in decode.c for DecodeCommitPrepared
and DecodeAbortPrepared as agreed in a previous review comment by
Amit and Dilip.
I've also incorporated Dilip's comment on acquiring SHARED lock rather
than EXCLUSIVE lock while looking for transaction matching Gid.
Since Peter's comments are many, I'll be sending patch updates in
parts addressing his comments.

regards,
Ajin Cherian
Fujitsu Australia