Re: Problem while setting the fpw with SIGHUP - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Problem while setting the fpw with SIGHUP
Date
Msg-id CAA4eK1K7J-_iVuULuu8Vgds3YxXYEFMMf7xukEho-QA9G_9_Pg@mail.gmail.com
Whole thread Raw
In response to Re: Problem while setting the fpw with SIGHUP  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: Problem while setting the fpw with SIGHUP  (Michael Paquier <michael@paquier.xyz>)
List pgsql-hackers
On Mon, Sep 10, 2018 at 11:54 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Thanks, but what I wanted you to verify is the commit that broke it in
> 9.5.  On again looking at it, I think it is below code in commit
> 2076db2aea that caused this problem.  If possible, can you once test
> it before and at this commit or at least do the manual review of same
> to cross-verify?
>

I have myself investigated this further and found that this problem
started occuring due to code rearrangement in commits 2c03216d83 and
2076db2aea.  In commit 2076db2aea, below check for comparing the old
and new value for fullPageWrites got changed:

> -       if ((Insert->fullPageWrites || Insert->forcePageWrites) &&
> !doPageWrites)
> +       if (fpw_lsn != InvalidXLogRecPtr && fpw_lsn <= RedoRecPtr &&
> doPageWrites)
>         {

However, it alone didn't lead to this problem because
XLogRecordAssemble() gives the valid value of fpw_lsn irrespective of
whether full-page-writes was enabled or not. Later in commit
2c03216d83, we changed XLogRecordAssemble() such that it will give the
valid value of fpw_lsn only if doPageWrites is enabled, basically this
part of the commit:

+ /* Determine if this block needs to be backed up */
+ if (regbuf->flags & REGBUF_FORCE_IMAGE)
+ needs_backup = true;
+ else if (regbuf->flags & REGBUF_NO_IMAGE)
+ needs_backup = false;
+ else if (!doPageWrites)
+ needs_backup = false;
+ else
  {
- /* Simple data, just include it */
- len += rdt->len;
+ /*
+ * We assume page LSN is first data on *every* page that can be
+ * passed to XLogInsert, whether it has the standard page layout
+ * or not.
+ */
+ XLogRecPtr page_lsn = PageGetLSN(regbuf->page);
+
+ needs_backup = (page_lsn <= RedoRecPtr);
+ if (!needs_backup)
+ {
+ if (*fpw_lsn == InvalidXLogRecPtr || page_lsn < *fpw_lsn)
+ *fpw_lsn = page_lsn;
+ }
  }

So, the problem started appearing after some rearrangement of code in
both the above-mentioned commits.  I verified that this problem
doesn't exist in versions <=9.4, so backpatch-through 9.5.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


pgsql-hackers by date:

Previous
From: Kyotaro HORIGUCHI
Date:
Subject: Re: [HACKERS] Restricting maximum keep segments by repslots
Next
From: Jesper Pedersen
Date:
Subject: Re: executor relation handling