Date: Tue, 24 Jan 2023 11:45:35 +0900 (JST)
Message-Id: <20230124.114535.1107425862791533999.horikyota.ntt@gmail.com>
To: osumi.takamichi@fujitsu.com
Cc: smithpb2250@gmail.com, vignesh21@gmail.com, kuroda.hayato@fujitsu.com,
 shveta.malik@gmail.com, dilipbalaut@gmail.com, amit.kapila16@gmail.com,
 euler@eulerto.com, m.melihmutlu@gmail.com, andres@anarazel.de,
 marcos@f10.com.br, pgsql-hackers@postgresql.org
Subject: Re: Time delayed LR (WAS Re: logical replication restrictions)
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <TYCPR01MB8373A23EFC25B22CB132383AEDCB9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
References: <CAHut+Puwrb_DEV_7ANSKrQr23-dAmL9vrNV8ww7XsAP+kej0uw@mail.gmail.com>
	<TYCPR01MB8373BED9E390C4839AF56685EDC59@TYCPR01MB8373.jpnprd01.prod.outlook.com>
	<TYCPR01MB8373A23EFC25B22CB132383AEDCB9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20230124.114535.1107425862791533999.horikyota.ntt%40gmail.com>
Precedence: bulk

> Attached the updated patch v19.

+ maybe_delay_apply(TransactionId xid, TimestampTz finish_ts)

I look this spelling strange.  How about maybe_apply_delay()?


send_feedback():
+	 * If the subscriber side apply is delayed (because of time-delayed
+	 * replication) then do not tell the publisher that the received latest
+	 * LSN is already applied and flushed, otherwise, it leads to the
+	 * publisher side making a wrong assumption of logical replication
+	 * progress. Instead, we just send a feedback message to avoid a publisher
+	 * timeout during the delay.
 	 */
-	if (!have_pending_txes)
+	if (!have_pending_txes && !in_delayed_apply)
 		flushpos = writepos = recvpos;

Honestly I don't like this wart. The reason for this is the function
assumes recvpos = applypos but we actually call it while holding
unapplied changes, that is, applypos < recvpos.

Couldn't we maintain an additional static variable "last_applied"
along with last_received?  In this case the condition cited above
would be as follows and in_delayed_apply will become unnecessary.

+	if (!have_pending_txes && last_received == last_applied)

The function is a static function and always called with a variable
last_received that has the same scope with the function, as the first
parameter. Thus we can remove the first parameter then let the
function directly look at the both two varaibles instead.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center