Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1na6dS-0004eB-Bn for pgsql-hackers@arkaria.postgresql.org; Fri, 01 Apr 2022 02:03:34 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1na6dQ-0004zu-9b for pgsql-hackers@arkaria.postgresql.org; Fri, 01 Apr 2022 02:03:32 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1na6dP-0004zl-6y for pgsql-hackers@lists.postgresql.org; Fri, 01 Apr 2022 02:03:32 +0000 Received: from wout3-smtp.messagingengine.com ([64.147.123.19]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1na6dM-0001Po-4F for pgsql-hackers@lists.postgresql.org; Fri, 01 Apr 2022 02:03:30 +0000 Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.west.internal (Postfix) with ESMTP id A26243200C14; Thu, 31 Mar 2022 22:03:25 -0400 (EDT) Received: from imap49 ([10.202.2.99]) by compute2.internal (MEProxy); Thu, 31 Mar 2022 22:03:26 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eulerto.com; h= cc:cc:content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm1; bh=HgyoK4EHt19q1C7J/lfKU14Ar9LOUH9dj2JtgW b7QFQ=; b=SK3KQYeGO0JD4HEg/TKkSvlJox09j2NaYJEstBSHcBbVDzvom1vdkv 2F5fO+dTaMqF6iTJhjdRJd8AXod7QSq2t3i0U7UrQIjZceABafZWhJpHGpqTROWk iNsCW1A1N07XEseJ0JaMxfziWoDaIwYYbDi3Ztn2G8VJ939wFCFSl/IcRmzlHi5i yrh597cABXAyCasP/nzv+GmDWQMgw+8E0wFXSHxuC0HcyV5yXje022IsqYoBnhOQ HN1rK/3DUjqWX2CTXL6m5F6dEMPUaK33vQ1ON48W5laoXRJ6gegykAYm6FibsWjc P1O6hSYvzEYfklooM1VSMgs03sYIKU8Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=HgyoK4EHt19q1C7J/ lfKU14Ar9LOUH9dj2JtgWb7QFQ=; b=BNOjQGS8n8oCV/aPDwNHDnhKCX6lQNiIF TfdD4MyV8rmEIZyS5MKWH+kYi0yOyc21/GeKymG1kJeaN3XwrBOItegZ1vx6vjL7 D8wPmkzsIn8pslaNCw4tEvvIMr+jLGO64iR0zktn6jSI63t3eZq+aQ7ElP9I28AD M49yuaxXQL0/3ar7/HdmriFKwccsSShQAzyz9T8DTYpyHh1w/OII6bRlIGghMu6D iNLWvIiwFY7IZg9ZqKPWzv20E42pbXh8wnldNXUok2o7q3YUMuyEzv7xwcJgkzMb Zq9OpgN7zJca9/f2rDQjbYUi1ooJ6WhCtDK8SDgb+nXQI3jEGcVeA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvvddrudeihedgheefucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgesrgdtreerreerjeenucfhrhhomhepfdfguhhl vghrucfvrghvvghirhgrfdcuoegvuhhlvghrsegvuhhlvghrthhordgtohhmqeenucggtf frrghtthgvrhhnpeeiieehfeefheeuheellefffefgteejleeiveeikeeikeelffegffeu tdeigeeltdenucffohhmrghinhepphhoshhtghhrvghsqhhlrdhorhhgpdgvnhhtvghrph hrihhsvggusgdrtghomhenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgr ihhlfhhrohhmpegvuhhlvghrsegvuhhlvghrthhordgtohhm X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id E58B3F6043A; Thu, 31 Mar 2022 22:03:24 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.7.0-alpha0-382-g88b93171a9-fm-20220330.001-g88b93171 Mime-Version: 1.0 Message-Id: <0271e197-2cfe-4627-9c11-47535cc7fdf3@www.fastmail.com> In-Reply-To: References: Date: Thu, 31 Mar 2022 23:03:03 -0300 From: "Euler Taveira" To: "Masahiko Sawada" , "Amit Kapila" Cc: "wangw.fnst@fujitsu.com" , "kuroda.hayato@fujitsu.com" , "Peter Smith" , "Fabrice Chapuis" , "Simon Riggs" , "Petr Jelinek" , "tanghy.fnst@fujitsu.com" , "PostgreSQL Hackers" , "Ajin Cherian" Subject: Re: Logical replication timeout problem Content-Type: multipart/alternative; boundary=97a123cc443a4367a52e07faf3773c56 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --97a123cc443a4367a52e07faf3773c56 Content-Type: text/plain;charset=utf-8 Content-Transfer-Encoding: quoted-printable On Thu, Mar 31, 2022, at 9:24 AM, Masahiko Sawada wrote: > The patch basically looks good to me. But the only concern to me is > that once we get the patch committed, we will have to call > update_progress() at all paths in callbacks that process changes. > Which seems poor maintainability. I didn't like the current fix for the same reason. We need a robust feed= back system for logical replication. We had this discussion in the "skip empty transactions" thread [1]. > On the other hand, possible another solution would be to add a new > callback that is called e.g., every 1000 changes so that walsender > does its job such as timeout handling while processing the decoded > data in reorderbuffer.c. The callback is set only if the walsender > does logical decoding, otherwise NULL. With this idea, other plugins > will also be able to benefit without changes. But I=E2=80=99m not real= ly sure > it=E2=80=99s a good design, and adding a new callback introduces compl= exity. No new callback is required. In the current code, each output plugin callback is responsible to call OutputPluginUpdateProgress. It is up to the output plugin author to add = calls to this function. The lack of a call in a callback might cause issues li= ke what was described in the initial message. The functions CreateInitDecodingContext and CreateDecodingContext receiv= es the update_progress function as a parameter. These functions are called in 2 places: (a) streaming replication protocol (CREATE_REPLICATION_SLOT) and= (b) SQL logical decoding functions (pg_logical_*_changes). Case (a) uses WalSndUpdateProgress as a progress function. Case (b) does not have one = because it is not required -- local decoding/communication. There is no custom u= pdate progress routine for each output plugin which leads me to the question: couldn't we encapsulate the update progress call into the callback funct= ions? If so, we could have an output plugin parameter to inform which callback= s we would like to call the update progress routine. This would simplify the = code, make it less error prone and wouldn't impose a burden on maintainability. [1] https://www.postgresql.org/message-id/20200309183018.tzkzwu635sd366e= j%40alap3.anarazel.de -- Euler Taveira EDB https://www.enterprisedb.com/ --97a123cc443a4367a52e07faf3773c56 Content-Type: text/html;charset=utf-8 Content-Transfer-Encoding: quoted-printable
On Thu, Mar 31,= 2022, at 9:24 AM, Masahiko Sawada wrote:
The patch basically looks good to me. But= the only concern to me is
that once we get the patch comm= itted, we will have to call
update_progress() at all paths= in callbacks that process changes.
Which seems poor maint= ainability.
I didn't like the current fix for= the same reason. We need a robust feedback
system for log= ical replication. We had this discussion in the "skip empty
transactions" thread [1].

On the other hand, possible another solu= tion would be to add a new
callback that is called e.g., e= very 1000 changes so that walsender
does its job such as t= imeout handling while processing the decoded
data in reord= erbuffer.c. The callback is set only if the walsender
does= logical decoding, otherwise NULL. With this idea, other plugins
will also be able to benefit without changes. But I=E2=80=99m not= really sure
it=E2=80=99s a good design, and adding a new = callback introduces complexity.
No new callba= ck is required.

In the current code, each o= utput plugin callback is responsible to call
OutputPluginU= pdateProgress. It is up to the output plugin author to add calls
to this function. The lack of a call in a callback might cause is= sues like what
was described in the initial message.

The functions CreateInitDecodingContext and Crea= teDecodingContext receives the
update_progress function as= a parameter. These functions are called in 2
places: (a) = streaming replication protocol (CREATE_REPLICATION_SLOT) and (b)
SQL logical decoding functions (pg_logical_*_changes). Case (a) u= ses
WalSndUpdateProgress as a progress function. Case (b) = does not have one because
it is not required -- local deco= ding/communication. There is no custom update
progress rou= tine for each output plugin which leads me to the question:
couldn't we encapsulate the update progress call into the callback fun= ctions?
If so, we could have an output plugin parameter to= inform which callbacks we
would like to call the update p= rogress routine. This would simplify the code,
make it les= s error prone and wouldn't impose a burden on maintainability.
=



--
Euler Taveira=
EDB   https://www.enterprisedb.com/

--97a123cc443a4367a52e07faf3773c56--