Hello, David!
> I see you opted to only jumble the low-order byte of the pending null
> counter. I used the full 4 bytes as I don't think the extra 3 bytes is
> a problem. Witrh v7 the memcpy to copy the pending_nulls into the
> buffer is inlined into a single instruction. It's semi-conceivable
> that you could have more than 255 NULLs given that a List can contain
> Nodes. I don't know if we ever have any NULL items in Lists at the
> moment, but I don't think that that's disallowed. I'd feel much safer
> taking the full 4 bytes of the counter to help prevent future
> surprises.
Yes _jumbleList could theoretically handle cases with over 256 Node elements in same list,
but most (or all) of them is non-NULL (as I know only PlannedStmt->subplans accepts NULL elements),
Most of the foreach cycles over query lists don't have any NULL safety checks;
we just assume that the lists contain no NULL pointers.
I still believe storing just one byte is sufficient.
> I'm happy to proceed with the v7 version. I'm also happy to credit you
> As the primary author of it, given that you came up with the v2b
> patch. It might be best to extract the performance stuff that's in v7
> and apply that separately from the bug fix. If you're still interested
> and have time in the next 7 hours to do it, would you be able to split
> the v7 as described, making v8-0001 as the bug fix and v8-0002 as the
> performance stuff? If so, could you also add the total_jumble_len to
> v8-0001 like Michael's last patch had, but adjust so it's only
> maintained for Assert builds. Michael is quite keen on having more
> assurance that custom jumble functions don't decide to forego any
> jumbling.
As I can see, you have already split the patches. I apologize for the delay; I don't have
much time to work on this issue. Thank you for your proposal to credit me as the patch author.
