Re: cataloguing NOT NULL constraints

On Sat, May 11, 2024 at 5:40 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> I have found two more problems that I think are going to require some
> more work to fix, so I've decided to cut my losses now and revert the
> whole. I'll come back again in 18 with these problems fixed.

Bummer, but makes sense.

> Specifically, the problem is that I mentioned that we could restrict the
> NOT NULL NO INHERIT addition in pg_dump for primary keys to occur only
> in pg_upgrade; but it turns this is not correct. In normal
> dump/restore, there's an additional table scan to check for nulls when
> the constraints is not there, so the PK creation would become measurably
> slower. (In a table with a million single-int rows, PK creation goes
> from 2000ms to 2300ms due to the second scan to check for nulls).

I have a feeling that any theory of the form "X only needs to happen
during pg_upgrade" is likely to be wrong. pg_upgrade isn't really
doing anything especially unusual: just creating some objects and
loading data. Those things can also be done at other times, so
whatever is needed during pg_upgrade is also likely to be needed at
other times. Maybe that's not sound reasoning for some reason or
other, but that's my intuition.

> The addition of NOT NULL NO INHERIT constraints for this purpose
> collides with addition of constraints for other reasons, and it forces
> us to do unpleasant things such as altering an existing constraint to go
> from NO INHERIT to INHERIT. If this happens only during pg_upgrade,
> that would be okay IMV; but if we're forced to allow in normal operation
> (and in some cases we are), it could cause inconsistencies, so I don't
> want to do that. I see a way to fix this (adding another query in
> pg_dump that detects which columns descend from ones used in PKs in
> ancestor tables), but that's definitely too much additional mechanism to
> be adding this late in the cycle.

I'm sorry that I haven't been following this thread closely, but I'm
confused about how we ended up here. What exactly are the user-visible
behavior changes wrought by this patch, and how do they give rise to
these issues? One change I know about is that a constraint that is
explicitly catalogued (vs. just existing implicitly) has a name. But
it isn't obvious to me that such a difference, by itself, is enough to
cause all of these problems: if a NOT NULL constraint is created
without a name, then I suppose we just have to generate one. Maybe the
fact that the constraints have names somehow causes ugliness later,
but I can't quite understand why it would.

The other possibility that occurs to me is that I think the motivation
for cataloging NOT NULL constraints was that we wanted to be able to
track dependencies on them, or something like that, which seems like
it might be able to create issues of the type that you're facing, but
the details aren't clear to me. Changing any behavior in this area
seems like it could be quite tricky, because of things like the
interaction between PRIMARY KEY and NOT NULL, which is rather
idiosyncratic but upon which a lot of existing SQL (including SQL not
controlled by us) likely depends. If there's not a clear plan for how
we keep all the stuff that works today working, I fear we'll end up in
an endless game of whack-a-mole. If you've already written the design
ideas down someplace, I'd appreciate a pointer in the right direction.

Or maybe there's some other issue entirely. In any case, sorry about
the revert, and sorry that I haven't paid more attention to this.

--
Robert Haas
EDB: http://www.enterprisedb.com