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

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