Subject: Re: [BUG]Update Toast data failure in logical replication

From: Michael Paquier <[email-masked]>
Date: Mon, 24 Jan 2022 12:58:34 +0900

On Wed, Aug 11, 2021 at 06:14:55PM +0530, Dilip Kumar wrote:
> Right

Amit, are you planning to look more at this patch?  It has been a
couple of months since the last update, and this is still a bug as far
as I understand.

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().
--
Michael

---

From: Amit Kapila <[email-masked]>
Date: Mon, 24 Jan 2022 17:35:50 +0530

> HeapTupleHasExternal(), or just heap_copy_tuple_as_datum().

That can add overhead in cases where we don't need to log the toasted
values of the old tuple. We only need it for the case where we have
unchanged toasted replica identity columns. In the previous version
[1], we were doing something like you are suggesting and that seems to
have overhead as explained in the second paragraph of the email [2].
Also, Alvaro seems to have some reservations about that change. I
don't know if there is a better way to fix this but I could be missing
something.

[1] - https://www.postgresql.org/message-id/CAFiTN-sTS4bB7W3UJV3iUm%3DwKdr9EpOwyK97hNr77MzFQm_NBw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAA4eK1KgZr%3DQSBE_Qh0Qfb2ma1Tc6%2BZxkMaUHO7aC7b9WSCRaw%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: "Euler Taveira" <[email-masked]>
Date: Mon, 24 Jan 2022 15:55:34 -0300

> 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/

---

From: Amit Kapila <[email-masked]>
Date: Tue, 25 Jan 2022 11:59:10 +0530

On Tue, Jan 25, 2022 at 12:26 AM Euler Taveira <[email-masked]> wrote:
> subroutine heap_tuple_attr_equals().
>

I am not sure if your proposal is much different compared to v4 or how
much it improves the situation? I see you didn't consider
'check_external_attr' parameter and I think that is important to know
if the key has any external toast value. Overall, I see your point
that the change of APIs looks a bit ugly. But, I guess that is more
due to their names and current purpose. I think it could be better if
we bring all the code of heap_tuple_attr_equals in its only caller
HeapDetermineModifiedColumns or at least part of the code where we get
attr value and can determine whether the value is stored externally.
Then change name of HeapDetermineModifiedColumns to
HeapDetermineColumnsInfo with additional parameters.

> As Alvaro said I don't think adding
> future we'll have to decompose it again.
>

True, but we can make the required changes at that point as well.
OTOH, we can do what you are suggesting as well but I am not sure if
that is required.

-- 
With Regards,
Amit Kapila.