Re: WIP: Covering + unique indexes.

On Tue, Apr 12, 2016 at 9:14 AM, Anastasia Lubennikova
<a(dot)lubennikova(at)postgrespro(dot)ru> wrote:
> Sooner or later, I'd like to see this patch finished.

Me, too.

> For now, it has two complaints:
> - support of expressions as included columns.
> Frankly, I don't understand, why it's a problem of the patch.
> The patch is already big enough and it will be much easier to add
> expressions support in the following patch, after the first one will be
> stable.
> I wonder, if someone has objections to that?

Probably. If we limit the scope of something, it's always in a way
that limits the functionality available to users, rather than limits
how generalized the new functionality is, and so cutting scope
sometimes isn't possible. There is a very high value placed on
features working well together. A user ought to be able to rely on the
intuition that features work well together. Preserving that general
ability for users to guess correctly what will work based on what they
already know is seen as important.

For example, notice that the INSERT documentation allows UPSERT unique
index inference to optionally accept an opclass or collation. So far,
the need for this functionality is totally theoretical (in practice
all B-Tree opclasses have the same idea about equality across a given
type, and we have no case insensitive collations), but it's still
there. Making that work was not a small effort (there was a follow-up
bugfix commit just for that, too). This approach is mostly about
making the implementation theoretically sound (or demonstrating that
it is) by considering edge-cases up-front. Often, there will be
benefits to a maximally generalized approach that were not initially
anticipated by the patch author, or anyone else.

I agree that it is difficult to uphold this standard at all times, but
there is something to be said for it. Postgres development must have a
very long term outlook, and this approach tends to make things easier
for future patch authors by making the code more maintainable. Even if
this is the wrong thing in specific cases, it's sometimes easier to
just do it than to convince others that their concern is misplaced in
this one instance.

> Yes, it's a kind of delayed feature. But should we wait for every patch when
> it will be entirely completed?

I think that knowing where and how to cut scope is an important skill.
If this question is asked as a general question, then the answer must
be "yes". I suggest asking a more specific question. :-)

> - lack of review and testing
> Obviously I did as much testing as I could.
> So, if reviewers have any concerns about the patch, I'm waiting forward to
> see them.

For what it's worth, I agree that you put a great deal of effort into
this patch, and it did not get in to 9.6 because of a collective
failure to focus minds on the patch. Your patch was a credible
attempt, which is impressive when you consider that the B-Tree code is
so complicated. There is also the fact that there is now a very small
list of credible reviewers for B-Tree patches; you must have noticed
that not even amcheck was committed, even though I was asked to
produce a polished version in February during the FOSDEM dev meeting,
and even though it's just a contrib module that is totally orientated
around finding bugs and so on. I'm not happy about that either, but
that's just something I have to swallow.

I fancy myself as am expert on the B-Tree code, but I've never managed
to make an impact in improving its performance at all (I've never made
a serious effort, but have had many ideas). So, in case it needs to be
said, I'll say it: You've chosen a very ambitious set of projects to
work on, by any standard. I think it's a good thing that you've been
ambitious, and I don't suggest changing that, since I think that you
have commensurate skill. But, in order to be successful in these
projects, patience and resolve are very important.

--
Peter Geoghegan