Re: Skip collecting decoded changes of already-aborted transactions

On Mon, Feb 3, 2025 at 10:41 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Jan 29, 2025 at 11:12 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Tue, Jan 28, 2025 at 9:26 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Jan 27, 2025 at 7:01 PM Peter Smith <smithpb2250(at)gmail(dot)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.
>

If there are no further comments on v18 patches, I'm going to push
them tomorrow.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com