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

On Tue, Jan 25, 2022 at 12:26 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> 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().
>

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

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.