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

Hi Alexander,

Thank you for bringing this thread to my attention, this is a very
interesting proposal. Personally, I have some doubts about the way
cross-type operators are handled in GiST, but this is another issue.
Within the current framework, this proposal implements things
correctly, so I have no conceptual issue with it.

Concerning the implementation itself, I have a couple comments:

1. In the consistent functions, I believe `subtype` can never be
`InvalidOid` based on the current code. It probably doesn't hurt to
keep it in the condition, but personally I'd drop it. Either way, I
thought it was worth mentioning.

2. I'm not sure I like the approach of casting everything to an
`int64` and using a new `gbt_int_consistent_x` function. It works,
so if others have no problem with it I can accept it, but I have
another suggestion: re-use `gbt_num_consistent` even for cross-type
cases by creating cross-type `gbtree_ninfo` objects with cross-type
`f_gt`, `f_ge`, `f_eq`, etc. functions. I have attached a v3
patchset with patch 0003 containing my suggestions. Feel free to
take them or leave them as you see fit. This suggestion requires
one important change in `gbt_num_consistent`:

```
else
- retval = (tinfo->f_le(key->lower, query, flinfo) &&
+ retval = (tinfo->f_ge(query, key->lower, flinfo) &&
tinfo->f_le(query, key->upper, flinfo));
break;
```

This is to make sure that `gbt_num_consistent` call all the
comparison functions with `query` on the left and `key->lower/upper`
on the right, to allow for cross-type comparison functions in `tinfo`.

0003 keeps your SQL/catalog work unchanged, compiles cleanly, and
passes the existing btree_gist regression including the cross-type
cases in 0002 (with the SRF-based catalog-drift check dropped, per
point 3).

3. This is related to my suggestion in 2. as it removes the
`gbt_int_crosstype_subtypes` SQL function (there is no
`gbt_int_crosstype_table` anymore in C). Instead, the `switch`
clause in the consistent functions raises an error in its default
path, so RHS types that are not handled can be caught quickly. As
you mention, I don't think there is a way to catch such issues at
index validation time, so this is the best I could think of.

Note that in theory we could put the `gbtree_ninfo` objects in a
table and then write a function equivalent to
`gbt_int_crosstype_subtypes` to still have the test you suggested,
but I think that switch statements are a bit clearer and perhaps
more efficient than looping through a table for the dispatch.

To me this is similar to adding new same-type operators to a GiST
opclass. Nothing checks at validation time that all the operators
in the opclass are effectively handled by the given consistent
function. So I don't see it as a problem to have the same behaviour
for the cross-type operators.

Other than that, this is a good proposal and definitely something
worth having in postgres.
Please let me know what you think of my suggestions.

Best regards,
Maxime