|From:||Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>|
|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?|
|Views:||Raw Message | Whole Thread | Download mbox|
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>
> 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.firstname.lastname@example.org ?
> > >>>
> > >>> 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  as patch 0001.
> I regard  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.
>  https://www.postgresql.org/message-id/20180711033241.GQ1661@paquier.xyz
>  https://www.postgresql.org/message-id/CAKJS1f9iF55cwx-LUOreRokyi9UZESXOLHuFDkt0wksZN+KqWw@mail.gmail.com
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.
Actual fix of WAL skipping feature.
NTT Open Source Software Center
|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'|