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