On Mon, Nov 11, 2024 at 5:40 PM Peter Smith <smithpb2...@gmail.com> wrote: > > On Tue, Nov 12, 2024 at 5:00 AM Masahiko Sawada <sawada.m...@gmail.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
v6-0001-Skip-logical-decoding-of-already-aborted-transact.patch
Description: Binary data
