Re: Problem while setting the fpw with SIGHUP

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Robert Haas <robertmhaas(at)gmail(dot)com>, hlinnaka <hlinnaka(at)iki(dot)fi>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Problem while setting the fpw with SIGHUP
Date: 2018-09-13 11:08:30
Message-ID: CAA4eK1K7J-_iVuULuu8Vgds3YxXYEFMMf7xukEho-QA9G_9_Pg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Mon, Sep 10, 2018 at 11:54 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jesper Pedersen 2018-09-13 11:27:45 Re: executor relation handling
Previous Message Kyotaro HORIGUCHI 2018-09-13 09:29:31 Re: [HACKERS] Restricting maximum keep segments by repslots