On Sat, Jan 16, 2016 at 9:43 PM, 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. >
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
