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
