On Wed, Jan 29, 2025 at 9:32 PM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Some comments for patch v17-0001.
Thank you for reviewing the patch!
>
> ======
> Commit message.
>
> 1.
> typo /noticeble/noticeable/
Fixed.
>
> ======
> .../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
Hmm, it doesn't seem so tricky to me that a function with the name
ReorderBufferCheckAndTruncateAbortedTXN() checks the transaction
status to truncate an aborted transaction and caches the transaction
status as a 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?
We can have a similar assert there but why do you think it's needed there?
>
> ======
> 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?
>
I'm not sure these names are better.
In logical decoding context, we neither commit nor rollback
transactions decoded from WAL records as the transaction outcomes come
only from WAL records. So I guess it's easy-to-grasp that
RBTXN_IS_COMMITTED means "this is a committed transaction" but not "we
committed the transaction". I think this is a similar understanding as
what we're trying to rename RBTXN_PREPARE to RBTXN_IS_PREPARE.
Similarly, we have rb->commit() and rb->abort(), I would not think
like we're committing or aborting the transaction. So the lag between
the ->commit()/abort() happening and these flags getting set is not
confusing (at least for me). I think we can leave these names as they
are, and if we need to remember if a commit message has been sent, we
would be able to have a flag like RBTXN_SENT_COMMIT.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
