Re: logical replication empty transactions

On Mon, Mar 28, 2022 at 9:22 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monday, March 28, 2022 3:08 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, Mar 25, 2022 at 12:50 PM houzj(dot)fnst(at)fujitsu(dot)com
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > Attach the new version patch with this change.
> > >
> >
> > Few comments:
>
> Thanks for the comments.
>
> > =================
> > 1. I think we can move the keep_alive check after the tracklag record
> > check to keep it consistent with another patch [1].
>
> Changed.
>
> > 2. Add the comment about the new parameter skipped_xact atop
> > WalSndUpdateProgress.
>
> Added.
>
> > 3. I think we need to call pq_flush_if_writable after sending a
> > keepalive message to avoid delaying sync transactions.
>
> Agreed.
> If we don’t flush the data, we might flush the keepalive later than before. And
> we could get the reply later as well and then the release of syncwait could be
> delayed.
>
> Attach the new version patch which addressed the above comments.
> The patch also adds a loop after the newly added keepalive message
> to make sure the message is actually flushed to the client like what
> did in WalSndWriteData.
>

Thank you for updating the patch!

Some 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.

---
+ 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.

---
+# 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.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/