On Mon, Feb 7, 2022 2:55 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Sat, Feb 5, 2022 at 6:10 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Fri, Feb 4, 2022 at 9:06 PM Alvaro Herrera <alvhe...@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
