On Tue, Oct 6, 2020 at 10:23 AM peter.b.sm...@fujitsu.com <peter.b.sm...@fujitsu.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.
