Re: recovering from "found xmin ... from before relfrozenxid ..."

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