Re: Skip collecting decoded changes of already-aborted transactions

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Ajin Cherian <itsajin@gmail.com>, vignesh C <vignesh21@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-01-14T02:01:46Z
Lists: pgsql-hackers
Hi Sawada-San.

Some review comments for patch v13-0002.

======

I think the v12 ambiguity of RBTXN_PREPARE versus RBTXN_SENT_PREPARE
was mostly addressed already by the improved comments for the macros
in patch 0001.

Meanwhile, patch v13-0002 says it is renaming constants for better
consistency, but I don't think it went far enough.

For example, better name consistency would be achieved by changing
*all* of the constants related to prepared transactions:

#define RBTXN_IS_PREPARED          0x0040
#define RBTXN_IS_PREPARED_SKIPPED  0x0080
#define RBTXN_IS_PREPARED_SENT 0x0200

where:

RBTXN_IS_PREPARED. This means it's a prepared transaction. (but we
can't tell from this if it is skipped or sent).

RBTXN_IS_PREPARED_SKIPPED. This means it's a prepared transaction
(RBTXN_IS_PREPARED) and it's being skipped.

RBTXN_IS_PREPARED_SENT. This means it's a prepared transaction
(RBTXN_IS_PREPARED) and we've sent it.

~

A note about RBTXN_IS_PREPARED. Since all of these constants are
clearly about transactions (e.g. "TXN" in prefix "RBTXN_"), I felt
patch 0002 calling this RBTXN_IS_PREPARED_TXN just seemed like adding
a redundant _TXN. e.g. we don't say RBTXN_IS_COMMITTED_TXN etc.

======
Kind Regards,
Peter Smith.
Fujitsu Australia



Commits

  1. Rename RBTXN_PREPARE to RBTXN_IS_PREPARE for better clarification.

  2. Skip logical decoding of already-aborted transactions.