Re: Refactoring of heapam code. - Mailing list pgsql-hackers

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