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.
