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