Re: 2019-03 CF Summary / Review - Tranche #2

On Fri, Feb 15, 2019 at 9:45 PM Andres Freund <andres@anarazel.de> wrote:
> - Make nbtree keys unique by appending heap TID, suffix truncation
>
>   NR: Important, seemingly carefully worked on feature. But it's
>   *complicated* stuff, and there's only been a bit of design review by
>   Heikki. The author's a committer.  I don't feel qualified to judge
>   this.

I think that's fair. The best way of understanding the risks as a
non-expert is to think of the patch as having two distinct components:

1. Make nbtree entries unique + add suffix truncation -- the basic mechanism.

2. Make more intelligent decisions about where to split pages, to work
with suffix truncation, while still mostly caring about the balance of
free space among each half of the split, and caring about a number of
other new concerns besides these two.

You're right that some parts of the patch are very complicated, but
those are all contained in the second component. That has been the
main focus of Heikki's review, by far. This second component is
concerned with picking a split point that is already known to be legal
based on the *existing* criteria. If there were bugs here, they could
not result in data corruption. The worst outcome I can conceive of is
that an index would be bloated in a new and novel way. It would be
possible to correct that in a point release without breaking on-disk
compatibility. That would be painful, certainly, but it's far from the
worst outcome.

Granted, there are also one or two subtle things in the first, more
critical component, but these are also the things that were
established earliest and have received the most testing. And, amcheck
is now capable of doing point lookups using the same code paths as
index scans (calling _bt_search()) to relocate each and every tuple on
the leaf level, starting from the root. The first component does not
change anything about how crash recovery or VACUUM works, either. It's
all about how keys compare, and how new pivot tuples are generated --
it's mostly about the key space, while changing very little about the
physical on-disk representation. (It builds on the on-disk
representation changes added in Postgres 11, for INCLUDE indexes.)

> - SortSupport implementation on inet/cdir
>
>   WOA: This is a substantial amount of code submitted first for the last
>   CF.
>
>   Andres: punt to v13

I was kind of involved here. I think that it's fair to punt, based on
the rule about submitting a big patch to the last CF.

> - amcheck verification for GiST
>
>   WOA: Some locking changes are apparently needed, possibly a bit too
>   much to fix up for v12?

I had hoped that Andrey Borodin would get back to me on this soon. It
does still seem unsettled.

> - insensitive/non-deterministic collations
>
>   NR: Peter has stated that he's targeting v12, but I'm not sure this
>   had enough review? But it's not *that* complicated...

I could help out here.

--
Peter Geoghegan