On Wed, Jan 29, 2025 at 11:12 PM Peter Smith <smithpb2...@gmail.com> wrote: > > 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.
Thank you for reviewing the patch. > > ====== > .../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); > Agreed with all the above comments. Since checking prepared-transaction-related-flags is getting complicated I've introduced RBTXN_PREPARE_STATUS_FLAGS so that we can check the desired prepared transaction status easily. > ~ > > 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? IIUC ReorderBufferReplay() is responsible for sending a prepare message in this case. See the comment around there: /* * By this time the txn has the prepare record information and it is * important to use that so that downstream gets the accurate * information. If instead, we have passed commit information here * then downstream can behave as it has already replayed commit * prepared after the restart. */ I've attached the updated patches. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
v18-0001-Skip-logical-decoding-of-already-aborted-transac.patch
Description: Binary data
v18-0002-Rename-RBTXN_PREPARE-to-RBTXN_IS_PREPARE-for-bet.patch
Description: Binary data
