RE: logical replication empty transactions

On Tuesday, March 29, 2022 3:20 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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