Re: Skip collecting decoded changes of already-aborted transactions

On Mon, Nov 11, 2024 at 5:40 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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".

Fixex. We have another place using 'topbig', but I think we can leave it.

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

I think we already mentioned the transaction is going to be spilled
but actually not.

+-- Execute a transaction that is prepared and aborted. We detect that the
+-- transaction is aborted before spilling changes, and then skip collecting
+-- further changes.

>
> ~~~
>
> 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"

Fixed.

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

Agreed, added.

>
> ~~~
>
> 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?

Agreed.

During more testing, I found some bugs in the previous version patch,
so the latest patch incorporates some changes in addition to the
review comments I got so far.

Regards,

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