Re: Document atthasmissing default optimization avoids verification table scan

From: "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>
To: James Coleman <jtc331(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andrew Dunstan <andrew(at)dunslane(dot)net>, "Bossart, Nathan" <bossartn(at)amazon(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Document atthasmissing default optimization avoids verification table scan
Date: 2022-03-26 23:14:05
Message-ID: CAKFQuwYQU55wqGmEiNY=5Wa5Zzo4ajgZV9j4gHofT93K1aYgAA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Sat, Mar 26, 2022 at 3:25 PM James Coleman <jtc331(at)gmail(dot)com> wrote:

> On Fri, Mar 25, 2022 at 4:40 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >
> > On Tue, Jan 25, 2022 at 8:49 AM James Coleman <jtc331(at)gmail(dot)com> wrote:
> > > Here's a version that looks like that. I'm not convinced it's an
> > > improvement over the previous version: again, I expect more advanced
> > > users to already understand this concept, and I think moving it to the
> > > ALTER TABLE page could very well have the effect of burying i(amidst
> > > the ton of detail on the ALTER TABLE page) concept that would be
> > > useful to learn early on in a tutorial like the DDL page. But if
> > > people really think this is an improvement, then I can acquiesce.
> >
> > I vote for rejecting both of these patches.
> >
> > 0001 adds the following sentence to the documentation: "A <literal>NOT
> > NULL</literal> constraint may be added to the new column in the same
> > statement without requiring scanning the table to verify the
> > constraint." My first reaction when I read this sentence was that it
> > was warning the user about the absence of a hazard that no one would
> > expect in the first place. We could also document that adding a NOT
> > NULL constraint will not cause your gas tank to catch fire, but nobody
> > was worried about that until we brought it up.
>
> As noted at minimum we (Braintree Payments) feared this hazard. That's
> reasonable because adding a NOT NULL constraint normally requires a
> table scan while holding an exclusive lock. It's fairly obvious why
> someone like us (any anyone who can't have downtime) would be paranoid
> about any possibility of long-running operations under exclusive locks
>
>
Reading the docs again I see:

ALTER TABLE ... ALTER COLUMN ... SET/DROP NOT NULL
"SET NOT NULL may only be applied to a column provided none of the records
in the table contain a NULL value for the column. Ordinarily this is
checked during the ALTER TABLE by scanning the entire table; however, if a
valid CHECK constraint is found which proves no NULL can exist, then the
table scan is skipped."

And the claim is that the reader would read this behavior of the ALTER
COLUMN ... SET NOT NULL command and assume that it might also apply to:

ALTER TABLE ... ADD COLUMN ... DEFAULT NOT NULL

I accept that such an assumption is plausible and worth disabusing
(regardless of my opinion, that is, to my understanding, why this patch is
being introduced).

To that end we should do so in the ALTER COLUMN ... SET NOT NULL section,
not the ADD COLUMN ... DEFAULT NOT NULL (or, specifically, its
corresponding paragraph in the notes section).

I would suggest rewriting 0001 to target ALTER COLUMN instead of in the
generic notes section (in the paragraph beginning "Adding a column with a
volatile DEFAULT") for the desired clarification.

0002, the moving of existing content from DDL to ALTER TABLE, does not have
agreement and the author of this patch isn't behind it. I'm not inclined
to introduce a patch to push forth the discussion to conclusion myself. So
for now it should just die.

David J.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2022-03-26 23:20:44 Re: Small TAP tests cleanup for Windows and unused modules
Previous Message Thomas Munro 2022-03-26 23:12:07 Re: pgsql: Add 'basebackup_to_shell' contrib module.