Re: Skip collecting decoded changes of already-aborted transactions

Hi Sawda-San,

Here are some more review comments for the latest (accidentally called
v6 again?) v6-0001 patch.

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

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

You had replied (referring to the above comment):
I think we already mentioned the transaction is going to be spilled
but actually not.

~

Yes, spilling was already mentioned in the current comment but I felt
it assumes the reader is expected to know details of why it was going
to be spilled in the first place.

In other words, I thought the comment could include a bit more
explanatory background info:
(Also, it's not really "we detect" the abort -- it's the new postgres
code of this patch that detects it.)

SUGGESTION:
Execute a transaction that is prepared but then aborted. The INSERT
data exceeds the 'logical_decoding_work_mem limit' limit which
normally would result in the transaction being spilled to disk, but
now when Postgres detects the abort it skips the spilling and also
skips collecting further changes.

~~~

2.
+-- Check if the transaction is not spilled as it's already aborted.
+SELECT count(*) FROM
pg_logical_slot_get_changes('regression_slot_stats4_twophase', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+SELECT pg_stat_force_next_flush();
+SELECT slot_name, spill_txns, spill_count FROM
pg_stat_replication_slots WHERE slot_name =
'regression_slot_stats4_twophase';
+

/Check if the transaction is not spilled/Verify that the transaction
was not spilled/

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

ReorderBufferResetTXN:

3.
/* Discard the changes that we just streamed */
- ReorderBufferTruncateTXN(rb, txn, rbtxn_prepared(txn));
+ ReorderBufferTruncateTXN(rb, txn, rbtxn_prepared(txn), true);

Looking at the calling code for ReorderBufferResetTXN it seems this
function can called for streaming OR prepared. So is it OK here to be
passing hardwired 'true' as the txn_streaming parameter, or should
that be passing rbtxn_is_streamed(txn)?

~~~

ReorderBufferLargestStreamableTopTXN:

4.
if ((largest == NULL || txn->total_size > largest_size) &&
(txn->total_size > 0) && !(rbtxn_has_partial_change(txn)) &&
- rbtxn_has_streamable_change(txn))
+ rbtxn_has_streamable_change(txn) && !(rbtxn_is_aborted(txn)))
{
largest = txn;
largest_size = txn->total_size;

I felt that this increasingly complicated code would be a lot easier
to understand if you just separate the conditions into: (a) the ones
that filter out transaction you don't care about; (b) the ones that
check for the largest size. For example,

SUGGESTION:
dlist_foreach(...)
{
...

/* Don't consider these kinds of transactions for eviction. */
if (rbtxn_has_partial_change(txn) ||
!rbtxn_has_streamable_change(txn) || rbtxn_is_aborted(txn))
continue;

/* Find the largest of the eviction candidates. */
if ((largest == NULL || txn->total_size > largest_size) &&
(txn->total_size > 0))
{
largest = txn;
largest_size = txn->total_size;
}
}

~~~

ReorderBufferCheckMemoryLimit:

5.
+ /* skip the transaction if already aborted */
+ if (ReorderBufferCheckTXNAbort(rb, txn))
+ {
+ /* All changes should be truncated */
+ Assert(txn->size == 0 && txn->total_size == 0);
+ continue;
+ }

The "discard all changes accumulated so far" side-effect happening
here is not very apparent from the function name. Maybe a better name
for ReorderBufferCheckTXNAbort() would be something like
'ReorderBufferCleanupIfAbortedTXN()'.

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