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
