On Tue, Jan 28, 2025 at 9:26 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Mon, Jan 27, 2025 at 7:01 PM Peter Smith <smithpb2...@gmail.com> wrote: > > ...
> > To be honest, I didn't understand the "CLEAR" part of that name. It > > seems more like it should've been called something like > > RBTXN_IS_SERIALIZED_ALREADY or RBTXN_IS_SERIALIZED_PREVIOUSLY or > > whatever instead of something that appears to be saying "has the > > RBTXN_IS_SERIALIZED bitflag been cleared?" I understand the reluctance > > to over-comment everything but OTOH currently there is no way really > > to understand what these flags mean without looking through all the > > code to try to figure them out from the usage. > > > > My recurring gripe about these flags is simply that their meanings and > > how to use them should be apparent just by looking at reorderbuffer.h > > and not having to guess anything or look at how they get used in the > > code. It doesn't matter if that is achieved by better constant names, > > by more comments or by enhanced macros/functions with asserts but > > currently just looking at that file still leaves the reader with lots > > of unanswered questions. > > I see your point. IIUC we have the comments about what the checks with > the flags means but not have the description about the relationship > among the flags. I think we can start a new thread for clarifying > these flags and their usage. We can also discuss renaming > RBTXN_IS_SERIALIZED[_CLEARE] there too. > OK. ====== Some comments for patch v17-0002. ====== .../replication/logical/reorderbuffer.c ReorderBufferSkipPrepare: 1. + /* txn must have been marked as a prepared transaction */ + Assert((txn->txn_flags & RBTXN_IS_PREPARED) != 0); + txn->txn_flags |= RBTXN_SKIPPED_PREPARE; Should this also be asserting that the _SENT_PREPARE flag is false, because we cannot be skipping it if we already sent the prepare. ~~~ ReorderBufferFinishPrepared: 2. - txn->txn_flags |= RBTXN_PREPARE; - /* - * The prepare info must have been updated in txn even if we skip - * prepare. + * txn must have been marked as a prepared transaction and skipped but + * not sent a prepare. Also, the prepare info must have been updated + * in txn even if we skip prepare. */ + Assert((txn->txn_flags & (RBTXN_IS_PREPARED | RBTXN_SKIPPED_PREPARE)) != 0); + Assert((txn->txn_flags & RBTXN_SENT_PREPARE) == 0); Assert(txn->final_lsn != InvalidXLogRecPtr); 2a. If it must have been prepared *and* skipped (as the comment says) then the first assert should be written as: Assert((txn->txn_flags & (RBTXN_IS_PREPARED | RBTXN_SKIPPED_PREPARE)) == (RBTXN_IS_PREPARED | RBTXN_SKIPPED_PREPARE)); or easier to just have 2 asserts: Assert(txn->txn_flags & RBTXN_IS_PREPARED); Assert(txn->txn_flags & RBTXN_SKIPPED_PREPARE); ~ 2b. later in the same function there is code: if (is_commit) rb->commit_prepared(rb, txn, commit_lsn); else rb->rollback_prepared(rb, txn, prepare_end_lsn, prepare_time); So it is OK to do a commit_prepared/rollback_prepared even though no prepare() has been sent? ====== Kind Regards, Peter Smith. Fujitsu Australia
