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

On Tue, Oct 6, 2020 at 10:23 AM Peter(dot)B(dot)Smith(at)fujitsu(dot)com
<Peter(dot)B(dot)Smith(at)fujitsu(dot)com> wrote:
>
> ==========
> Patch V6-0001, File: src/include/replication/reorderbuffer.h
> ==========
>
> QUESTION
> Line 116
> @@ -162,9 +163,13 @@ typedef struct ReorderBufferChange
> #define RBTXN_HAS_CATALOG_CHANGES 0x0001
> #define RBTXN_IS_SUBXACT 0x0002
> #define RBTXN_IS_SERIALIZED 0x0004
> -#define RBTXN_IS_STREAMED 0x0008
> -#define RBTXN_HAS_TOAST_INSERT 0x0010
> -#define RBTXN_HAS_SPEC_INSERT 0x0020
> +#define RBTXN_PREPARE 0x0008
> +#define RBTXN_COMMIT_PREPARED 0x0010
> +#define RBTXN_ROLLBACK_PREPARED 0x0020
> +#define RBTXN_COMMIT 0x0040
> +#define RBTXN_IS_STREAMED 0x0080
> +#define RBTXN_HAS_TOAST_INSERT 0x0100
> +#define RBTXN_HAS_SPEC_INSERT 0x0200
>
> I was wondering why when adding new flags, some of the existing flag
> masks were also altered.
> I am assuming this is ok because they are never persisted but are only
> used in the protocol (??)
>
> ;

This is bad even though there is no direct problem. I don't think we
need to change the existing ones, we can add the new ones at the end
with the number starting where the last one ends.

>
>
> COMMENT
> Line 133
>
> Assert(strlen(txn->gid) > 0);
> Shouldn't that assertion also check txn->gid is not NULL (to prevent
> NPE in case gid was NULL)
>
> ;

I think that would be better and a stronger Assertion than the current one.

>
> COMMENT
> Line 177
> +logicalrep_read_prepare(StringInfo in, LogicalRepPrepareData * prepare_data)
>
> prepare_data->prepare_type = flags;
> This code may be OK but it does seem a bit of an abuse of the flags.
>
> e.g. Are they flags or are the really enum values?
> e.g. And if they are effectively enums (it appears they are) then
> seemed inconsistent that |= was used when they were previously
> assigned.
>
> ;

I don't understand this point. As far as I can see at the time of
write (logicalrep_write_prepare()), the patch has used |=, and at the
time of reading (logicalrep_read_prepare()) it has used assignment
which seems correct from the code perspective. Do you have a better
proposal?

>
>
>
> COMMENT
> Line 408
> +pgoutput_commit_prepared_txn(LogicalDecodingContext *ctx,
> ReorderBufferTXN *txn,
>
> Since all this function is identical to pg_output_prepare it might be
> better to either
> 1. just leave this as a wrapper to delegate to that function
> 2. remove this one entirely and assign the callback to the common
> pgoutput_prepare_txn
>
> ;

I think this is because as of now the patch uses the same function and
protocol message to send both Prepare and Commit/Rollback Prepare
which I am not sure is the right thing. I suggest keeping that code as
it is for now. Let's first try to figure out if it is a good idea to
overload the same protocol message and use flags to distinguish the
actual message. Also, I don't know whether prepare_lsn is required
during commit time?

>
> COMMENT
> Line 419
> +pgoutput_abort_prepared_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn,
>
> Since all this function is identical to pg_output_prepare if might be
> better to either
> 1. just leave this as a wrapper to delegate to that function
> 2. remove this one entirely and assign the callback to the common
> pgoutput_prepare_tx
>
> ;

Due to reasons mentioned for the previous comment, let's keep this
also as it is for now.

--
With Regards,
Amit Kapila.