Hi, On 2019-04-22 12:33:24 -0400, Tom Lane wrote: > ISTM that Michael's proposed wording change shows that the existing > comment is easily misinterpreted. I don't think these minor grammatical > fixes will avoid the misinterpretation problem, and so some more-extensive > rewording is called for.
Fair enough. > But TBH, now that I look at the code, I think the entire optimization > is a bad idea and should be removed. Am I right in thinking that the > presence of a wrong attnotnull marker could cause the generated code to > actually crash, thanks to not checking the tuple's natts field? I don't > have enough faith in our enforcement of those constraints to want to see > JIT taking that risk to save a nanosecond or two. It's not a minor optimization, it's very measurable. Without the check there's no pipeline stall when the memory for the tuple header is not in the CPU cache (very common, especially for seqscans and such, due to the "backward" memory location ordering of tuples in seqscans, which CPUs don't predict). Server grade CPUs of the last ~5 years just march on and start the work to fetch the first attributes (especially if they're NOT NULL) - but can't do that if natts has to be checked. And starting to check the NULL bitmap for NOT NULL attributes, would make that even worse - and would required if we don't trust attnotnull. > (Possibly I'd not think this if I weren't fresh off a couple of days > with my nose in the ALTER TABLE SET NOT NULL code. But right now, > I think that believing that that code does not and never will have > any bugs is just damfool.) But there's plenty places where we rely on NOT NULL actually working? We'll return wrong query results, and even crash in non-JIT places because we thought there was guaranteed to be datum? Greetings, Andres Freund
