Thread: Draft back-branch release notes are up for review

Draft back-branch release notes are up for review

From
Tom Lane
Date:
I've committed first-draft release notes for next week's
releases at

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=0995cefa74510ee0e38d1bf095b2eef2c1ea37c4

Please send any review comments by Sunday.

            regards, tom lane



Re: Draft back-branch release notes are up for review

From
Noah Misch
Date:
On Fri, Jun 14, 2019 at 04:58:47PM -0400, Tom Lane wrote:
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=0995cefa74510ee0e38d1bf095b2eef2c1ea37c4

> +<!--
> +Author: Peter Geoghegan <pg@bowt.ie>
> +Branch: master [9b42e7137] 2019-05-13 10:27:59 -0700
> +Branch: REL_11_STABLE [bf78f50ba] 2019-05-13 10:27:57 -0700
> +-->
> +     <para>
> +      Avoid corruption of a btree index in the unlikely case that a failure
> +      occurs during key truncation during a page split (Peter Geoghegan)
> +     </para>

To me, this text implies a cautious DBA should amcheck every index.  Reading
the thread[1], I no longer think that.  It's enough to monitor that VACUUM
doesn't start failing persistently on any index.  I suggest replacing this
release note text with something like the following:

  Avoid writing erroneous btree index data that does not change query results
  but causes VACUUM to abort with "failed to re-find parent key".  Affected
  indexes are rare; REINDEX fixes them.

(I removed "key truncation during a page split" as being too technical for
release notes.)

[1] https://postgr.es/m/flat/CAH2-WzkcWT_-NH7EeL=Az4efg0KCV+wArygW8zKB=+HoP=VWMw@mail.gmail.com 



Re: Draft back-branch release notes are up for review

From
Peter Geoghegan
Date:
On Sat, Jun 15, 2019 at 1:39 PM Noah Misch <noah@leadboat.com> wrote:
> To me, this text implies a cautious DBA should amcheck every index.  Reading
> the thread[1], I no longer think that.  It's enough to monitor that VACUUM
> doesn't start failing persistently on any index.  I suggest replacing this
> release note text with something like the following:
>
>   Avoid writing erroneous btree index data that does not change query results
>   but causes VACUUM to abort with "failed to re-find parent key".  Affected
>   indexes are rare; REINDEX fixes them.
>
> (I removed "key truncation during a page split" as being too technical for
> release notes.)

I agree that this isn't terribly significant in general. Your proposed
wording seems better than what we have now, but a reference to INCLUDE
indexes also seems like a good idea. They are the only type of index
that could possibly have the issue with page deletion/VACUUM becoming
confused. Even then, the risk seems minor, because there has to be an
OOM at precisely the wrong point.

If there was any kind of _bt_split() OOM in 11.3 that involved a
non-INCLUDE B-Tree index, then the OOM could only occur when we
allocate a temp page buffer. I verified that this causes no
significant issue for VACUUM. It is best avoided, since we still
"leak" the new page/buffer, although that is almost harmless.

-- 
Peter Geoghegan



Re: Draft back-branch release notes are up for review

From
Peter Geoghegan
Date:
On Sat, Jun 15, 2019 at 2:11 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Sat, Jun 15, 2019 at 1:39 PM Noah Misch <noah@leadboat.com> wrote:
> > To me, this text implies a cautious DBA should amcheck every index.  Reading
> > the thread[1], I no longer think that.  It's enough to monitor that VACUUM
> > doesn't start failing persistently on any index.  I suggest replacing this
> > release note text with something like the following:

FWIW, amcheck won't help here. It can only access pages through its
breadth-first search, and so will not land on any "leaked" page (i.e.
page that has no link to the tree). Ideally, amcheck would notice that
it hasn't visited certain blocks, and then inspect the blocks/pages in
a separate pass, but that doesn't happen right now.

As you know, VACUUM can find leaked blocks/pages because nbtree VACUUM
has an optimization that allows it to access them in sequential order.

-- 
Peter Geoghegan



Re: Draft back-branch release notes are up for review

From
Noah Misch
Date:
On Sat, Jun 15, 2019 at 02:11:41PM -0700, Peter Geoghegan wrote:
> On Sat, Jun 15, 2019 at 1:39 PM Noah Misch <noah@leadboat.com> wrote:
> > To me, this text implies a cautious DBA should amcheck every index.  Reading
> > the thread[1], I no longer think that.  It's enough to monitor that VACUUM
> > doesn't start failing persistently on any index.  I suggest replacing this
> > release note text with something like the following:
> >
> >   Avoid writing erroneous btree index data that does not change query results
> >   but causes VACUUM to abort with "failed to re-find parent key".  Affected
> >   indexes are rare; REINDEX fixes them.
> >
> > (I removed "key truncation during a page split" as being too technical for
> > release notes.)
> 
> I agree that this isn't terribly significant in general. Your proposed
> wording seems better than what we have now, but a reference to INCLUDE
> indexes also seems like a good idea. They are the only type of index
> that could possibly have the issue with page deletion/VACUUM becoming
> confused.

If true, that's important to mention, yes.



Re: Draft back-branch release notes are up for review

From
Tom Lane
Date:
Noah Misch <noah@leadboat.com> writes:
> On Sat, Jun 15, 2019 at 02:11:41PM -0700, Peter Geoghegan wrote:
>> I agree that this isn't terribly significant in general. Your proposed
>> wording seems better than what we have now, but a reference to INCLUDE
>> indexes also seems like a good idea. They are the only type of index
>> that could possibly have the issue with page deletion/VACUUM becoming
>> confused.

> If true, that's important to mention, yes.

Thanks for the input, guys.  What do you think of

     Avoid writing an invalid empty btree index page in the unlikely case
     that a failure occurs while processing INCLUDEd columns during a page
     split (Peter Geoghegan)

     The invalid page would not affect normal index operations, but it
     might cause failures in subsequent VACUUMs. If that has happened to
     one of your indexes, recover by reindexing the index.

            regards, tom lane



Re: Draft back-branch release notes are up for review

From
Peter Geoghegan
Date:
On Sat, Jun 15, 2019 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thanks for the input, guys.  What do you think of
>
>      Avoid writing an invalid empty btree index page in the unlikely case
>      that a failure occurs while processing INCLUDEd columns during a page
>      split (Peter Geoghegan)
>
>      The invalid page would not affect normal index operations, but it
>      might cause failures in subsequent VACUUMs. If that has happened to
>      one of your indexes, recover by reindexing the index.

That seems perfect.

Thanks
-- 
Peter Geoghegan



Re: Draft back-branch release notes are up for review

From
Noah Misch
Date:
On Sat, Jun 15, 2019 at 06:05:00PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Sat, Jun 15, 2019 at 02:11:41PM -0700, Peter Geoghegan wrote:
> >> I agree that this isn't terribly significant in general. Your proposed
> >> wording seems better than what we have now, but a reference to INCLUDE
> >> indexes also seems like a good idea. They are the only type of index
> >> that could possibly have the issue with page deletion/VACUUM becoming
> >> confused.
> 
> > If true, that's important to mention, yes.
> 
> Thanks for the input, guys.  What do you think of
> 
>      Avoid writing an invalid empty btree index page in the unlikely case
>      that a failure occurs while processing INCLUDEd columns during a page
>      split (Peter Geoghegan)
> 
>      The invalid page would not affect normal index operations, but it
>      might cause failures in subsequent VACUUMs. If that has happened to
>      one of your indexes, recover by reindexing the index.

Looks good.