Re: Document atthasmissing default optimization avoids verification table scan

From: James Coleman <jtc331(at)gmail(dot)com>
To: "David G(dot) Johnston" <david(dot)g(dot)johnston(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-27 18:17:40
Message-ID: CAAaqYe9iwu5LABTzL9sLAtFF+vSboMoDnd9ikB4nqrpCLtj6Vg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Sun, Mar 27, 2022 at 1:46 PM David G. Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>
> On Sun, Mar 27, 2022 at 10:00 AM James Coleman <jtc331(at)gmail(dot)com> wrote:
>>
>> As shown above, table scans (and specifically table scans used to
>> validate constraints, which is what this patch is about) are clearly
>> documented (more than once!) in the ALTER TABLE documentation. In fact
>> it's documented specifically in reference to SET NOT NULL, which is
>> even more specifically the type of constraint this patch is about.
>>
>> So "undocumented concept" is just not accurate, and so I don't see it
>> as a valid reason to reject the patch.
>>
>
> As you point out, where these scans are performed is documented. Your request, though, is to document a location where they are not performed instead of trusting in the absence of a statement meaning that no such scan happens. In this case no such scan of the table is ever needed when adding a column and so ADD COLUMN doesn't mention table scanning. We almost always choose not to document those things which do not happen. I don't always agree with this position but it is valid and largely adhered to. On that documentation theory/policy basis alone this patch can be rejected. 0001 as proposed is especially strong in violating this principle.

Hmm, I didn't realize that was project policy, but I'm a bit
surprised given that the sentence which 0001 replaces seems like a
direct violation of that also: "In neither case is a rewrite of the
table required."

> My two thoughts from yesterday take slightly different approaches to try and mitigate the same misunderstanding while also providing useful guidance to the reader to make sure the hazard of ALTER COLUMN SET NOT NULL is something they are thinking about even when adding a new column since forgetting to incorporate the NOT NULL during the add can be a costly mistake. The tweaking the notes section seems to be the more productive of the two approaches.

Yes, I like those suggestions. I've attached an updated patch that I
think fits a good bit more naturally into the Notes section
specifically addressing scans and rewrites on NOT NULL constraints.

Thanks for the feedback,
James Coleman

Attachment Content-Type Size
v4-0001-Document-atthasmissing-default-avoids-verificatio.patch application/octet-stream 1.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Erik Rijkers 2022-03-27 18:32:45 TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
Previous Message Tom Lane 2022-03-27 17:47:47 Re: refactoring basebackup.c (zstd workers)