Re: [HACKERS] WAL logging problem in 9.4.3?

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: andrew(dot)dunstan(at)2ndquadrant(dot)com
Cc: hlinnaka(at)iki(dot)fi, robertmhaas(at)gmail(dot)com, michael(at)paquier(dot)xyz, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [HACKERS] WAL logging problem in 9.4.3?
Date: 2018-10-11 08:04:53
Message-ID: 20181011.170453.123148806.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

At Thu, 11 Oct 2018 13:42:35 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20181011(dot)134235(dot)218062184(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Hello.
>
> At Fri, 27 Jul 2018 15:26:24 -0400, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote in <d0c9e197-5219-c094-418a-e5a6fbd8cdda(at)2ndQuadrant(dot)com>
> >
> >
> > On 07/18/2018 10:58 AM, Heikki Linnakangas wrote:
> > > On 18/07/18 16:29, Robert Haas wrote:
> > >> On Wed, Jul 18, 2018 at 9:06 AM, Michael Paquier <michael(at)paquier(dot)xyz>
> > >> wrote:
> > >>>> What's wrong with the approach proposed in
> > >>>> http://postgr.es/m/55AFC302.1060805@iki.fi ?
> > >>>
> > >>> For back-branches that's very invasive so that seems risky to me
> > >>> particularly seeing the low number of complaints on the matter.
> > >>
> > >> Hmm. I think that if you disable the optimization, you're betting that
> > >> people won't mind losing performance in this case in a maintenance
> > >> release.  If you back-patch Heikki's approach, you're betting that the
> > >> committed version doesn't have any bugs that are worse than the status
> > >> quo.  Personally, I'd rather take the latter bet.  Maybe the patch
> > >> isn't all there yet, but that seems like something we can work
> > >> towards.  If we just give up and disable the optimization, we won't
> > >> know how many people we ticked off or how badly until after we've done
> > >> it.
> > >
> > > Yeah. I'm not happy about backpatching a big patch like what I
> > > proposed, and Kyotaro developed further. But I think it's the least
> > > bad option we have, the other options discussed seem even worse.
> > >
> > > One way to review the patch is to look at what it changes, when
> > > wal_level is *not* set to minimal, i.e. what risk or overhead does it
> > > pose to users who are not affected by this bug? It seems pretty safe
> > > to me.
> > >
> > > The other aspect is, how confident are we that this actually fixes the
> > > bug, with least impact to users using wal_level='minimal'? I think
> > > it's the best shot we have so far. All the other proposals either
> > > don't fully fix the bug, or hurt performance in some legit cases.
> > >
> > > I'd suggest that we continue based on the patch that Kyotaro posted at
> > > https://www.postgresql.org/message-id/20180330.100646.86008470.horiguchi.kyotaro%40lab.ntt.co.jp.
> > >
> >
> >
> >
> > I have just spent some time reviewing Kyatoro's patch. I'm a bit
> > nervous, too, given the size. But I'm also nervous about leaving
> > things as they are. I suspect the reason we haven't heard more about
> > this is that these days use of "wal_level = minimal" is relatively
> > rare.
>
> Thank you for lokking this (and sorry for the late response).
>
> > I like the fact that this is closer to being a real fix rather than
> > just throwing out the optimization. Like Heikki I've come round to the
> > view that something like this is the least bad option.
> >
> > The code looks good to me - some comments might be helpful in
> > heap_xlog_update()
>
> Thanks. It is intending to avoid PANIC for a broken record. I
> reverted the part since PANIC would be preferable in the case.
>
> > Do we want to try this on HEAD and then backpatch it? Do we want to
> > add some testing along the lines Michael suggested?
>
> 44cac93464 hit this, rebased. And added Michael's TAP test
> contained in [1] as patch 0001.
>
> I regard [2] as an orthogonal issue.
>
> The previous patch didn't care of the case of
> BEGIN;CREATE;TRUNCATE;COMMIT case. This version contains a "fix"
> of nbtree (patch 0003) so that FPI of the metapage is always
> emitted when building an empty index. On the other hand this
> emits useless one or two FPIs (136 bytes each) on TRUNCATE in a
> separate transaction, but it won't matter so much.. Other index
> methods don't have this problem. Some other AMs emits initialize
> WALs even in minimal mode.
>
> This still has a bit too many elog(DEBUG2)s to see how it is
> working. I'm going to remove most of them in the final version.
>
> I started to prefix the file names with version 2.
>
> regards.
>
> [1] https://www.postgresql.org/message-id/20180711033241.GQ1661@paquier.xyz
>
> [2] https://www.postgresql.org/message-id/CAKJS1f9iF55cwx-LUOreRokyi9UZESXOLHuFDkt0wksZN+KqWw@mail.gmail.com
>
> or
>
> https://commitfest.postgresql.org/20/1811/

I refactored getPendingSyncEntry out of RecordPendingSync,
BufferNeedsWAL and RelationTruncate. And split the second patch
into infrastracture-side and user-side ones. I expect it makes
reviewing far easier.

I reaplce RelationNeedsWAL in a part of code added in
heap_update() by bfa2ab56bb.

- v3-0001-TAP-test-for-copy-truncation-optimization.patch

TAP test

-v3-0002-Write-WAL-for-empty-nbtree-index-build.patch

nbtree "fix"

- v3-0003-Add-infrastructure-to-WAL-logging-skip-feature.patch

Pending-sync infrastructure.

- v3-0004-Fix-WAL-skipping-feature.patch

Actual fix of WAL skipping feature.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachment Content-Type Size
v3-0004-Fix-WAL-skipping-feature.patch text/x-patch 16.5 KB
v3-0003-Add-infrastructure-to-WAL-logging-skip-feature.patch text/x-patch 22.6 KB
v3-0002-Write-WAL-for-empty-nbtree-index-build.patch text/x-patch 2.1 KB
v3-0001-TAP-test-for-copy-truncation-optimization.patch text/x-patch 7.2 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Surafel Temesgen 2018-10-11 08:54:55 COPY FROM WHEN condition
Previous Message Christoph Berg 2018-10-11 07:29:06 Debian mips: Failed test 'Check expected t_009_tbl data on standby'