On 2015-09-25 17:45, Alvaro Herrera wrote:
I think the API of getOpFamilyInfo is a bit odd; is the caller expected to fill intype and keytype always, and then it only sets the procs/opers lists? I wonder if it would be more sensible to have that routine receive the pg_opclass tuple (or even the opclass OID) instead of the opfamily OID. I think "amapi.h" is not a great file name. What about am_api.h?
Well we have related fdwapi.h and tsmapi.h (and several unrelated *api.h which also don't use "_" in the name) so amapi.h seems fine to me.
I'm unsure about BRIN_NPROC. Why did you set it to 15? It's not unthinkable that future opclass frameworks will have different numbers
The BRIN_NPROC should be probably defined in brin.c since it's only used for sizing local array variable in amvalidate and should be used to set amsupport in the init function as well then.
of support procs. For BRIN I'm thinking that we could add another support proc which validates the opclass definition using the specific framework; that way we will be able to check that the set of operators defined are correct, etc (something that the current approach cannot do).
As I said before in the thread I would prefer more granular approach to validation - have amvalidateopclass in the struct for the current functionality so that we can easily add more validators in the future. There can still be one amvalidate function exposed on SQL level that just calls all the amvalidate* functions that the am defines.
-- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
