On Mon, Mar 28, 2022 at 9:22 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > On Monday, March 28, 2022 3:08 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Fri, Mar 25, 2022 at 12:50 PM houzj.f...@fujitsu.com > > <houzj.f...@fujitsu.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/