Re: WIP: Rework access method interface

On Sat, Sep 26, 2015 at 12:55 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> 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.

Makes sense. I leave it amapi.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
>>
>
> 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.

Problem is that in BRIN, access method use only first 4 support procedures.
However, operator class can define more and call them from first 4 support
procedures. That allows operator classes to re-use same support procedures
and build additional levels of abstractions. I've
declared BRIN_MANDATORY_NPROCS, and brinvalidate checks only their
presence. We could check BRIN opclass more precisely, by introducing new
support procedure for validation. I think it's a subject of a separate
patch since it would change interface of BRIN.

> 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.

I agree about staying with one SQL-visible function.

Other changes:
* Documentation reflects interface changes.
* IndexAmRoutine moved from CacheMemoryContext to indexcxt.
* Various minor formatting improvements.
* Error messages are corrected.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company