From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
cc: Andres Freund <andres@anarazel.de>, Michael Paquier <michael@paquier.xyz>,
        Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@postgresql.org
Subject: Re: clean up docs for v12
In-reply-to: <20190422161955.GA17411@alvherre.pgsql>
References: <20190422161955.GA17411@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@2ndquadrant.com>
	message dated "Mon, 22 Apr 2019 12:19:55 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <16489.1555950804.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 22 Apr 2019 12:33:24 -0400
Message-ID: <16490.1555950804@sss.pgh.pa.us>
Precedence: bulk

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 availabl=
e in
>>> -	 * tuple. If so, we can start deforming. If not, need to make sure t=
o
>>> -	 * 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 th=
e
>>> +	 * 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 NUL=
L
>> 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