Re: Skip collecting decoded changes of already-aborted transactions

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.

======
.../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