Further info about picksplit-doesn't-support-secondary-split whining - Mailing list pgsql-hackers

I've spent the afternoon reading and documenting gistsplit.c, and
finally feel like I understand what's going on in there, particularly
with respect to the secondary-split logic of log-spam infamy.  I'll be
committing the results of that in a bit, but meanwhile: what I now
realize is that the "fallback" secondary split implementation in
supportSecondarySplit() is not especially awful, and opclass PickSplit
functions would generally need fairly invasive changes to do better than
that.  They'd need to try to build out from a pair of predetermined
union values, rather than selecting a split entirely on their own.
The comment I'm going to add about this looks like so:

/** This is the Split Vector to be returned by the PickSplit method.* PickSplit should fill the indexes of tuples to go
tothe left side into* spl_left[], and those to go to the right into spl_right[] (note the method* is responsible for
palloc'ingboth of these arrays!).  The tuple counts* go into spl_nleft/spl_nright, and spl_ldatum/spl_rdatum must be
setto* the union keys for each side.** If spl_ldatum_exists or spl_rdatum_exists is true, then we are performing* a
"secondarysplit" using a non-first index column.  In this case some* decisions have already been made about a page
split,and the set of tuples* being passed to PickSplit is just the tuples about which we are undecided.*
spl_ldatum/spl_rdatumthen contain the union keys for the tuples already* chosen to go left or right.  Ideally the
PickSplitmethod should take those* keys into account while deciding what to do with the remaining tuples.* In any case,
itshould union the given tuples' keys into the existing* spl_ldatum/spl_rdatum values rather than just setting those
valuesfrom* scratch, and then set spl_ldatum_exists/spl_rdatum_exists to false to show* it has done this.** If the
PickSplitmethod fails to clear spl_ldatum_exists/spl_rdatum_exists,* the core GiST code will make its own decision
abouthow to merge the* secondary-split results with the previously-chosen tuples, and will then* recompute the union
keysproperly.  This is a workable though often not* optimal approach.*/
 

Looking around shows that there is only one opclass PickSplit function
that makes any attempt to support secondary split directly, namely
gist_box_picksplit; and it's only handling the case in its fallbackSplit
code path, which is hopefully very seldom taken; and the code that it
has there is actually stupider than the "fallback" implementation in
gistsplit.c, because it's *not* making any attempt to build out from the
given ranges, it's just adding them in after the fact.  gistsplit.c can
do that for itself.  What's more, it takes the trouble to see which half
of the secondary split goes better with which half of the primary split,
useful logic that is being short-circuited by the inferior code in
gist_box_picksplit.

So IMO that's completely broken, and we should just rip out lines
226-234 of gistproc.c.

That would leave us with no, zero, opclasses that don't provoke the 
"picksplit method doesn't support secondary split" griping from
gistsplit.c.  Which makes one question why that message is there at all,
much less at LOG level.  It may have been intended to spur development
of smarter PickSplit functions, but since nothing has been done towards
that in over five years, I'm not holding my breath for it to happen.

I now think we should just remove that message entirely, and in the back
branches too.  There's no reason to expect that we'll ever have all the
GiST opclasses upgraded to do secondary splits internally.

Comments?
        regards, tom lane