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 1nC4W4-00035Q-74 for pgsql-hackers@arkaria.postgresql.org; Mon, 24 Jan 2022 18:56:36 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1nC4W3-0000ND-27 for pgsql-hackers@arkaria.postgresql.org; Mon, 24 Jan 2022 18:56:35 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nC4W1-0000N4-7v for pgsql-hackers@lists.postgresql.org; Mon, 24 Jan 2022 18:56:34 +0000 Received: from wnew3-smtp.messagingengine.com ([64.147.123.17]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nC4Vs-0003RU-FG for pgsql-hackers@postgresql.org; Mon, 24 Jan 2022 18:56:32 +0000 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailnew.west.internal (Postfix) with ESMTP id B42DD2B001D9; Mon, 24 Jan 2022 13:56:19 -0500 (EST) Received: from imap49 ([10.202.2.99]) by compute4.internal (MEProxy); Mon, 24 Jan 2022 13:56:20 -0500 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=fm3; bh=ThPEDtptZYPNQEB9tNNMAlnGljMAnHUi9GHYaY iSMXU=; b=eQSlZDGt+Osnjwuwl5VDFiENYJHVsO6iynN+nTtHmnEcnHZq39VBAS qDyteaNHWksQYPW+cPRbtadm38lmEvIOwuyCeadbt4a4A/f2dZJtMcyXWSuu/SvT nSASKERL5FgcPqSlb/2CsoZoq2jbQ8Oxf4SqsXzuS/ctdkaVCM6/M+ExK9UtAgWX FUBvCHj2nsr4I7pK/pV8oiYEBgX0YIuiwruOh/6CAY+35hIY3wo00J4egzOlqtfm +von/XSIGXzt0U/RRaEfUeFsyixORY1xmFw0W+jc6ycpxHqoMA3EwKzJXfc2mXYd GcLodRAsnlMsO6bSjkHFgu61OsYEwJHw== 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=fm1; bh=ThPEDtptZYPNQEB9t NNMAlnGljMAnHUi9GHYaYiSMXU=; b=PLa1mr5Z0tk7sp4hMVFfTOnFVRTaKfjWE QkAwjFKp3NI8s/TfSq9STYPwXDHILr43BiIAgth3uUHZiRFCq6ELdE7WqQJfoH3C TRY6YUGRD0iNITyLthsPMLQ4vXK/v6dOIrsqJyeEVgiwiXz+BQL6djw/NMEnqIvt fQZDaEMPAUk8s7S4HufmG5OH3DcxMcMokP8vRRfQZboCuLGXMsBAc8x/Xtt20h6M H1mnkKy+HYNYDrLNJD+ubUg7PSxyNCGFeiltFErcvXlnQAEBWQKUL8pqhu3YUlnF 5ow9218EDsBI46aAK5fG+keefnk+nZirxNUNR9TmL8rc2f/0EP/QQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvvddrvdeigdduvddtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgesrgdtreerreertdenucfhrhhomhepfdfguhhl vghrucfvrghvvghirhgrfdcuoegvuhhlvghrsegvuhhlvghrthhordgtohhmqeenucggtf frrghtthgvrhhnpeejtefhuedvgffftdetvedufedtjeeuffekleevkefhveekgeelfefh jeehtdejgeenucffohhmrghinhepvghnthgvrhhprhhishgvuggsrdgtohhmnecuvehluh hsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepvghulhgvrhesvghu lhgvrhhtohdrtghomh X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id C4FC5F60084; Mon, 24 Jan 2022 13:56:18 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-4585-ga9d9773056-fm-20220113.001-ga9d97730 Mime-Version: 1.0 Message-Id: In-Reply-To: References: <202108101438.bmmf4gqklowd@alvherre.pgsql> Date: Mon, 24 Jan 2022 15:55:34 -0300 From: "Euler Taveira" To: "Michael Paquier" , "Dilip Kumar" Cc: "Amit Kapila" , "Alvaro Herrera" , "Petr Jelinek" , "Peter Eisentraut" , "Tomas Vondra" , "Andres Freund" , "Kuntal Ghosh" , "tanghy.fnst@fujitsu.com" , pgsql-hackers Subject: Re: [BUG]Update Toast data failure in logical replication Content-Type: multipart/alternative; boundary=8a486c3abea84a108a52fc150fb6c8b3 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --8a486c3abea84a108a52fc150fb6c8b3 Content-Type: text/plain On Mon, Jan 24, 2022, at 12:58 AM, Michael Paquier wrote: > FWIW, I find the API changes of HeapDetermineModifiedColumns() and > ExtractReplicaIdentity() a bit grotty. Shouldn't we try to flatten > the old tuple instead? There are things like > toast_flatten_tuple_to_datum() for this purpose if a tuple satisfies > HeapTupleHasExternal(), or just heap_copy_tuple_as_datum(). > I checked v4 and I don't like the HeapDetermineModifiedColumns() and heap_tuple_attr_equals() changes either. It seems it is hijacking these functions to something else. I would suggest to change the signature to static void heap_tuple_attr_equals(TupleDesc tupdesc, int attrnum, HeapTuple tup1, HeapTuple tup2, bool *is_equal, bool *key_has_external); and static void HeapDetermineModifiedColumns(Relation relation, Bitmapset *interesting_cols, HeapTuple oldtup, HeapTuple newtup, Bitmapset *modified_attrs, bool *key_has_external); I didn't figure out a cheap way to check if the key has external value other than slightly modifying the HeapDetermineModifiedColumns() function and its subroutine heap_tuple_attr_equals(). As Alvaro said I don't think adding HeapTupleHasExternal() (as in v3) is a good idea because it does not optimize genuine cases such as a table whose PK is an integer and contains a single TOAST column. Another suggestion is to keep key_changed and add another attribute (key_has_external) to ExtractReplicaIdentity(). If we need key_changed in the future we'll have to decompose it again. You also encapsulates that optimization into the function that helps with future improvements/fixes. static HeapTuple ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool key_changed, bool key_has_external, bool *copy); -- Euler Taveira EDB https://www.enterprisedb.com/ --8a486c3abea84a108a52fc150fb6c8b3 Content-Type: text/html Content-Transfer-Encoding: quoted-printable
On Mon, Jan 24,= 2022, at 12:58 AM, Michael Paquier wrote:
FWIW, I find the API changes of HeapDete= rmineModifiedColumns() and
ExtractReplicaIdentity() a bit = grotty.  Shouldn't we try to flatten
the old tuple in= stead?  There are things like
toast_flatten_tuple_to_= datum() for this purpose if a tuple satisfies
HeapTupleHas= External(), or just heap_copy_tuple_as_datum().

=
I checked v4 and I don't like the HeapDetermineModifie= dColumns() and
heap_tuple_attr_equals() changes either. It= seems it is hijacking these
functions to something else. = I would suggest to change the signature to

= static void
heap_tuple_attr_equals(TupleDesc tupdesc, int = attrnum,
        &= nbsp;           &= nbsp;  HeapTuple tup1, HeapTuple tup2,
  &n= bsp;           &n= bsp;        bool *is_equal, bool *key= _has_external);

and

static void
HeapDetermineModifiedColumns(Relation re= lation, Bitmapset *interesting_cols,
   &nb= sp;           &nb= sp;           &nb= sp; HeapTuple oldtup, HeapTuple newtup,
   =             =             =   Bitmapset *modified_attrs, bool *key_has_external);

I didn't figure out a cheap way to check if the key has = external value other
than slightly modifying the HeapDeter= mineModifiedColumns() function and its
subroutine heap_tup= le_attr_equals(). As Alvaro said I don't think adding
Heap= TupleHasExternal() (as in v3) is a good idea because it does not optimiz= e
genuine cases such as a table whose PK is an integer and= contains a single
TOAST column.

<= div>Another suggestion is to keep key_changed and add another attribute<= br>
(key_has_external) to ExtractReplicaIdentity(). If we need= key_changed in the
future we'll have to decompose it agai= n. You also encapsulates that
optimization into the functi= on that helps with future improvements/fixes.

static HeapTuple
ExtractReplicaIdentity(Relation relati= on, HeapTuple tp, bool key_changed,
   &nbs= p;           &nbs= p;       bool key_has_external, bool *copy= );


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

--8a486c3abea84a108a52fc150fb6c8b3--