Hi,
Please see below review of the 0001-Skip-empty-transactions-for-logical-replication.patch
The make check passes.
+ /* output BEGIN if we haven't yet */
+ if (!data->xact_wrote_changes)
+ pgoutput_begin(ctx, txn);
+
+ data->xact_wrote_changes = true;
+
IMO, xact_wrote_changes flag is better set inside the if condition as it does not need to
be set repeatedly in subsequent calls to the same function.
* Stash BEGIN data in plugin's LogicalDecodingContext.output_plugin_private when plugin's begin callback called, don't write anything to the outstream
* Write out BEGIN message lazily when any other callback generates a message that does need to be written out
* If no BEGIN written by the time COMMIT callback called, discard the COMMIT too. Check if sync rep enabled. if it is, call LogicalDecodingContext.update_progress
from within the output plugin commit handler, otherwise just ignore the commit totally. Probably by calling OutputPluginUpdateProgress().
I think the code in the patch is similar to what has been described by Craig in the above snippet,
except instead of stashing the BEGIN message and sending the message lazily, it simply maintains a flag
in LogicalDecodingContext.output_plugin_private which defers calling output plugin's begin callback,
until any other callback actually generates a remote write.
Also, the patch does not contain the last part where he describes having OutputPluginUpdateProgress()
for synchronous replication enabled transactions.
However, some basic testing suggests that the patch does not have any notable adverse effect on
either the replication lag or the sync_rep performance.
I performed tests by setting up publisher and subscriber on the same machine with synchronous_commit = on and
ran pgbench -c 12 -j 6 -T 300 on unpublished pgbench tables.
I see that confirmed_flush_lsn is catching up just fine without any notable delay as compared to the test results without
the patch.
Also, the TPS for synchronous replication of empty txns with and without the patch remains similar.
Having said that, these are initial findings and I understand better performance tests are required to measure
reduction in consumption of network bandwidth and impact on synchronous replication and replication lag.
Thank you,
Rahila Syed