Re: [PATCH] btree_gist: add cross-type integer operator support for GiST

Alexander Nestorov <alexandernst(at)gmail(dot)com> writes:
> Attaching v7 patchset.

I spent a bit of time looking at v7. I think you have made the wrong
choice here:

> To let the cross-type path reuse gbt_num_consistent(), all comparison
> callbacks are now invoked as f(query, key): query on the left, key on
> the right

The index AMs are generally built on the principle that an index
condition looks like "index_key OP query_value", which is why
all the code looks like it does. So I object to changes like

case BTLessEqualStrategyNumber:
if (is_leaf)
- retval = tinfo->f_le(key->lower, query, flinfo);
+ retval = tinfo->f_ge(query, key->lower, flinfo);
else
retval = lower_is_below_query();
break;

It is confusing and error-prone for the code to be written exactly
backwards from the strategy code name. So I think you should undo
all those changes and instead change whatever you were trying to
avoid changing. I really doubt that that will lead to a messier
patch. (Maybe it'd be smart to split 0001 into two steps, one
that syncs whatever order-of-parameters problem you see and then
one that adds cross-type stuff on top of that.)

I'm also confused by all the names involving "q4" and the like.
Maybe that conveys something to your mind, but it conveys nothing
to mine, and I don't see any comments explaining the naming
convention. It'd probably be good to use slightly longer names.

Lastly, please avoid making random whitespace changes in places
away from your code changes. Those just distract reviewers,
and it's not usually apparent that the change is an improvement
anyway. Revising line breaks for no very good reason is likewise
a drag on reviewers' attention.

regards, tom lane