Re: Problem while setting the fpw with SIGHUP

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