Hello Masahiko-san,
Thanks for looking into the patch. Please find my comments inline below:
On Thu, Aug 6, 2020 at 1:42 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Wed, 5 Aug 2020 at 22:42, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > Please check the attached patch for the changes.
>
> I also looked at this version patch and have some small comments:
>
> + Oid relid = PG_GETARG_OID(0);
> + ArrayType *ta = PG_GETARG_ARRAYTYPE_P_COPY(1);
> + ItemPointer tids;
> + int ntids;
> + Relation rel;
> + Buffer buf;
> + Page page;
> + ItemId itemid;
> + BlockNumber blkno;
> + OffsetNumber *offnos;
> + OffsetNumber offno,
> + noffs,
> + curr_start_ptr,
> + next_start_ptr,
> + maxoffset;
> + int i,
> + nskippedItems,
> + nblocks;
>
> You declare all variables at the top of heap_force_common() function
> but I think we can declare some variables such as buf, page inside of
> the do loop.
>
Sure, I will do this in the next version of patch.
> ---
> + if (offnos[i] > maxoffset)
> + {
> + ereport(NOTICE,
> + errmsg("skipping tid (%u, %u) because it
> contains an invalid offset",
> + blkno, offnos[i]));
> + continue;
> + }
>
> If all tids on a page take the above path, we will end up logging FPI
> in spite of modifying nothing on the page.
>
Yeah, that's right. I've taken care of this in the new version of
patch which I am yet to share.
> ---
> + /* XLOG stuff */
> + if (RelationNeedsWAL(rel))
> + log_newpage_buffer(buf, true);
>
> I think we need to set the returned LSN by log_newpage_buffer() to the page lsn.
>
I think we are already setting the page lsn in the log_newpage() which
is being called from log_newpage_buffer(). So, AFAIU, no change would
be required here. Please let me know if I am missing something.
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com