Teodor Sigaev wrote:
> >I'm OK about continuing work on amvalidate if we can build consuensus on its design.
> >Could you give some feedback on amvalidate version of patch please?
> >http://www.postgresql.org/message-id/CAPpHfds8ZyWenz9vW6tE5RZXboL1vU_wSW181vEq+mU+v1dsiw@mail.gmail.com
>
> In attach a bit modified patch based on 4-th version, and if there are no
> strong objections, I will commit it. Waiting this patch stops Alexander's
> development on CREATE ACCESS METHOD and he needs to move forward.
I think the messages in ereport() need some work -- at the bare minimum,
do not uppercase the initial. Also things such as whether operation
names such as "order by" ought to be uppercase or in quotes, for example
here:
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("BRIN doesn't support order by operators")));
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?
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
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).
I think another pgindent run would be good -- there's some broken
whitespace here and there.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
