Re: logical replication empty transactions

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