On Thu, 6 Aug 2020 at 18:05, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> 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.
You're right. I'd missed the comment of log_newpage_buffer(). Thanks!
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
