Re: tableam vs. TOAST

On Thu, Nov 21, 2019 at 5:37 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Partial review: The 0001 patch seems very sensible.  Some minor comments
> on that:

Thanks for the review. Updated patches attached. This version is more
complete than the last set of patches I posted.  It looks like this:

0001 - Lets a table AM that needs a toast table choose the AM that
will be used to implement the toast table.
0002 - Refactoring and code cleanup for TOAST code.
0003 - Move heap-specific portion of logic refactored by previous
patch to a separate function.
0004 - Lets a table AM arrange to call a different function when
detoasting, instead of the one created by 0003.

> 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.

OK, I renamed residx to curchunk and nextidx to expectedchunk.

> 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?

No, not really. Adjusted.

> I believe this assertion
>
> +   Assert(endchunk <= totalchunks);
>
> should be < (strictly less).

I think you're right. Fixed.

> 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.

Pre-patch, there is this condition:

-        if ((residx != nextidx) || (residx > endchunk) || (residx < startchunk)

This checks that the expected chunk number is equal to the one we
want, but not greater than the last one we were expecting nor less
than the first one we were expecting. The check is redundant if you
suppose that we never compute nextidx so that it is outside the bounds
of startchunk..endchunk. Since nextidx (or expectedchunk as these
patches now call it) is initialized to startchunk and then only
incremented, it seems impossible for the first condition to ever fail,
so it is no longer tested there. The latter chunk is also no longer
tested there; instead, we do this:

+    Assert(endchunk < totalchunks);

That's what the commit message is on about.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company