Re: Skip collecting decoded changes of already-aborted transactions

Sorry for the late reply.

On Tue, Jun 11, 2024 at 7:41 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi, here are some review comments for your patch v4-0001.

Thank you for reviewing the patch!

>
> ======
> contrib/test_decoding/sql/stats.sql
>
> 1.
> Huh? The test fails because the "expected results" file for these new
> tests is missing from the patch.

Fixed.

>
> ======
> .../replication/logical/reorderbuffer.c
>
> 2.
> static void ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
> - bool txn_prepared);
> + bool txn_prepared, bool mark_streamed);
>
> IIUC this new 'mark_streamed' parameter is more like a prerequisite
> for the other conditions to decide to mark the tx as streamed -- i.e.
> it is more like 'can_mark_streamed', so I felt the name should be
> changed to be like that (everywhere it is used).

Agreed. I think 'txn_streaming' sounds better and consistent with
'txn_prepared'.

>
> ~~~
>
> 3. ReorderBufferTruncateTXN
>
> - * 'txn_prepared' indicates that we have decoded the transaction at prepare
> - * time.
> + * If mark_streamed is true, we could mark the transaction as streamed.
> + *
> + * 'streaming_txn' indicates that the given transaction is a
> streaming transaction.
> */
> static void
> -ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
> bool txn_prepared)
> +ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
> bool txn_prepared,
> + bool mark_streamed)
>
> ~
>
> What's that new comment about 'streaming_txn' for? It seemed unrelated
> to the patch code.

Removed.

>
> ~~~
>
> 4.
> /*
> * Mark the transaction as streamed.
> *
> * The top-level transaction, is marked as streamed always, even if it
> * does not contain any changes (that is, when all the changes are in
> * subtransactions).
> *
> * For subtransactions, we only mark them as streamed when there are
> * changes in them.
> *
> * We do it this way because of aborts - we don't want to send aborts for
> * XIDs the downstream is not aware of. And of course, it always knows
> * about the toplevel xact (we send the XID in all messages), but we never
> * stream XIDs of empty subxacts.
> */
> if (mark_streamed && (!txn_prepared) &&
> (rbtxn_is_toptxn(txn) || (txn->nentries_mem != 0)))
> txn->txn_flags |= RBTXN_IS_STREAMED;
>
> ~~
>
> With the patch introduction of the new parameter, I felt this code
> might be better if it was refactored as follows:
>
> /* Mark the transaction as streamed, if appropriate. */
> if (can_mark_streamed)
> {
> /*
> ... large comment
> */
> if ((!txn_prepared) && (rbtxn_is_toptxn(txn) || (txn->nentries_mem != 0)))
> txn->txn_flags |= RBTXN_IS_STREAMED;
> }

I think we don't necessarily need to make nested if statements just
for comments.

>
> ~~~
>
> 5. ReorderBufferPrepare
>
> - if (txn->concurrent_abort && !rbtxn_is_streamed(txn))
> + if (!txn_aborted && rbtxn_did_abort(txn) && !rbtxn_is_streamed(txn))
> rb->prepare(rb, txn, txn->final_lsn);
>
> ~
>
> Maybe I misunderstood this logic, but won't a "concurrent abort" cause
> your new Assert added in ReorderBufferProcessTXN to fail?
>
> + /* Update transaction status */
> + Assert((curtxn->txn_flags & (RBTXN_COMMITTED | RBTXN_ABORTED)) == 0);
>

I changed txn_flags checks, which should cover your concerns.

> ~~~
>
> 6. ReorderBufferCheckTXNAbort
>
> + /* Check the transaction status using CLOG lookup */
> + if (TransactionIdIsInProgress(txn->xid))
> + return false;
> +
> + if (TransactionIdDidCommit(txn->xid))
> + {
> + /*
> + * Remember the transaction is committed so that we can skip CLOG
> + * check next time, avoiding the pressure on CLOG lookup.
> + */
> + txn->txn_flags |= RBTXN_COMMITTED;
> + return false;
> + }
>
> IIUC the purpose of the TransactionIdDidCommit() was to avoid the
> overhead of calling the TransactionIdIsInProgress(). So, shouldn't the
> order of these checks be swapped? Otherwise, there might be 1 extra
> unnecessary call to TransactionIdIsInProgress() next time.

I'm not sure I understand your comment. IIUC we should use
TransactionIdDidCommit() with a preceding TransactionIdIsInProgress()
check. Also I think once we found the transaction is committed, we no
longer check the transaction status on CLOG nor call
TransactionIdIsInProgress().

>
> ======
> src/include/replication/reorderbuffer.h
>
> 7.
> #define RBTXN_PREPARE 0x0040
> #define RBTXN_SKIPPED_PREPARE 0x0080
> #define RBTXN_HAS_STREAMABLE_CHANGE 0x0100
> +#define RBTXN_COMMITTED 0x0200
> +#define RBTXN_ABORTED 0x0400
>
> For consistency with the existing bitmask names, I guess these should be named:
> - RBTXN_COMMITTED --> RBTXN_IS_COMMITTED
> - RBTXN_ABORTED --> RBTXN_IS_ABORTED

Agreed and changed.

>
> ~~~
>
> 8.
> Similarly, IMO the macros should have the same names as the bitmasks,
> like the other nearby ones generally seem to.
>
> rbtxn_did_commit --> rbtxn_is_committed
> rbtxn_did_abort --> rbtxn_is_aborted

Changed.

>
> ======
>
> 9.
> Also, attached is a top-up patch for other cosmetic nitpicks:
> - comment wording
> - typos in comments
> - excessive or missing blank lines
> - etc.
>

Applied your patch.

I've attached the updated patch. Will register it for the next commit fest.

Regards,

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