Обсуждение: Strange presentaion related to inheritance in \d+
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_attributeat ON (at.attnum = co.conkey[1]) WHERE co.contype = 'n' AND co.conrelid = 'c1'::pg_catalog.regclassAND 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
Вложения
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)
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
Вложения
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)
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