Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Apr-22, Andres Freund wrote:
>> On 2019-04-22 14:48:26 +0900, Michael Paquier wrote:
>>> /*
>>> - * Check if's guaranteed the all the desired attributes are available in
>>> - * tuple. If so, we can start deforming. If not, need to make sure to
>>> - * fetch the missing columns.
>>> + * Check if all the desired attributes are available in the tuple. If so,
>>> + * we can start deforming. If not, we need to make sure to fetch the
>>> + * missing columns.
>>> */
>> That's imo not an improvement. The guaranteed bit is actually
>> relevant. What this block is doing is eliding the check against the
>> tuple header for the number of attributes, if NOT NULL attributes for
>> later columns guarantee that the desired columns are present in the NULL
>> bitmap. But the rephrasing makes it sound like we're actually checking
>> against the tuple.
>>
>> I think it'd be better just to fix s/the all/that all/.
> (and s/if's/if it's/)
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.
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.
(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.)
regards, tom lane
