On Sun, Feb 5, 2023 at 3:57 AM Himanshu Upadhyaya <upadhyaya.himanshu@gmail.com> wrote: > Thanks, yes it's working fine with Prepared Transaction. > Please find attached the v9 patch incorporating all the review comments.
I don't know quite how we're still going around in circles about this, but this code makes no sense to me at all:
/* * Add data to the predecessor array even if the current or * successor's LP is not valid. We will not process/validate these * offset entries while looping over the predecessor array but * having all entries in the predecessor array will help in * identifying(and validating) the Root of a chain. */ if (!lp_valid[ctx.offnum] || !lp_valid[nextoffnum]) { predecessor[nextoffnum] = ctx.offnum; continue; }
If the current offset number is not for a valid line pointer, then it makes no sense to talk about the successor. An invalid redirected line pointer is one that points off the end of the line pointer array, or to before the beginning of the line pointer array, or to a line pointer that is unused. An invalid line pointer that is LP_USED is one which points to a location outside the page, or to a location inside the page. In none of these cases does it make any sense to talk about the next tuple. If the line pointer isn't valid, it's pointing to some invalid location where there cannot possibly be a tuple. In other words, if lp_valid[ctx.offnum] is false, then nextoffnum is a garbage value, and therefore referencing predecessor[nextoffnum] is useless and dangerous.
If the next offset number is not for a valid line pointer, we could in theory still assign to the predecessor array, as you propose here. In that case, the tuple or line pointer at ctx.offnum is pointing to the line pointer at nextoffnum and that is all fine. But what is the point? The comment claims that the point is that it will help us identify and validate the root of the hot chain. But if the line pointer at nextoffnum is not valid, it can't be the root of a hot chain. When we're talking about the root of a HOT chain, we're speaking about a tuple. If lp_valid[nextoffnum] is false, there is no tuple. Instead of pointing to a tuple, that line pointer is pointing to garbage.
Initially while implementing logic to identify the root of the HOT chain
I was getting crash and regression failure's that time I thought of having
this check along with a few other changes that were required,
but you are right, it's unnecessary to add data to the predecessor
array(in this case) and is not required. I am removing this from the patch.