On Jan 16, 2016, at 11:13 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Alexander Korotkov <a.korot...@postgrespro.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.
> * Collect the amvalidate functions into one file, which then leaves
> us wondering where to put that; surely not under any one AM's directory.
> A new file in src/backend/access/index/ is one plausible solution.
> This file would also be a reasonable place to put the amvalidate()
> dispatch function itself.
> I'm somewhat leaning to the second choice, but perhaps someone has
> a better idea, or an argument against doing that.

Doesn't seem very modular.  How about putting common code there but AM-specific 
code in each AM's directory?  It would be nice if adding a new AM meant mostly 
adding a new directory, not much touching the common code.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to