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+v1d...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers