05.08.2016 20:56, Alvaro Herrera:
Anastasia Lubennikova wrote:
Working on page compression and some other issues related to
access methods, I found out that the code related to heap
looks too complicated. Much more complicated, than it should be.
Since I anyway got into this area, I want to suggest a set of improvements.
Hm. I'm hacking on heapam.c pretty heavily ATM. Your first patch
causes no problem I think, or if it does it's probably pretty minor, so
that's okay. I'm unsure about the second one -- I think it's okay too,
because it mostly seems to be about moving stuff from heapam.c to hio.c
and shuffling some code around that I don't think I'm modifying.
I'm sure these patches will not cause any problems. The second one is
mostly about moving unrelated stuff from heapam.c to hio.c and renaming
couple of functions in heap.c.
Anyway, the are not a final solution just proof of concept.
By the way, I thought about looking at the mentioned patch and
probably reviewing it, but didn't find any of your patches on the
current commitfest. Could you point out the thread?
Also I think that it's quite strange to have a function heap_create(), that
is called
from index_create(). It has absolutely nothing to do with heap access
method.
Agreed. But changing its name while keeping it in heapam.c does not
really improve things enough. I'd rather have it moved elsewhere that's
not specific to "heaps" (somewhere in access/common perhaps). However,
renaming the functions would break third-party code for no good reason.
I propose that we only rename things if we also break it for other
reasons (say because the API changes in a fundamental way).
Yes, I agree that it should be moved to another file.
Just to be more accurate: it's not in heapam.c now, it is in
"src/backend/catalog/heap.c" which requires much more changes
than I did.
Concerning to the third-party code. It's a good observation and we
definitely should keep it in mind. Nevertheless, I doubt that there's a
lot of
external calls to these functions. And I hope this thread will end up with
fundamental changes introducing clear API for all mentioned problems.
One more thing that requires refactoring is "RELKIND_RELATION" name.
We have a type of relation called "relation"...
I don't see us fixing this bit, really. Historical baggage and all
that. For example, a lot of SQL queries use "WHERE relkind = 'r'". If
we change the name, we should probably also change the relkind constant,
and that would break the queries. If we change the name and do not
change the constant, then we have to have a comment "we call them
RELKIND_TABLE but the char is r because it was called RELATION
previously", which isn't any better.
Good point.
I'd rather change it to RELKIND_BASIC_RELATION or something like that,
which is more specific and still goes well with 'r' constant. But minor
changes
like that can wait until we clarify the API in general.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
