Horton

Thread

Commits

  1. Disallow changing NO INHERIT status of a not-null constraint

  2. Catalog not-null constraints

  1. Strange presentaion related to inheritance in \d+

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-08-28T07:16:58Z

    While translating a message, I found a questionable behavior in \d+,
introduced by a recent commit b0e96f3119. In short, the current code
hides the constraint's origin when "NO INHERIT" is used.

For these tables:

create table p (a int, b int not null default 0);
create table c1 (a int, b int not null default 1) inherits (p);

The output from "\d+ c1" contains the lines:

> Not-null constraints:
>    "c1_b_not_null" NOT NULL "b" *(local, inherited)*

But with these tables:

create table p (a int, b int not null default 0);
create table c1 (a int, b int not null NO INHERIT default 1) inherits (p);

I get:

> Not-null constraints:
>    "c1_b_not_null" NOT NULL "b" *NO INHERIT*

Here, "NO INHERIT" is mapped from connoinherit, and conislocal and
"coninhcount <> 0" align with "local" and "inherited". For a clearer
picuture, those values for c1 are as follows.

=# SELECT co.conname, at.attname, co.connoinherit, co.conislocal, co.coninhcount FROM pg_catalog.pg_constraint co JOIN pg_catalog.pg_attribute at ON (at.attnum = co.conkey[1]) WHERE co.contype = 'n' AND co.conrelid = 'c1'::pg_catalog.regclass AND at.attrelid = 'c1'::pg_catalog.regclass ORDER BY at.attnum;
    conname    | attname | connoinherit | conislocal | coninhcount 
---------------+---------+--------------+------------+-------------
 c1_b_not_null | b       | t            | t          |           1

It feels off to me, but couldn't find any discussion about it. Is it
the intended behavior? I believe it's more appropriate to show the
origins even when specifed as NO INHERIT.

======
If not so, the following change might be possible, which is quite simple.

> Not-null constraints:
>    "c1_b_not_null" NOT NULL "b" NO INHERIT(local, inherited)

However, it looks somewhat strange as the information in parentheses
is not secondary to "NO INHERIT".  Thus, perhaps a clearer or more
proper representation would be:

>    "c1_b_not_null" NOT NULL "b" (local, inherited, not inheritable)

That being said, I don't come up with a simple way to do this for now..
(Note that we need to translate the puctuations and the words.)

There's no need to account for all combinations. "Local" and
"inherited" don't be false at the same time and the combination (local
& !inherited) is not displayed. Given these factors, we're left with 6
possible combinations, which I don't think aren't worth the hassle:

(local, inherited, not inheritable)
(inherited, not inheritable) # I didn't figure out how to cause this.
(not inheritable)
(local, inherited)
(inherited)
"" (empty string, means local)

A potential solution that comes to mind is presenting the attributes
in a space sparated list after a colon as attached. (Honestly, I'm not
fond of the format and the final term, though.)

>    "c1_b_not_null" NOT NULL "b": local inherited uninheritable

In 0001, I did wonder about hiding "local" when it's not inherited,
but this behavior rfollows existing code.

In 0002, I'm not completely satisfied with the location, but standard
regression test suite seems more suitable for this check than the TAP
test suite used for testing psql.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Re: Strange presentaion related to inheritance in \d+

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-08-28T11:36:00Z

    On 2023-Aug-28, Kyotaro Horiguchi wrote:

> But with these tables:
> 
> create table p (a int, b int not null default 0);
> create table c1 (a int, b int not null NO INHERIT default 1) inherits (p);
> 
> I get:
> 
> > Not-null constraints:
> >    "c1_b_not_null" NOT NULL "b" *NO INHERIT*
> 
> Here, "NO INHERIT" is mapped from connoinherit, and conislocal and
> "coninhcount <> 0" align with "local" and "inherited". For a clearer
> picuture, those values for c1 are as follows.

Hmm, I think the bug here is that we let you create a constraint in c1
that is NOINHERIT.  If the parent already has one INHERIT constraint
in that column, then the child must have that one also; it's not
possible to have both a constraint that inherits and one that doesn't.

I understand that there are only three possibilities for a NOT NULL
constraint in a column:

- There's a NO INHERIT constraint.  A NO INHERIT constraint is always
  defined locally in that table.  In this case, if there is a parent
  relation, then it must either not have a NOT NULL constraint in that
  column, or it may also have a NO INHERIT one.  Therefore, it's
  correct to print NO INHERIT and nothing else.  We could also print
  "(local)" but I see no point in doing that.

- A constraint comes inherited from one or more parent tables and has no
  local definition.  In this case, the constraint always inherits
  (otherwise, the parent wouldn't have given it to this table).  So
  printing "(inherited)" and nothing else is correct.

- A constraint can have a local definition and also be inherited.  In
  this case, printing "(local, inherited)" is correct.

Have I missed other cases?


The NO INHERIT bit is part of the syntax, which is why I put it in
uppercase and not marked it for translation.  The other two are
informational, so they are translatable.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them."    (Freeman Dyson)

  3. Re: Strange presentaion related to inheritance in \d+

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-08-29T04:53:34Z

    At Mon, 28 Aug 2023 13:36:00 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in 
> On 2023-Aug-28, Kyotaro Horiguchi wrote:
> 
> > But with these tables:
> > 
> > create table p (a int, b int not null default 0);
> > create table c1 (a int, b int not null NO INHERIT default 1) inherits (p);
> > 
> > I get:
> > 
> > > Not-null constraints:
> > >    "c1_b_not_null" NOT NULL "b" *NO INHERIT*
> > 
> > Here, "NO INHERIT" is mapped from connoinherit, and conislocal and
> > "coninhcount <> 0" align with "local" and "inherited". For a clearer
> > picuture, those values for c1 are as follows.
> 
> Hmm, I think the bug here is that we let you create a constraint in c1
> that is NOINHERIT.  If the parent already has one INHERIT constraint
> in that column, then the child must have that one also; it's not
> possible to have both a constraint that inherits and one that doesn't.

Yeah, I had the same question about the coexisting of the two.

> I understand that there are only three possibilities for a NOT NULL
> constraint in a column:
> 
> - There's a NO INHERIT constraint.  A NO INHERIT constraint is always
>   defined locally in that table.  In this case, if there is a parent
>   relation, then it must either not have a NOT NULL constraint in that
>   column, or it may also have a NO INHERIT one.  Therefore, it's
>   correct to print NO INHERIT and nothing else.  We could also print
>   "(local)" but I see no point in doing that.
> 
> - A constraint comes inherited from one or more parent tables and has no
>   local definition.  In this case, the constraint always inherits
>   (otherwise, the parent wouldn't have given it to this table).  So
>   printing "(inherited)" and nothing else is correct.
> 
> - A constraint can have a local definition and also be inherited.  In
>   this case, printing "(local, inherited)" is correct.
> 
> Have I missed other cases?

Seems correct. I don't see another case given that NO INHERIT is
inhibited when a table has an inherited constraint.

> The NO INHERIT bit is part of the syntax, which is why I put it in
> uppercase and not marked it for translation.  The other two are
> informational, so they are translatable.

Given the conditions above, I agree with you.

Attached is the initial version of the patch. It prevents "CREATE
TABLE" from executing if there is an inconsisntent not-null
constraint.  Also I noticed that "ALTER TABLE t ADD NOT NULL c NO
INHERIT" silently ignores the "NO INHERIT" part and fixed it.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: Strange presentaion related to inheritance in \d+

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-08-29T17:28:28Z

    On 2023-Aug-29, Kyotaro Horiguchi wrote:

> Attached is the initial version of the patch. It prevents "CREATE
> TABLE" from executing if there is an inconsisntent not-null
> constraint.  Also I noticed that "ALTER TABLE t ADD NOT NULL c NO
> INHERIT" silently ignores the "NO INHERIT" part and fixed it.

Great, thank you.  I pushed it after modifying it a bit -- instead of
throwing the error in MergeAttributes, I did it in
AddRelationNotNullConstraints().  It seems cleaner this way, mostly
because we already have to match these two constraints there.  (I guess
you could argue that we waste catalog-insertion work before the error is
reported and the whole thing is aborted; but I don't think this is a
serious problem in practice.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

  5. Re: Strange presentaion related to inheritance in \d+

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-08-30T00:46:58Z

    At Tue, 29 Aug 2023 19:28:28 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in 
> On 2023-Aug-29, Kyotaro Horiguchi wrote:
> 
> > Attached is the initial version of the patch. It prevents "CREATE
> > TABLE" from executing if there is an inconsisntent not-null
> > constraint.  Also I noticed that "ALTER TABLE t ADD NOT NULL c NO
> > INHERIT" silently ignores the "NO INHERIT" part and fixed it.
> 
> Great, thank you.  I pushed it after modifying it a bit -- instead of
> throwing the error in MergeAttributes, I did it in
> AddRelationNotNullConstraints().  It seems cleaner this way, mostly
> because we already have to match these two constraints there.  (I guess

I agree that it is cleaner.

> you could argue that we waste catalog-insertion work before the error is
> reported and the whole thing is aborted; but I don't think this is a
> serious problem in practice.)

Given the rarity and the speed required, I agree that early-catching
is not that crucial here. Thanks for clearing that up.

regardes.


-- 
Kyotaro Horiguchi
NTT Open Source Software Center