agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: clean up docs for v12
Date: Mon, 22 Apr 2019 12:33:24 -0400
Message-ID: <16490.1555950804@sss.pgh.pa.us> (raw)
In-Reply-To: <20190422161955.GA17411@alvherre.pgsql>
References: <20190422161955.GA17411@alvherre.pgsql>
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
view thread (40+ messages) latest in thread
Message-ID: <16490.1555950804@sss.pgh.pa.us>
Permalink: ../16490.1555950804@sss.pgh.pa.us/
Also on: postgresql.org/message-id/16490.1555950804@sss.pgh.pa.us
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, alvherre@2ndquadrant.com, andres@anarazel.de, michael@paquier.xyz, pryzby@telsasoft.com
Subject: Re: clean up docs for v12
In-Reply-To: <16490.1555950804@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox