From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> |
Cc: | Petr Jelinek <petr(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Teodor Sigaev <teodor(at)sigaev(dot)ru>, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Robert Haas <robertmhaas(at)gmail(dot)com> |
Subject: | Re: WIP: Rework access method interface |
Date: | 2016-01-16 16:13:06 |
Message-ID: | 30257.1452960786@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
* 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
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2016-01-16 17:14:19 | Re: WIP: Rework access method interface |
Previous Message | Tom Lane | 2016-01-16 15:28:26 | Re: [GENERAL] Re: [BUGS] about test_parser installation failure problem(PostgreSQL in 9.5.0)? |