Обсуждение: best place for "rtree" strategy numbers

In Emre Hasegeli's patch for range/inet/geometry opclasses for BRIN, he
chose to move strategy numbers from gist.h to a more central place.  He
chose skey.h because that's where btree strategy numbers are defined,
but I'm not sure I agree with that choice.

It's been clear for a while now that these numbers are not gist-only
anyway; spgist and GIN also make use of them, and now brin inclusion
opclass infrastructure also wants to use the same numbers.  This is the
reason for a central place, anyway.  However, we find datatype specific
definitions in places such as rangetypes.h:

/* Operator strategy numbers used in the GiST and SP-GiST range opclasses */
/* Numbers are chosen to match up operator names with existing usages */
#define RANGESTRAT_BEFORE               1
#define RANGESTRAT_OVERLEFT             2
[ .. etc .. ]

and network_gist.c:

/** Operator strategy numbers used in the GiST inet_ops opclass*/
#define INETSTRAT_OVERLAPS      3
#define INETSTRAT_EQ            18
#define INETSTRAT_NE            19
[ .. etc .. ]

Obviously, the expectation is that the numbers are not needed outside
the opclass implementation itself.  I think that's a lost cause, mainly
because we want to have consistent operator names across datatypes.

Therefore I would like to move those numbers elsewhere.  Maybe we could
have a new file which would be used only for strategy numbers, such as
src/include/access/stratnum.h, and we would have something like this,
and redefine the ones elsewhere to reference those.  For instance, 
rangetypes.h could look like this:

/* Operator strategy numbers used in the GiST and SP-GiST range opclasses */
/* Numbers are chosen to match up operator names with existing usages */
#define RANGESTRAT_BEFORE                RTLeftStrategyNumber
#define RANGESTRAT_OVERLEFT                RTOverLeftStrategyNumber
#define RANGESTRAT_OVERLAPS                RTOverlapStrategyNumber

and so on.  (I am a bit hesitant about using the "RT" prefix in those
symbols, since the set has grown quite a bit from R-Tree alone.  But I
don't have any ideas on what else to use either.)

I can, of course, just leave these files well enough alone and just rely
on the numbers not changing (which surely they won't anyway) and
remaining consistent with numbers used in new opclasses (which surely
they will lest they be born inconsistent with existing ones).

Opinions?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> In Emre Hasegeli's patch for range/inet/geometry opclasses for BRIN, he
> chose to move strategy numbers from gist.h to a more central place.  He
> chose skey.h because that's where btree strategy numbers are defined,
> but I'm not sure I agree with that choice.

Yeah, putting those in skey.h was probably not such a great idea.
(IIRC, they didn't use to be there ... it's probably my fault that
they're there now.)

> Therefore I would like to move those numbers elsewhere.  Maybe we could
> have a new file which would be used only for strategy numbers, such as
> src/include/access/stratnum.h, and we would have something like this,
> and redefine the ones elsewhere to reference those.

+1

> I can, of course, just leave these files well enough alone and just rely
> on the numbers not changing (which surely they won't anyway) and
> remaining consistent with numbers used in new opclasses (which surely
> they will lest they be born inconsistent with existing ones).

Yeah, we do have checks in opr_sanity which will complain if inconsistent
strategy numbers are used for similarly-named operators.  That's a pretty
weak test though, and operator names aren't exactly the right thing to
check anyway.  I'd be good with pushing all of that stuff to a new central
header.
        regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> > I can, of course, just leave these files well enough alone and just rely
> > on the numbers not changing (which surely they won't anyway) and
> > remaining consistent with numbers used in new opclasses (which surely
> > they will lest they be born inconsistent with existing ones).
>
> Yeah, we do have checks in opr_sanity which will complain if inconsistent
> strategy numbers are used for similarly-named operators.  That's a pretty
> weak test though, and operator names aren't exactly the right thing to
> check anyway.  I'd be good with pushing all of that stuff to a new central
> header.

So here's a patch for this.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvaro.herrera@2ndquadrant.com> writes:
> So here's a patch for this.

Looks reasonable to me (though I only eyeballed it, not tested).

Do we want to push this into 9.5, or wait for 9.6?
        regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvaro.herrera@2ndquadrant.com> writes:
> > So here's a patch for this.
> 
> Looks reasonable to me (though I only eyeballed it, not tested).
> 
> Do we want to push this into 9.5, or wait for 9.6?

My intention is to push this now, before pushing brin inclusion.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Adjacent strategy number of range types don't match the central
definitions.  Attached patch changes it.

Also, maybe it is better to include stratnum.h on rangetypes.h and
inet.h rather than the .c files as the definitions are used in the
headers.  I wouldn't re-define them, anyway.  Two definitions just to
say 3 is too much for my taste.