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