Re: tableam vs. TOAST

On 2019-11-08 17:59, Robert Haas wrote:
> OK. Could you see what you think of the attached patches? 0001 does
> some refactoring of toast_fetch_datum() and toast_fetch_datum_slice()
> to make them look more like each other and clean up a bunch of stuff
> that I thought was annoying, and 0002 then pulls out the common logic
> into a heap-specific function. If you like this direction, we could
> then push the heap-specific function below tableam, but I haven't done
> that yet.

Partial review: The 0001 patch seems very sensible.  Some minor comments 
on that:

Perhaps rename the residx variable (in both functions).  You have gotten 
rid of all the res* variables except that one.  That name as it is right 
now isn't very helpful at all.

You have collapsed the error messages for "chunk %d of %d" and "final 
chunk %d" and replaced it with just "chunk %d".  I think it might be 
better to keep the "chunk %d of %d" wording, for more context, or was 
there a reason why you wanted to remove the total count from the message?

I believe this assertion

+   Assert(endchunk <= totalchunks);

should be < (strictly less).

In the commit message you state that this assertion replaces a run-time 
check, but I couldn't quite make out which one you are referring to 
because all the existing run-time checks are kept, with slightly 
refactored conditions.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services