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

Some comments anyways on the first patch:

1. It does not apply cleanly with git-apply - many white space errors

2. I don't understand the difference between PageGetItemHeapHeaderOnly() and PageGetItemHeap(). They seem to do exactly the same thing and can be used interchangeably.

3. While I like the idea of using separate interfaces to get heap/index tuple from a page, may be they can internally use a common interface instead of duplicating what PageGetItem() does already.

4. Should we rename PageGetItemHeap() to PageGetHeapTuple() or something like that?

5. If we do this, we should probably have corresponding wrapper functions/macros for remaining callers of PageGetItem()

I also looked at the refactoring design doc. Looks like a fine approach to me, but the API will need much more elaborate discussions. I am not sure if the interfaces as presented are enough to support everything that even heapam can do today. And much more thoughts will be required to ensure we don't miss out things that new primary access method may need.

A few points regarding how the proposed API maps to heapam:

- How do we support parallel scans on heap?

- Does the primary AM need to support locking of rows?

- Would there be separate API to interpret the PAM tuple itself? (something that htup_details.h does today). It seems natural that every PAM will have its own interpretation of tuple structure and we would like to hide that inside PAM implementation.

- There are many upper modules that need access to system attributes (xmin, xmax for starters). How do you plan to handle that? You think we can provide enough abstraction so that the callers don't need to know the tuple structures of individual PAM?

I don't know what to do with the CF entry itself. I could change the status to "waiting on author" but then the design draft may not get enough attention. So I'm leaving it in the current state for others to look at.

Thanks,

Pavan