Re: Refactoring of heapam code.

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.

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