Re: clean up docs for v12

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