Thread: Draft back-branch release notes are up for review
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
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
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
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
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.
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
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
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.