RE: [BUG]Update Toast data failure in logical replication - Mailing list pgsql-hackers

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