Re: WIP: Rework access method interface

On Sat, Jan 16, 2016 at 9:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> > [ aminterface-13.patch ]
>
> I've started to review this. There are a bunch of cosmetic things I don't
> like, notably the include-file nesting you've chosen, but they're fixable.
> One item that I think could use some discussion is where to put the new
> amvalidate functions. I don't especially like your choice to drop them
> into nbtree.c, gist.c, etc, for a couple of reasons:
>
> 1. These aren't really at the same semantic level as functions like
> btinsert or btgettuple; they're not part of the implementation of an
> index, and indeed are *users* of indexes (at least of the catalog
> indexes).
>
> 2. This approach substantially bloats the #include lists for the
> relevant files, which again is a token of the validate functions not
> belonging where they were put.
>
> 3. There's probably room to share code across the different validators;
> but this design isn't very amenable to that.
>
> A comparison point worth noting is that the amcostestimate functions
> are in more or less the same boat: they aren't part of the index
> implementation in any meaningful way, but are really part of the
> planner instead. Those are all in selfuncs.c, not under backend/access
> at all.
>
> There are a couple of things we could do instead:
>
> * Put each amvalidate function into its own file (but probably keep it
> in the same directory as now). This is a reasonable response to
> points #1 and #2 but isn't very much help for #3.
>

Shouldn't we try to move amhandler function as well along with
amvalidate? I think moving each am's handler and validate into
am specific new file can make this arrangement closer to what
we have for PL's (ex. we have plpgsql_validator and plpgsql_call_
handler in pl_handler.c and similar handler and validator functions
for other languages in their corresponding modules).

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com