Horton

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

tanghy <tanghy.fnst@fujitsu.com>

View thread
From: "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, Euler Taveira <euler@eulerto.com>, Michael Paquier <michael@paquier.xyz>, 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>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2022-02-09T01:18:24Z
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, Feb 7, 2022 2:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Sat, Feb 5, 2022 at 6:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Feb 4, 2022 at 9:06 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> > >
> > >  I have some suggestions
> > > on the comments and docs though.
> > >
> >
> > Thanks, your suggestions look good to me. I'll take care of these in
> > the next version.
> >
> 
> Attached please find the modified patches.
> 

Thanks for your patch. I tried it and it works well.
Two small comments:

1)
+static Bitmapset *HeapDetermineColumnsInfo(Relation relation,
+										   Bitmapset *interesting_cols,
+										   Bitmapset *external_cols,
+										   HeapTuple oldtup, HeapTuple newtup,
+										   bool *id_has_external);

+HeapDetermineColumnsInfo(Relation relation,
+						 Bitmapset *interesting_cols,
+						 Bitmapset *external_cols,
+						 HeapTuple oldtup, HeapTuple newtup,
+						 bool *has_external)

The declaration and the definition of this function use different parameter
names for the last parameter (id_has_external and has_external), it's better to
be consistent.

2)
+		/*
+		 * Check if the old tuple's attribute is stored externally and is a
+		 * member of external_cols.
+		 */
+		if (VARATT_IS_EXTERNAL((struct varlena *) DatumGetPointer(value1)) &&
+			bms_is_member(attrnum - FirstLowInvalidHeapAttributeNumber,
+						  external_cols))
+			*has_external = true;

If has_external is already true, it seems we don't need this check, so should we
check has_external first?

Regards,
Tang