Horton

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

Euler Taveira <euler@eulerto.com>

View thread
From: "Euler Taveira" <euler@eulerto.com>
To: "Michael Paquier" <michael@paquier.xyz>, "Dilip Kumar" <dilipbalaut@gmail.com>
Cc: "Amit Kapila" <amit.kapila16@gmail.com>, "Alvaro Herrera" <alvherre@alvh.no-ip.org>, "Petr Jelinek" <petr.jelinek@enterprisedb.com>, "Peter Eisentraut" <peter.eisentraut@enterprisedb.com>, "Tomas Vondra" <tomas.vondra@enterprisedb.com>, "Andres Freund" <andres@anarazel.de>, "Kuntal Ghosh" <kuntalghosh.2007@gmail.com>, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2022-01-24T18:55:34Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. WAL log unchanged toasted replica identity key attributes.

  2. Add new wal_level, logical, sufficient for logical decoding.

  3. Add the notion of REPLICA IDENTITY for a table.

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/