Re: Document atthasmissing default optimization avoids verification table scan - Mailing list pgsql-hackers

On Sun, Mar 27, 2022 at 11:43 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Sat, Mar 26, 2022 at 6:25 PM James Coleman <jtc331@gmail.com> wrote:
> > I simply do not accept the claim that this is not a reasonable concern
> > to have nor that this isn't worth documenting.
>
> I don't think I said that the concern wasn't reasonable, but I don't
> think the fact that one person or organization had a concern means
> that it has to be worth documenting.

You said "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." That seemed to me even stronger than "not
a reasonable concern", and so while I agree that one organization
having a concern doesn't mean that it has to be documented, it does
seem clear to me that one such organization dispels the idea that "no
one would expect [this]", which is why I said it in response to that
statement.

> And I didn't say either that it's
> not intrinsically worth documenting. I said it doesn't fit nicely into
> the documentation we have.

That was not the critique I took away from your email at all. It is,
however, what Tom noted, and I agree it's a relevant question.

> Since you didn't like my last example, let's try another one. If
> someone shows up and proposes a documentation patch to explain what a
> BitmapOr node means, we're probably going to reject it, because it
> makes no sense to document that one node and not all the others. That
> doesn't mean that people shouldn't want to know what BitmapOr means,
> but it's just not sensible to document that one thing in isolation,
> even if somebody somewhere happened to be confused by that thing and
> not any of the other nodes.
>
> In the same way, I think you're trying to jam documentation of one
> particular point into the documentation when there are many other
> similar points that are not documented, and I think it's very awkward.
> It looks to me like you want to document that a table scan isn't
> performed in a certain case when we haven't documented the rule that
> would cause that table scan to be performed in other cases, or even
> what a table scan means in this context, or any of the similar things
> that are equally important, like a table rewrite or an index rebuild,
> or any of the rules for when those things happen.

In the ALTER TABLE docs page "table scan" is used in the section on
"SET NOT NULL", "full table scan" is used in the sections on "ADD
table_constraint_using_index" and "ATTACH PARTITION", and "table scan"
is used again in the "Note" section. Table rewrites are similarly
discussed repeatedly on that page. Indeed the docs make a clear effort
to point out where table scans and table rewrites do and do not occur
(albeit not in one unified place -- it's scattered through the various
subcommands and notes sections. Indeed the Notes section explicitly
say "The main reason for providing the option to specify multiple
changes in a single ALTER TABLE is that multiple table scans or
rewrites can thereby be combined into a single pass over the table."

So I believe it is just factually incorrect to say that "we haven't
documented...what a table scan means in this context, or any of the
similar things that are equally important, like a table rewrite or an
index rebuild, or any of the rules for when those things happen."

> It's arguable in my mind whether it is worth documenting all of those
> rules, although I am not opposed to it if somebody wants to do the
> work. But I *am* opposed to documenting that a certain situation is an
> exception to an undocumented rule about an undocumented concept.
> That's going to create confusion, not dispel it.

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.

Thanks,
James Coleman