RE: logical replication empty transactions

On Tuesday, March 29, 2022 3:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> 
> Some comments:

Thanks for the comments!

> 
> +       if (skipped_xact &&
> +               SyncRepRequested() &&
> +               ((volatile WalSndCtlData *)
> WalSndCtl)->sync_standbys_defined)
> +       {
> +               WalSndKeepalive(false, ctx->write_location);
> 
> I think we can use 'lsn' since it is actually ctx->write_location.

Agreed, and changed.

> ---
> +       if (!sent_begin_txn)
> +       {
> +               elog(DEBUG1, "Skipped replication of an empty
> transaction with XID: %u", txn->xid);
> +               return;
> +       }
> 
> The log message should start with lowercase.

Changed.

> ---
> +# Note that the current location of the log file is not grabbed
> +immediately # after reloading the configuration, but after sending one
> +SQL command to # the node so as we are sure that the reloading has taken
> effect.
> +$log_location = -s $node_subscriber->logfile;
> +
> +$node_publisher->safe_psql('postgres', "INSERT INTO tab_notrep VALUES
> +(11)");
> +
> +$node_publisher->wait_for_catchup('tap_sub');
> +
> +$logfile = slurp_file($node_publisher->logfile, $log_location);
> 
> I think we should get the log location of the publisher node, not subscriber
> node.

Changed.

Attach the new version patch which addressed the
above comments and slightly adjusted some code comments.

Best regards,
Hou zj