Re: Skip collecting decoded changes of already-aborted transactions

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.

Regards,

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