Re: Refactoring of heapam code.

08.08.2016 12:43, Anastasia Lubennikova:
> 08.08.2016 03:51, Michael Paquier:
>> On Sat, Aug 6, 2016 at 2:56 AM, Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>> Anastasia Lubennikova wrote:
>>>> So there is a couple of patches. They do not cover all mentioned
>>>> problems,
>>>> but I'd like to get a feedback before continuing.
>>> I agree that we could improve things in this general area, but I do not
>>> endorse any particular changes you propose in these patches; I haven't
>>> reviewed your patches.
>> Well, I am interested in the topic. And just had a look at the
>> patches proposed.
>>
>> + /*
>> + * Split PageGetItem into set of different macros
>> + * in order to make code more readable.
>> + */
>> +#define PageGetItemHeap(page, itemId) \
>> +( \
>> + AssertMacro(PageIsValid(page)), \
>> + AssertMacro(ItemIdHasStorage(itemId)), \
>> + (HeapTupleHeader)(((char *)(page)) + ItemIdGetOffset(itemId)) \
>> +)
>> Placing into bufpage.h macros that are specific to heap and indexes is
>> not that much a good idea. I'd suggest having the heap ones in
>> heapam.h, and the index ones in a more generic place, as
>> src/access/common as already mentioned by Alvaro.
>>
>> + onpage_tup = PageGetItemHeapHeaderOnly(page, newitemid);
>> Just PageGetItemHeapHeader would be fine.
>>
>> @@ -2324,7 +2087,6 @@ FreeBulkInsertState(BulkInsertState bistate)
>> pfree(bistate);
>> }
>>
>> -
>> /*
>> * heap_insert - insert tuple into a heap
>> Those patches have some noise.
>>
>> Patch 0002 is doing two things: reorganizing the order of the routines
>> in heapam.c and move some routines from heapam.c to hio.c. Those two
>> could be separate patches/
>>
>> If the final result is to be able to extend custom AMs for tables, we
>> should have a structure like src/access/common/index.c,
>> src/access/common/table.c and src/access/common/relation.c for
>> example, and have headers dedicated to them. But yes, there is
>> definitely a lot of room for refactoring as we are aiming, at least I
>> guess so, at having more various code paths for access methods.
>
> Thank you for the review, I'll fix these problems in final version.
>
> Posting the first message I intended to raise the discussion. Patches
> were attached mainly to illustrate the problem and to be more concrete.
>
> Thank you for attention and feedback.
> Since there are no objections to the idea in general, I'll write an
> exhaustive
> README about current state of the code and then propose API design
> to discuss details.
>
> Stay tuned.
>

Here is the promised design draft.
https://wiki.postgresql.org/wiki/HeapamRefactoring

Looking forward to your comments.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company