Re: [HACKERS] WAL logging problem in 9.4.3?

At Thu, 11 Oct 2018 13:42:35 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20181011.134235.218062184.horiguchi.kyotaro@lab.ntt.co.jp>
> Hello.
> 
> At Fri, 27 Jul 2018 15:26:24 -0400, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote in <d0c9e197-5219-c094-418a-e5a6fbd8cdda@2ndQuadrant.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@paquier.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