Re: using index or check in ALTER TABLE SET NOT NULL

Stephen Frost <sfrost@snowman.net>

From: Stephen Frost <sfrost@snowman.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>, Sergei Kornilov <sk@zsrv.org>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2017-11-29T15:58:12Z
Lists: pgsql-hackers
Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Isn't the first concern addressed by using SPI..?
> 
> I did not look at the patch yet, but TBH if it uses SPI for sub-operations
> of ALTER TABLE I think that is sufficient reason to reject it out of hand.

You mean like what ALTER TABLE ... ADD FOREIGN KEY does?

> Doing things that way would create way too much of a vulnerability surface
> for code touching a partially-updated table.  At minimum, we'd have to
> blow holes in existing protections like CheckTableNotInUse, and I think
> we'd be forever finding other stuff that failed to work quite right in
> that context.  I do not want ALTER TABLE going anywhere near the planner
> or executor; I'm not even happy that it uses the parser (for index
> definition reconstruction).

That's more along the lines of the kind of response I was expecting
given the suggestion, and perhaps a good reason to just go with the
index-based lookup, when an index is available to do so with, but I'm
not entirely sure how this is different from how we handle foreign keys.

Thanks!

Stephen

Commits

  1. Avoid using INFO elevel for what are fundamentally debug messages.

  2. Revert setting client_min_messages to 'debug1' in new tests.

  3. Allow ALTER TABLE .. SET NOT NULL to skip provably unnecessary scans.

  4. Improve predtest.c's internal docs, and enhance its functionality a bit.