public inbox for [email protected]  

help / color / mirror / Atom feed

From: Amit Kapila <[email protected]>
To: Euler Taveira <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Petr Jelinek <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Kuntal Ghosh <[email protected]>
Cc: [email protected] <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: [BUG]Update Toast data failure in logical replication
Date: Tue, 25 Jan 2022 11:59:10 +0530
Message-ID: <CAA4eK1+q4xu5pHsSUL=kPO+8sBNeUsqnYCoOZ-NWy6nLEVC3SQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAA4eK1+k2TKPmdeZxgp2vgFtv2C8+dMKHR=-ty9HswDcCqkK6A@mail.gmail.com>
	<[email protected]>
	<CAA4eK1KgZr=QSBE_Qh0Qfb2ma1Tc6+ZxkMaUHO7aC7b9WSCRaw@mail.gmail.com>
	<CAFiTN-uW50w0tWoUBg_VYCdvNeCzT=t=JzhmiFd452FrLOwMMQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>

On Tue, Jan 25, 2022 at 12:26 AM Euler Taveira <[email protected]> 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.

view thread (5+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [BUG]Update Toast data failure in logical replication
  In-Reply-To: <CAA4eK1+q4xu5pHsSUL=kPO+8sBNeUsqnYCoOZ-NWy6nLEVC3SQ@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox