Some comments for patch v17-0001.
======
Commit message.
1.
typo /noticeble/noticeable/
======
.../replication/logical/reorderbuffer.c
ReorderBufferCheckAndTruncateAbortedTXN:
2.
It seemed tricky that the only place that is setting the
RBTXN_IS_COMMITTED flag is the function
ReorderBufferCheckAndTruncateAbortedTXN because neither the function
name nor the function comment gives any indication that it should be
having this side effect
~~~
ReorderBufferProcessTXN:
3.
if (rbtxn_prepared(txn))
+ {
rb->prepare(rb, txn, commit_lsn);
+ txn->txn_flags |= RBTXN_SENT_PREPARE;
+ }
In ReorderBufferStreamCommit there is an assertion that we are not
trying to do another prepare() if the _SENT_PREPARE flag is already
set. Should this code have a similar assert?
======
src/include/replication/reorderbuffer.h
4.
+#define RBTXN_SENT_PREPARE 0x0200
+#define RBTXN_IS_COMMITTED 0x0400
+#define RBTXN_IS_ABORTED 0x0800
IIUC, unlike the _SENT_PREPARE, those _IS_COMMITTED and _IS_ABORTED
flags are not quite the same as saying rb->commit() or rb->abort() was
called. But, those flags are only set some time later by
ReorderBufferCheckAndTruncateAbortedTXN() function based on the commit
log status.
The lag between the commit/abort happening and these flag getting set
seems unintuitive. Should they be named differently -- e.g. maybe
RBTXN_IS_CLOG_COMMITTED, RBTXN_IS_CLOG_ABORTED instead?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
