Re: Skip collecting decoded changes of already-aborted transactions

On Tue, Nov 12, 2024 at 5:00 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached the updated patch.
>

Hi, here are some review comments for the latest v6-0001.

======
contrib/test_decoding/sql/stats.sql

1.
+INSERT INTO stats_test SELECT 'serialize-topbig--1:'||g.i FROM
generate_series(1, 5000) g(i);

I didn't understand the meaning of "serialize-topbig--1". My guess is
it is a typo that was supposed to say "toobig".

Perhaps there should also be some comment to explain that this
"toobig" stuff was done deliberately like this to exceed
'logical_decoding_work_mem' because that would normally (if it was not
aborted) cause a spill to disk.

~~~

2.
+-- Check stats. We should not spill anything as the transaction is already
+-- aborted.
+SELECT pg_stat_force_next_flush();
+SELECT slot_name, spill_txns AS spill_txn, spill_count AS spill_count
FROM pg_stat_replication_slots WHERE slot_name =
'regression_slot_stats4_twophase';
+

Those aliases seem unnecessary: "spill_txns AS spill_txn" and
"spill_count AS spill_count"

======
.../replication/logical/reorderbuffer.c

ReorderBufferCheckTXNAbort:

3.
Other static functions are also declared at the top of this module.
For consistency, shouldn't this be the same?

~~~

4.
+ * We don't mark the transaction as streamed since this function can be
+ * called for non-streamed transactions too.
+ */
+ ReorderBufferTruncateTXN(rb, txn, rbtxn_prepared(txn), false);
+ ReorderBufferToastReset(rb, txn);

Given the comment says "since this function can be called for
non-streamed transactions too", would it be easier to pass
rbtxn_is_streamed(txn) here instead of 'false', and then just remove
the comment?

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