On Thu, Nov 21, 2019 at 5:37 AM Peter Eisentraut <peter.eisentr...@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
v9-0002-Code-cleanup-for-toast_fetch_datum-and-toast_fetc.patch
Description: Binary data
v9-0003-Move-heap-specific-detoasting-logic-into-a-separa.patch
Description: Binary data
v9-0004-tableam-New-callback-relation_fetch_toast_slice.patch
Description: Binary data
v9-0001-tableam-Allow-choice-of-toast-AM.patch
Description: Binary data
