Re: recovering from "found xmin ... from before relfrozenxid ..." - Mailing list pgsql-hackers

Attached v4 patch fixes the latest comments from Robert and Masahiko-san.

Changes:
1) Let heap_force_kill and freeze functions to be used with toast tables.
2) Replace "int nskippedItems" with "bool did_modify_page" flag to
know if any modification happened in the page or not.
3) Declare some of the variables such as buf, page inside of the do
loop instead of declaring them at the top of heap_force_common
function.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Thu, Aug 6, 2020 at 2:49 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> 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