Re: Skip collecting decoded changes of already-aborted transactions

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