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 04:42:35
Message-ID: 20181011.134235.218062184.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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/

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachment Content-Type Size
v2-0001-TAP-test-for-copy-truncation-optimization.patch text/x-patch 7.2 KB
v2-0002-Fix-WAL-logging-problem.patch text/x-patch 38.2 KB
v2-0003-Write-WAL-for-empty-nbtree-index-build.patch text/x-patch 2.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2018-10-11 04:45:12 Re: Calculate total_table_pages after set_base_rel_sizes()
Previous Message David Rowley 2018-10-11 04:05:05 Re: Small performance tweak to run-time partition pruning