Обсуждение: cataloguing NOT NULL constraints
I've been working on having NOT NULL constraints have pg_constraint rows. Everything is working now. Some things are a bit weird, and I would like opinions on them: 1. In my implementation, you can have more than one NOT NULL pg_constraint row for a column. What should happen if the user does ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL; ? Currently it throws an error about the ambiguity (ie. which constraint to drop). Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull' bit is lost when the last one such constraint goes away. 2. If a table has a primary key, and a table is created that inherits from it, then the child has its column(s) marked attnotnull but there is no pg_constraint row for that. This is not okay. But what should happen? 1. a CHECK(col IS NOT NULL) constraint is created for each column 2. a PRIMARY KEY () constraint is created Note that I've chosen not to create CHECK(foo IS NOT NULL) pg_constraint rows for columns in the primary key, unless an explicit NOT NULL declaration is also given. Adding them would be a very easily solution to problem 2 above, but ISTM that such constraints would be redundant and not very nice. After gathering input on these thing, I'll finish the patch and post it. As far as I can tell, everything else is working (except the annoying pg_dump tests, see below). Thanks Implementation notes: In the current implementation I am using CHECK constraints, so these constraints are contype='c', conkey={col} and the corresponding expression. pg_attribute.attnotnull is still there, and it is set true when at least one "CHECK (col IS NOT NULL)" constraint (and it's been validated) or PRIMARY KEY constraint exists for the column. CHECK constraint names are no longer "tab_col_check" when the expression is CHECK (foo IS NOT NULL). The constraint is now going to be named "tab_col_not_null" If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint printed by psql: (this is a bit more noisy that previously and it changes a lot of regression tests output). 55489 16devel 1776237=# create table tab (a int not null); CREATE TABLE 55489 16devel 1776237=# \d tab Tabla «public.tab» Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión ─────────┼─────────┼──────────────┼──────────┼───────────── a │ integer │ │ not null │ Restricciones CHECK: "tab_a_not_null" CHECK (a IS NOT NULL) pg_dump no longer prints NOT NULL in the table definition; rather, the CHECK constraint is dumped as a separate table constraint (still within the CREATE TABLE statement though). This preserves any possible constraint name, in case one was specified by the user at creation time. In order to search for the correct constraint for each column for various DDL actions, I just inspect each pg_constraint row for the table and match conkey and the CHECK expression. Some things would be easier with a new pg_attribute column that carries a pg_constraint.oid of the constraint for that column; however, that seems to be just catalog bloat and is not normalized, so I decided not to do it. Nice side-effect: if you add CHECK (foo IS NOT NULL) NOT VALID, and later validate that constraint, the attnotnull bit becomes set. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
References to past discussions and patches: https://postgr.es/m/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com https://www.postgresql.org/message-id/flat/1343682669-sup-2532@alvh.no-ip.org https://www.postgresql.org/message-id/20160109030002.GA671800@alvherre.pgsql I started this time around from the newest of my patches in those threads, but the implementation has changed considerably from what's there. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
I started this time around from the newest of my patches in those
threads, but the implementation has changed considerably from what's
there.
I don´t know exactly what will be the scope of this process you're working on, but there is a gap on foreign key constraint too.
It is possible to have wrong values on a FK constraint if you disable checking of it with session_replication_role or disable trigger all
I know you can create that constraint with "not valid" and it'll be checked when turned on. But if I just forgot that ...
So would be good to have validate constraints which checks, even if it's already valid
I know you can create that constraint with "not valid" and it'll be checked when turned on. But if I just forgot that ...
So would be good to have validate constraints which checks, even if it's already valid
drop table if exists tb_pk cascade;create table tb_pk(key integer not null primary key);
drop table if exists tb_fk cascade;create table tb_fk(fk_key integer);
alter table tb_fk add constraint fk_pk foreign key (fk_key) references tb_pk (key);
insert into tb_pk values(1);
alter table tb_fk disable trigger all; --can be with session_replication_role too.
insert into tb_fk values(5); --wrong values on that table
drop table if exists tb_fk cascade;create table tb_fk(fk_key integer);
alter table tb_fk add constraint fk_pk foreign key (fk_key) references tb_pk (key);
insert into tb_pk values(1);
alter table tb_fk disable trigger all; --can be with session_replication_role too.
insert into tb_fk values(5); --wrong values on that table
Then, you could check
alter table tb_fk validate constraint fk_pk
or
alter table tb_fk validate all constraints
or
alter table tb_fk validate all constraints
On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote: > I've been working on having NOT NULL constraints have pg_constraint > rows. > > Everything is working now. Some things are a bit weird, and I would > like opinions on them: > > 1. In my implementation, you can have more than one NOT NULL > pg_constraint row for a column. What should happen if the user does > ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL; > ? Currently it throws an error about the ambiguity (ie. which > constraint to drop). I'd say that is a good solution, particularly if there is a hint to drop the constraint instead, similar to when you try to drop an index that implements a constraint. > Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull' > bit is lost when the last one such constraint goes away. Wouldn't it be the correct solution to set "attnotnumm" to FALSE only when the last NOT NULL constraint is dropped? > 2. If a table has a primary key, and a table is created that inherits > from it, then the child has its column(s) marked attnotnull but there > is no pg_constraint row for that. This is not okay. But what should > happen? > > 1. a CHECK(col IS NOT NULL) constraint is created for each column > 2. a PRIMARY KEY () constraint is created I think it would be best to create a primary key constraint on the partition. Yours, Laurenz Albe
On 2022-Aug-18, Laurenz Albe wrote: > On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote: > > 1. In my implementation, you can have more than one NOT NULL > > pg_constraint row for a column. What should happen if the user does > > ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL; > > ? Currently it throws an error about the ambiguity (ie. which > > constraint to drop). > > I'd say that is a good solution, particularly if there is a hint to drop > the constraint instead, similar to when you try to drop an index that > implements a constraint. Ah, I didn't think about the hint. I'll add that, thanks. > > Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull' > > bit is lost when the last one such constraint goes away. > > Wouldn't it be the correct solution to set "attnotnumm" to FALSE only > when the last NOT NULL constraint is dropped? ... when the last NOT NULL or PRIMARY KEY constraint is dropped. We have to keep attnotnull set when a PK exists even if there's no specific NOT NULL constraint. > > 2. If a table has a primary key, and a table is created that inherits > > from it, then the child has its column(s) marked attnotnull but there > > is no pg_constraint row for that. This is not okay. But what should > > happen? > > > > 1. a CHECK(col IS NOT NULL) constraint is created for each column > > 2. a PRIMARY KEY () constraint is created > > I think it would be best to create a primary key constraint on the > partition. Sorry, I wasn't specific enough. This applies to legacy inheritance only; partitioning has its own solution (as you say: the PK constraint exists), but legacy inheritance works differently. Creating a PK in children tables is not feasible (because unicity cannot be maintained), but creating a CHECK (NOT NULL) constraint is possible. I think a PRIMARY KEY should not be allowed to exist in an inheritance parent, precisely because of this problem, but it seems too late to add that restriction now. This behavior is absurd, but longstanding: 55432 16devel 1787364=# create table parent (a int primary key); CREATE TABLE 55432 16devel 1787364=# create table child () inherits (parent); CREATE TABLE 55432 16devel 1787364=# insert into parent values (1); INSERT 0 1 55432 16devel 1787364=# insert into child values (1); INSERT 0 1 55432 16devel 1787364=# select * from parent; a ─── 1 1 (2 filas) -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "But static content is just dynamic content that isn't moving!" http://smylers.hates-software.com/2007/08/15/fe244d0c.html
On Thu, 2022-08-18 at 11:04 +0200, Alvaro Herrera wrote: > On 2022-Aug-18, Laurenz Albe wrote: > > On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote: > > > Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull' > > > bit is lost when the last one such constraint goes away. > > > > Wouldn't it be the correct solution to set "attnotnumm" to FALSE only > > when the last NOT NULL constraint is dropped? > > ... when the last NOT NULL or PRIMARY KEY constraint is dropped. We > have to keep attnotnull set when a PK exists even if there's no specific > NOT NULL constraint. Of course, I forgot that. I hope that is not too hard to implement. > > > 2. If a table has a primary key, and a table is created that inherits > > > from it, then the child has its column(s) marked attnotnull but there > > > is no pg_constraint row for that. This is not okay. But what should > > > happen? > > > > > > 1. a CHECK(col IS NOT NULL) constraint is created for each column > > > 2. a PRIMARY KEY () constraint is created > > > > I think it would be best to create a primary key constraint on the > > partition. > > Sorry, I wasn't specific enough. This applies to legacy inheritance > only; partitioning has its own solution (as you say: the PK constraint > exists), but legacy inheritance works differently. Creating a PK in > children tables is not feasible (because unicity cannot be maintained), > but creating a CHECK (NOT NULL) constraint is possible. > > I think a PRIMARY KEY should not be allowed to exist in an inheritance > parent, precisely because of this problem, but it seems too late to add > that restriction now. This behavior is absurd, but longstanding: My mistake; you clearly said "inherits". Since such an inheritance child currently does not have a primary key, you can insert duplicates. So automatically adding a NUT NULL constraint on the inheritance child seems the only solution that does not break backwards compatibility. pg_upgrade would have to be able to cope with that. Forcing a primary key constraint on the inheritance child could present an upgrade problem. Even if that is probably a rare and strange case, I don't think we should risk that. Moreover, if we force a primary key on the inheritance child, using ALTER TABLE ... INHERIT might have to create a unique index on the table, which can be cumbersome if the table is large. So I think a NOT NULL constraint is the least evil. Yours, Laurenz Albe
On Thu, Aug 18, 2022 at 6:04 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2022-Aug-18, Laurenz Albe wrote: > > On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote: > > > 2. If a table has a primary key, and a table is created that inherits > > > from it, then the child has its column(s) marked attnotnull but there > > > is no pg_constraint row for that. This is not okay. But what should > > > happen? > > > > > > 1. a CHECK(col IS NOT NULL) constraint is created for each column > > > 2. a PRIMARY KEY () constraint is created > > > > I think it would be best to create a primary key constraint on the > > partition. > > Sorry, I wasn't specific enough. This applies to legacy inheritance > only; partitioning has its own solution (as you say: the PK constraint > exists), but legacy inheritance works differently. Creating a PK in > children tables is not feasible (because unicity cannot be maintained), > but creating a CHECK (NOT NULL) constraint is possible. Yeah, I think it makes sense to think of the NOT NULL constraints on their own in this case, without worrying about the PK constraint that created them in the first place. BTW, maybe you are aware, but the legacy inheritance implementation is not very consistent about wanting to maintain the same NULLness for a given column in all members of the inheritance tree. For example, it allows one to alter the NULLness of an inherited column: create table p (a int not null); create table c (a int) inherits (p); \d c Table "public.c" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- a | integer | | not null | Inherits: p alter table c alter a drop not null ; ALTER TABLE \d c Table "public.c" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- a | integer | | | Inherits: p Contrast that with the partitioning implementation: create table pp (a int not null) partition by list (a); create table cc partition of pp default; \d cc Table "public.cc" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- a | integer | | not null | Partition of: pp DEFAULT alter table cc alter a drop not null ; ERROR: column "a" is marked NOT NULL in parent table IIRC, I had tried to propose implementing the same behavior for legacy inheritance back in the day, but maybe we left it alone for not breaking compatibility. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
On 2022-Aug-22, Amit Langote wrote: > Yeah, I think it makes sense to think of the NOT NULL constraints on > their own in this case, without worrying about the PK constraint that > created them in the first place. Cool, that's enough votes that I'm comfortable implementing things that way. > BTW, maybe you are aware, but the legacy inheritance implementation is > not very consistent about wanting to maintain the same NULLness for a > given column in all members of the inheritance tree. For example, it > allows one to alter the NULLness of an inherited column: Right ... I think what gives this patch most of its complexity is the number of odd, inconsistent cases that have to preserve historical behavior. Luckily I think this particular behavior is easy to implement. > IIRC, I had tried to propose implementing the same behavior for legacy > inheritance back in the day, but maybe we left it alone for not > breaking compatibility. Yeah, that wouldn't be surprising. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "The problem with the facetime model is not just that it's demoralizing, but that the people pretending to work interrupt the ones actually working." (Paul Graham)
So I was wrong in thinking that "this case was simple to implement" as I replied upthread. Doing that actually required me to rewrite large parts of the patch. I think it ended up being a good thing, because in hindsight the approach I was using was somewhat bogus anyway, and the current one should be better. Please find it attached. There are still a few problems, sadly. Most notably, I ran out of time trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode. I have to review that again, but I think it'll need a deeper rethink of how we pg_upgrade inherited constraints. So the pg_upgrade tests are known to fail. I'm not aware of any other tests failing, but I'm sure the cfbot will prove me wrong. I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull, to allow setting pg_attribute.attnotnull without adding a CHECK constraint (only used internally). I would like to find a better way to go about this, so I may remove it again, therefore it's not fully implemented. There are *many* changed regress expect files and I didn't carefully vet all of them. Mostly it's the addition of CHECK constraints in the footers of many \d listings and stuff like that. At a quick glance they appear valid, but I need to review them more carefully still. We've had pg_constraint.conparentid for a while now, but for some constraints we continue to use conislocal/coninhcount. I think we should get rid of that and rely on conparentid completely. An easily fixed issue is that of constraint naming. ChooseConstraintName has an argument for passing known constraint names, but this patch doesn't use it and it must. One issue that I don't currently know how to fix, is the fact that we need to know whether a column is a row type or not (because they need a different null test). At table creation time that's easy to know, because we have the descriptor already built by the time we add the constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT then we don't. Some ancient code comments suggest that allowing a child table's NOT NULL constraint acquired from parent shouldn't be independently droppable. This patch doesn't change that, but it's easy to do if we decide to. However, that'd be a compatibility break, so I'd rather not do it in the same patch that introduces the feature. Overall, there's a lot more work required to get this to a good shape. That said, I think it's the right direction. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La primera ley de las demostraciones en vivo es: no trate de usar el sistema. Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)
Вложения
On Wed, Aug 31, 2022 at 3:19 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
So I was wrong in thinking that "this case was simple to implement" as I
replied upthread. Doing that actually required me to rewrite large
parts of the patch. I think it ended up being a good thing, because in
hindsight the approach I was using was somewhat bogus anyway, and the
current one should be better. Please find it attached.
There are still a few problems, sadly. Most notably, I ran out of time
trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode.
I have to review that again, but I think it'll need a deeper rethink of
how we pg_upgrade inherited constraints. So the pg_upgrade tests are
known to fail. I'm not aware of any other tests failing, but I'm sure
the cfbot will prove me wrong.
I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull,
to allow setting pg_attribute.attnotnull without adding a CHECK
constraint (only used internally). I would like to find a better way to
go about this, so I may remove it again, therefore it's not fully
implemented.
There are *many* changed regress expect files and I didn't carefully vet
all of them. Mostly it's the addition of CHECK constraints in the
footers of many \d listings and stuff like that. At a quick glance they
appear valid, but I need to review them more carefully still.
We've had pg_constraint.conparentid for a while now, but for some
constraints we continue to use conislocal/coninhcount. I think we
should get rid of that and rely on conparentid completely.
An easily fixed issue is that of constraint naming.
ChooseConstraintName has an argument for passing known constraint names,
but this patch doesn't use it and it must.
One issue that I don't currently know how to fix, is the fact that we
need to know whether a column is a row type or not (because they need a
different null test). At table creation time that's easy to know,
because we have the descriptor already built by the time we add the
constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT
then we don't.
Some ancient code comments suggest that allowing a child table's NOT
NULL constraint acquired from parent shouldn't be independently
droppable. This patch doesn't change that, but it's easy to do if we
decide to. However, that'd be a compatibility break, so I'd rather not
do it in the same patch that introduces the feature.
Overall, there's a lot more work required to get this to a good shape.
That said, I think it's the right direction.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)
Hi,
For findNotNullConstraintAttnum():
+ if (multiple == NULL)
+ break;
+ break;
Shouldn't `pfree(arr)` be called before breaking ?
+static Constraint *makeNNCheckConstraint(Oid nspid, char *constraint_name,
You used `NN` because there is method makeCheckNotNullConstraint, right ?
I think it would be better to expand `NN` so that its meaning is easy to understand.
Cheers
On Wed, Aug 31, 2022 at 4:08 PM Zhihong Yu <zyu@yugabyte.com> wrote:
On Wed, Aug 31, 2022 at 3:19 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:So I was wrong in thinking that "this case was simple to implement" as I
replied upthread. Doing that actually required me to rewrite large
parts of the patch. I think it ended up being a good thing, because in
hindsight the approach I was using was somewhat bogus anyway, and the
current one should be better. Please find it attached.
There are still a few problems, sadly. Most notably, I ran out of time
trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode.
I have to review that again, but I think it'll need a deeper rethink of
how we pg_upgrade inherited constraints. So the pg_upgrade tests are
known to fail. I'm not aware of any other tests failing, but I'm sure
the cfbot will prove me wrong.
I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull,
to allow setting pg_attribute.attnotnull without adding a CHECK
constraint (only used internally). I would like to find a better way to
go about this, so I may remove it again, therefore it's not fully
implemented.
There are *many* changed regress expect files and I didn't carefully vet
all of them. Mostly it's the addition of CHECK constraints in the
footers of many \d listings and stuff like that. At a quick glance they
appear valid, but I need to review them more carefully still.
We've had pg_constraint.conparentid for a while now, but for some
constraints we continue to use conislocal/coninhcount. I think we
should get rid of that and rely on conparentid completely.
An easily fixed issue is that of constraint naming.
ChooseConstraintName has an argument for passing known constraint names,
but this patch doesn't use it and it must.
One issue that I don't currently know how to fix, is the fact that we
need to know whether a column is a row type or not (because they need a
different null test). At table creation time that's easy to know,
because we have the descriptor already built by the time we add the
constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT
then we don't.
Some ancient code comments suggest that allowing a child table's NOT
NULL constraint acquired from parent shouldn't be independently
droppable. This patch doesn't change that, but it's easy to do if we
decide to. However, that'd be a compatibility break, so I'd rather not
do it in the same patch that introduces the feature.
Overall, there's a lot more work required to get this to a good shape.
That said, I think it's the right direction.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)Hi,For findNotNullConstraintAttnum():+ if (multiple == NULL)
+ break;Shouldn't `pfree(arr)` be called before breaking ?+static Constraint *makeNNCheckConstraint(Oid nspid, char *constraint_name,You used `NN` because there is method makeCheckNotNullConstraint, right ?I think it would be better to expand `NN` so that its meaning is easy to understand.Cheers
Hi,
For tryExtractNotNullFromNode , in the block for `if (rel == NULL)`:
+ return false;
I think you meant returning NULL since false is for boolean.
Cheers
There were a lot more problems in that submission than I at first realized, and I had to rewrite a lot of code in order to fix them. I have fixed all the user-visible problems I found in this version, and reviewed the tests results more carefully so I am now more confident that behaviourally it's doing the right thing; but 1. the pg_upgrade test problem is still unaddressed, 2. I haven't verified that catalog contents is correct, especially regarding dependencies, 3. there are way too many XXX and FIXME comments sprinkled everywhere. I'm sure a couple of these XXX comments can be left for later work, and there's a few that should be dealt with by merely removing them; but the others (and all FIXMEs) represent pending work. Also, I'm not at all happy about having this new ConstraintNotNull artificial node there; perhaps this can be solved by using a regular Constraint with some new flag, or maybe it will even work without any extra flags by the fact that the node appears where it appears. Anyway, requires investigation. Also, the AT_SetAttNotNull continues to irk me. test_ddl_deparse is also unhappy. This is probably an easy fix; apparently, ATExecDropConstraint has been doing things wrong forever. Anyway, here's version 2 of this, with apologies for those who spent time reviewing version 1 with all its brokenness. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "On the other flipper, one wrong move and we're Fatal Exceptions" (T.U.X.: Term Unit X - http://www.thelinuxreview.com/TUX/)
Вложения
Hi,
w.r.t. the while loop in findNotNullConstraintAttnum():
+ if (multiple == NULL)
+ break;
+ break;
I think `pfree(arr)` should be called before breaking.
+ if (constraint->cooked_expr != NULL)
+ return tryExtractNotNullFromNode(stringToNode(constraint->cooked_expr), rel);
+ else
+ return tryExtractNotNullFromNode(constraint->raw_expr, rel);
+ return tryExtractNotNullFromNode(stringToNode(constraint->cooked_expr), rel);
+ else
+ return tryExtractNotNullFromNode(constraint->raw_expr, rel);
nit: the `else` keyword is not needed.
+ if (isnull)
+ elog(ERROR, "null conbin for constraint %u", conForm->oid);
+ elog(ERROR, "null conbin for constraint %u", conForm->oid);
It would be better to expand `conbin` so that the user can better understand the error.
Cheers
Hi Alvaro, On Sat, Sep 10, 2022 at 2:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > There were a lot more problems in that submission than I at first > realized, and I had to rewrite a lot of code in order to fix them. I > have fixed all the user-visible problems I found in this version, and > reviewed the tests results more carefully so I am now more confident > that behaviourally it's doing the right thing; but > > 1. the pg_upgrade test problem is still unaddressed, > 2. I haven't verified that catalog contents is correct, especially > regarding dependencies, > 3. there are way too many XXX and FIXME comments sprinkled everywhere. > > I'm sure a couple of these XXX comments can be left for later work, and > there's a few that should be dealt with by merely removing them; but the > others (and all FIXMEs) represent pending work. > > Also, I'm not at all happy about having this new ConstraintNotNull > artificial node there; perhaps this can be solved by using a regular > Constraint with some new flag, or maybe it will even work without any > extra flags by the fact that the node appears where it appears. Anyway, > requires investigation. Also, the AT_SetAttNotNull continues to irk me. > > test_ddl_deparse is also unhappy. This is probably an easy fix; > apparently, ATExecDropConstraint has been doing things wrong forever. > > Anyway, here's version 2 of this, with apologies for those who spent > time reviewing version 1 with all its brokenness. I have been testing this with the intention of understanding how you made this work with inheritance. While doing so with the previous version, I ran into an existing issue (bug?) that I reported at [1]. I ran into another while testing version 2 that I think has to do with this patch. So this happens: -- regular inheritance create table foo (a int not null); create table foo1 (a int not null); alter table foo1 inherit foo; alter table foo alter a drop not null ; ERROR: constraint "foo_a_not_null" of relation "foo1" does not exist -- partitioning create table parted (a int not null) partition by list (a); create table part1 (a int not null); alter table parted attach partition part1 default; alter table parted alter a drop not null; ERROR: constraint "parted_a_not_null" of relation "part1" does not exist In both of these cases, MergeConstraintsIntoExisting(), called by CreateInheritance() when attaching the child to the parent, marks the child's NOT NULL check constraint as the child constraint of the corresponding constraint in parent, which seems fine and necessary. However, ATExecDropConstraint_internal(), the new function called by ATExecDropNotNull(), doesn't seem to recognize when recursing to the child tables that a child's copy NOT NULL check constraint attached to the parent's may have a different name, so scanning pg_constraint with the parent's name is what gives the above error. I wonder if it wouldn't be better for ATExecDropNotNull() to handle its own recursion rather than delegating it to the DropConstraint()? The same error does not occur when the NOT NULL constraint is added to parent after-the-fact and thus recursively to the children: -- regular inheritance create table foo (a int); create table foo1 (a int not null) inherits (foo); alter table foo alter a set not null; alter table foo alter a drop not null ; ALTER TABLE -- partitioning create table parted (a int) partition by list (a); create table part1 partition of parted (a not null) default; alter table parted alter a set not null; alter table parted alter a drop not null; ALTER TABLE And the reason for that seems a bit accidental, because MergeWithExistingConstraint(), called by AddRelationNewConstraints() when recursively adding the NOT NULL check constraint to a child, does not have the code to find the child's already existing constraint that matches with it. So, in this case, we get a copy of the parent's constraint with the same name in the child. There is a line in the header comments of both MergeWithExistingConstraint() and MergeConstraintsIntoExisting() asking to keep their code in sync, so maybe the patch missed adding the new NOT NULL check constraint logic to the former? Also, it seems that the inheritance recursion for SET NOT NULL is now occurring both in the prep phase and exec phase due to the following new code added to ATExecSetNotNull(): @@ -7485,6 +7653,50 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), attnum); ... + /* See if there's one already, and skip this if so. */ + constr = findNotNullConstraintAttnum(rel, attnum, NULL); + if (constr && direct) + heap_freetuple(constr); /* nothing to do */ + else + { + Constraint *newconstr; + ObjectAddress addr; + List *children; + List *already_done_rels; + + newconstr = makeCheckNotNullConstraint(rel->rd_rel->relnamespace, + constrname, + NameStr(rel->rd_rel->relname), + colName, + false, /* XXX is_row */ + InvalidOid); + + addr = ATAddCheckConstraint_internal(wqueue, tab, rel, newconstr, + false, false, false, lockmode); + already_done_rels = list_make1_oid(RelationGetRelid(rel)); + + /* and recurse into children, if there are any */ + children = find_inheritance_children(RelationGetRelid(rel), lockmode); + ATAddCheckConstraint_recurse(wqueue, children, newconstr, It seems harmless because ATExecSetNotNull() set up to run on the children by the prep phase becomes a no-op due to the work done by the above code, but maybe we should keep one or the other. Regarding the following bit: - /* If rel is partition, shouldn't drop NOT NULL if parent has the same */ + /* + * If rel is partition, shouldn't drop NOT NULL if parent has the same. + * XXX is this consideration still valid? Can we get rid of this by + * changing the type of dependency between the two constraints instead? + */ if (rel->rd_rel->relispartition) { Oid parentId = get_partition_parent(RelationGetRelid(rel), false); Yes, it seems we can now prevent dropping a partition's NOT NULL constraint by seeing that it is inherited, so no need for this block which was written when the NOT NULL constraints didn't have the inherited marking. BTW, have you thought about making DROP NOT NULL command emit a different error message than what one gets now: create table foo (a int); create table foo1 (a int) inherits (foo); alter table foo alter a set not null; alter table foo1 alter a drop not null ; ERROR: cannot drop inherited constraint "foo_a_not_null" of relation "foo1" Like, say: ERROR: cannot drop an inherited NOT NULL constraint Maybe you did and thought that it's OK for it to spell out the internally generated constraint name, because we already require users to know that they exist, say to drop it using DROP CONSTRAINT. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://www.postgresql.org/message-id/CA%2BHiwqFggpjAvsVqNV06HUF6CUrU0Vo3pLgGWCViGbPkzTiofg%40mail.gmail.com
On 09.09.22 19:58, Alvaro Herrera wrote: > There were a lot more problems in that submission than I at first > realized, and I had to rewrite a lot of code in order to fix them. I > have fixed all the user-visible problems I found in this version, and > reviewed the tests results more carefully so I am now more confident > that behaviourally it's doing the right thing; but Reading through the SQL standard again, I think this patch goes a bit too far in folding NOT NULL and CHECK constraints together. The spec says that you need to remember whether a column was defined as NOT NULL, and that the commands DROP NOT NULL and SET NOT NULL only affect constraints defined in that way. In this implementation, a constraint defined as NOT NULL is converted to a CHECK (x IS NOT NULL) constraint and the original definition is forgotten. Besides that, I think that users are not going to like that pg_dump rewrites their NOT NULL constraints into CHECK table constraints. I suspect that this needs a separate contype for NOT NULL constraints that is separate from CONSTRAINT_CHECK.
On 2022-Sep-14, Peter Eisentraut wrote: > Reading through the SQL standard again, I think this patch goes a bit too > far in folding NOT NULL and CHECK constraints together. The spec says that > you need to remember whether a column was defined as NOT NULL, and that the > commands DROP NOT NULL and SET NOT NULL only affect constraints defined in > that way. In this implementation, a constraint defined as NOT NULL is > converted to a CHECK (x IS NOT NULL) constraint and the original definition > is forgotten. Hmm, I don't read it the same way. Reading SQL:2016, they talk about a nullability characteristic (/known not nullable/ or /possibly nullable/): : 4.13 Columns, fields, and attributes : [...] : Every column has a nullability characteristic that indicates whether the : value from that column can be the null value. A nullability characteristic : is either known not nullable or possibly nullable. : Let C be a column of a base table T. C is known not nullable if and only : if at least one of the following is true: : — There exists at least one constraint NNC that is enforced and not : deferrable and that simply contains a <search condition> that is a : <boolean value expression> that is a readily-known-not-null condition for C. : [other possibilities] then in the same section they explain that this is derived from a table constraint: : A column C is described by a column descriptor. A column descriptor : includes: : [...] : — If C is a column of a base table, then an indication of whether it is : defined as NOT NULL and, if so, the constraint name of the associated : table constraint definition. [aside: note that elsewhere (<boolean value expression>), they define "readily-known-not-null" in Syntax Rule 3), of 6.39 <boolean value expression>: : 3) Let X denote either a column C or the <key word> VALUE. Given a : <boolean value expression> BVE and X, the notion “BVE is a : readily-known-not-null condition for X” is defined as follows. : Case: : a) If BVE is a <predicate> of the form “RVE IS NOT NULL”, where RVE is a : <row value predicand> that is a <row value constructor predicand> that : simply contains a <common value expression>, <boolean predicand>, or : <row value constructor element> that is a <column reference> that : references C, then BVE is a readily-known-not-null condition for C. : b) If BVE is the <predicate> “VALUE IS NOT NULL”, then BVE is a : readily-known-not-null condition for VALUE. : c) Otherwise, BVE is not a readily-known-not-null condition for X. edisa] Later, <column definition> says literally that specifying NOT NULL in a column is equivalent to the CHECK (.. IS NOT NULL) table constraint: : 11.4 <column definition> : : Syntax Rules, : 17) If a <column constraint definition> is specified, then let CND be : the <constraint name definition> if one is specified and let CND be the : zero-length character character string otherwise; let CA be the : <constraint characteristics> if specified and let CA be the zero-length : character string otherwise. The <column constraint definition> is : equivalent to a <table constraint definition> as follows. : : Case: : : a) If a <column constraint definition> is specified that contains the : <column constraint> NOT NULL, then it is equivalent to the following : <table constraint definition>: : CND CHECK ( C IS NOT NULL ) CA In my reading of it, it doesn't follow that you have to remember whether the table constraint was created by saying explicitly by doing CHECK (c IS NOT NULL) or as a plain NOT NULL column constraint. The idea of being able to do DROP NOT NULL when only a constraint defined as CHECK (c IS NOT NULL) exists seems to follow from there; and also that you can use DROP CONSTRAINT to remove one added via plain NOT NULL; and that both these operations change the nullability characteristic of the column. This is made more explicit by the fact that they do state that the nullability characteristic can *not* be "destroyed" for other types of constraints, in 11.26 <drop table constraint definition>, Syntax Rule 11) : 11) Destruction of TC shall not cause the nullability characteristic of : any of the following columns of T to change from known not nullable to : possibly nullable: : : a) A column that is a constituent of the primary key of T, if any. : b) The system-time period start column, if any. : c) The system-time period end column, if any. : d) The identity column, if any. then General Rule 7) explains that this does indeed happen for columns declared to have some sort of NOT NULL constraint, without saying exactly how was that constraint defined: : 7) If TC causes some column COL to be known not nullable and no other : constraint causes COL to be known not nullable, then the nullability : characteristic of the column descriptor of COL is changed to possibly : nullable. > Besides that, I think that users are not going to like that pg_dump rewrites > their NOT NULL constraints into CHECK table constraints. This is a good point, but we could get around it by decreeing that pg_dump dumps the NOT NULL in the old way if the name is not changed from whatever would be generated normally. This would require some games to remove the CHECK one; and it would also mean that partitions would not use the same constraint as the parent, but rather it'd have to generate a new constraint name that uses its own table name, rather than the parent's. (This makes me wonder what should happen if you rename a table: should we go around and rename all the automatically-named constraints as well? Probably not, but this may annoy people that creates table under one name, then rename them into their final places afterwards. pg_dump may behave funny for those. We can tackle that later, if ever. But consider that moving the table across schemas might cause even weirder problems, since the standard says constraint names must not conflict within a schema ...) > I suspect that this needs a separate contype for NOT NULL constraints that > is separate from CONSTRAINT_CHECK. Maybe it is possible to read this in the way you propose, but I think that interpretation is strictly less useful than the one I propose. Also, see this reply from Tom to Vitaly Burovoy who was proposing something that seems to derivate from this interpretation: https://www.postgresql.org/message-id/flat/17684.1462339177%40sss.pgh.pa.us -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Hay dos momentos en la vida de un hombre en los que no debería especular: cuando puede permitírselo y cuando no puede" (Mark Twain)
On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint > printed by psql: (this is a bit more noisy that previously and it > changes a lot of regression tests output). > > 55489 16devel 1776237=# create table tab (a int not null); > CREATE TABLE > 55489 16devel 1776237=# \d tab > Tabla «public.tab» > Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión > ─────────┼─────────┼──────────────┼──────────┼───────────── > a │ integer │ │ not null │ > Restricciones CHECK: > "tab_a_not_null" CHECK (a IS NOT NULL) In a table with many columns, most of which are NOT NULL, this is going to produce a ton of clutter. I don't like that. I'm not sure what a good alternative would be, though. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, 19 Sept 2022 at 09:32, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint
> printed by psql: (this is a bit more noisy that previously and it
> changes a lot of regression tests output).
>
> 55489 16devel 1776237=# create table tab (a int not null);
> CREATE TABLE
> 55489 16devel 1776237=# \d tab
> Tabla «public.tab»
> Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión
> ─────────┼─────────┼──────────────┼──────────┼─────────────
> a │ integer │ │ not null │
> Restricciones CHECK:
> "tab_a_not_null" CHECK (a IS NOT NULL)
In a table with many columns, most of which are NOT NULL, this is
going to produce a ton of clutter. I don't like that.
I'm not sure what a good alternative would be, though.
I thought I saw some discussion about the SQL standard saying that there is a difference between putting NOT NULL in a column definition, and CHECK (column_name NOT NULL). So if we're going to take this seriously, I think that means there needs to be a field in pg_constraint which identifies whether a constraint is a "real" one created explicitly as a constraint, or if it is just one created because a field is marked NOT NULL.
If this is correct, the answer is easy: don't show constraints that are there only because of a NOT NULL in the \d or \d+ listings. I certainly don't want to see that clutter and I'm having trouble seeing why anybody else would want to see it either; the information is already there in the "Nullable" column of the field listing.
The error message for a duplicate constraint name when creating a constraint needs however to be very clear that the conflict is with a NOT NULL constraint and which one, since I'm proposing leaving those ones off the visible listing, and it would be very bad for somebody to get "duplicate name" and then be unable to see the conflicting entry.
Isaac Morland <isaac.morland@gmail.com> writes: > I thought I saw some discussion about the SQL standard saying that there is > a difference between putting NOT NULL in a column definition, and CHECK > (column_name NOT NULL). So if we're going to take this seriously, I think > that means there needs to be a field in pg_constraint which identifies > whether a constraint is a "real" one created explicitly as a constraint, or > if it is just one created because a field is marked NOT NULL. If we're going to go that way, I think that we should take the further step of making not-null constraints be their own contype rather than an artificially generated CHECK. The bloat in pg_constraint from CHECK expressions made this way seems like an additional reason not to like doing it like that. regards, tom lane
On Mon, 19 Sept 2022 at 15:32, Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint > > printed by psql: (this is a bit more noisy that previously and it > > changes a lot of regression tests output). > > > > 55489 16devel 1776237=# create table tab (a int not null); > > CREATE TABLE > > 55489 16devel 1776237=# \d tab > > Tabla «public.tab» > > Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión > > ─────────┼─────────┼──────────────┼──────────┼───────────── > > a │ integer │ │ not null │ > > Restricciones CHECK: > > "tab_a_not_null" CHECK (a IS NOT NULL) > > In a table with many columns, most of which are NOT NULL, this is > going to produce a ton of clutter. I don't like that. > > I'm not sure what a good alternative would be, though. I'm not sure on the 'good' part of this alternative, but we could go with a single row-based IS NOT NULL to reduce such clutter, utilizing the `ROW() IS NOT NULL` requirement of a row only matching IS NOT NULL when all attributes are also IS NOT NULL: Check constraints: "tab_notnull_check" CHECK (ROW(a, b, c, d, e) IS NOT NULL) instead of: Check constraints: "tab_a_not_null" CHECK (a IS NOT NULL) "tab_b_not_null" CHECK (b IS NOT NULL) "tab_c_not_null" CHECK (c IS NOT NULL) "tab_d_not_null" CHECK (d IS NOT NULL) "tab_e_not_null" CHECK (e IS NOT NULL) But the performance of repeated row-casting would probably not be as good as our current NULL checks if we'd use the current row infrastructure, and constraint failure reports wouldn't be as helpful as the current attribute NOT NULL failures. Kind regards, Matthias van de Meent
On 2022-Sep-19, Robert Haas wrote: > On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > 55489 16devel 1776237=# \d tab > > Tabla «public.tab» > > Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión > > ─────────┼─────────┼──────────────┼──────────┼───────────── > > a │ integer │ │ not null │ > > Restricciones CHECK: > > "tab_a_not_null" CHECK (a IS NOT NULL) > > In a table with many columns, most of which are NOT NULL, this is > going to produce a ton of clutter. I don't like that. > > I'm not sure what a good alternative would be, though. Perhaps that can be solved by displaying the constraint name in the table: 55489 16devel 1776237=# \d tab Tabla «public.tab» Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión ─────────┼─────────┼──────────────┼────────────────┼───────────── a │ integer │ │ tab_a_not_null │ (Perhaps we can change the column title also, not sure.) -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "The Gord often wonders why people threaten never to come back after they've been told never to return" (www.actsofgord.com)
On 2022-Sep-19, Isaac Morland wrote: > I thought I saw some discussion about the SQL standard saying that there is > a difference between putting NOT NULL in a column definition, and CHECK > (column_name NOT NULL). So if we're going to take this seriously, Was it Peter E.'s reply to this thread? https://postgr.es/m/bac841ed-b86d-e3c2-030d-02a3db067307@enterprisedb.com because if it wasn't there, then I would like to know what you source is. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Thou shalt not follow the NULL pointer, for chaos and madness await thee at its end." (2nd Commandment for C programmers)
On 2022-Sep-19, Matthias van de Meent wrote: > I'm not sure on the 'good' part of this alternative, but we could go > with a single row-based IS NOT NULL to reduce such clutter, utilizing > the `ROW() IS NOT NULL` requirement of a row only matching IS NOT NULL > when all attributes are also IS NOT NULL: > > Check constraints: > "tab_notnull_check" CHECK (ROW(a, b, c, d, e) IS NOT NULL) There's no way to mark this NOT VALID individually or validate it afterwards, though. > But the performance of repeated row-casting would probably not be as > good as our current NULL checks The NULL checks would still be mostly done by the attnotnull checks internally, so there shouldn't be too much of a difference. .. though I'm now wondering if there's additional overhead from checking the constraint twice on each row: first the attnotnull bit, then the CHECK itself. Hmm. That's probably quite bad. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Tue, 20 Sept 2022 at 06:56, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
The NULL checks would still be mostly done by the attnotnull checks
internally, so there shouldn't be too much of a difference.
.. though I'm now wondering if there's additional overhead from checking
the constraint twice on each row: first the attnotnull bit, then the
CHECK itself. Hmm. That's probably quite bad.
Another reason to treat NOT NULL-implementing constraints differently.
My thinking is that pg_constraint entries for NOT NULL columns are mostly an implementation detail. I've certainly never cared whether I had an actual constraint corresponding to my NOT NULL columns. So I think marking them as such, or a different contype, and excluding them from \d+ display, probably makes sense. Just need to deal with the issue of trying to create a constraint and having its name conflict with a NOT NULL constraint. Could it work to reserve [field name]_notnull for NOT NULL-implementing constraints? I'd be worried about what happens with field renames; renaming the constraint automatically seems a bit weird, but maybe…
On 2022-Sep-20, Isaac Morland wrote: > On Tue, 20 Sept 2022 at 06:56, Alvaro Herrera <alvherre@alvh.no-ip.org> > wrote: > > > .. though I'm now wondering if there's additional overhead from checking > > the constraint twice on each row: first the attnotnull bit, then the > > CHECK itself. Hmm. That's probably quite bad. > > Another reason to treat NOT NULL-implementing constraints differently. Yeah. > My thinking is that pg_constraint entries for NOT NULL columns are mostly > an implementation detail. I've certainly never cared whether I had an > actual constraint corresponding to my NOT NULL columns. Naturally, all catalog entries are implementation details; a user never really cares if an entry exists or not, only that the desired semantics are provided. In this case, we want the constraint row because it gives us some additional features, such as the ability to mark NOT NULL constraints NOT VALID and validating them later, which is a useful thing to do in large production databases. We have some hacks to provide part of that functionality using straight CHECK constraints, but you cannot cleanly get the `attnotnull` flag set for a column (which means it's hard to add a primary key, for example). It is also supposed to fix some inconsistencies such as disallowing to remove a constraint on a table when it is implied from a constraint on an ancestor table. Right now we have ad-hoc protections for partitions, but we don't do that for legacy inheritance. That said, the patch I posted for this ~10 years ago used a separate contype and was simpler than what I ended up with now, but amusingly enough it was returned at the time with the argument that it would be better to treat them as normal CHECK constraints; so I want to be very sure that we're not just going around in circles. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Tue, Sep 20, 2022 at 10:39 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > That said, the patch I posted for this ~10 years ago used a separate > contype and was simpler than what I ended up with now, but amusingly > enough it was returned at the time with the argument that it would be > better to treat them as normal CHECK constraints; so I want to be very > sure that we're not just going around in circles. I don't have an intrinsic view on whether we ought to have one contype or two, but I think this comment of yours from a few messages ago is right on point: ".. though I'm now wondering if there's additional overhead from checking the constraint twice on each row: first the attnotnull bit, then the CHECK itself. Hmm. That's probably quite bad." For that exact reason, it seems absolutely necessary to be able to somehow identify the "redundant" check constraints, so that we don't pay the expense of revalidating them. Another contype would be one way of identifying such constraints, but it could probably be done in other ways, too. Perhaps it could even be discovered dynamically, like when we build a relcache entry. I actually have no idea what design is best. I am a little confused as to why we want to do this, though. While we're on the topic of what is more complicated and simpler, what functionality do we get out of adding all of these additional catalog entries that then have to be linked back to the corresponding attnotnull markings? And couldn't we get that functionality in some much simpler way? Like, if you want to track whether the NOT NULL constraint has been validated, we could add an attnotnullvalidated column, or probably better, change the existing attnotnull column to a character used as an enum, or maybe an integer bit-field of some kind. I'm not trying to blow up your patch with dynamite or anything, but I'm a little suspicious that this may be one of those cases where pgsql-hackers discussed turns a complicated project into an even more complicated project to no real benefit. One thing that I don't particularly like about this whole design is that it feels like it creates a bunch of catalog bloat. Now all of the attnotnull flags also generate additional pg_constraint rows. The catalogs in the default install will be bigger than before, and the catalogs after user tables are created will be more bigger. If we get some nifty benefit out of all that, cool! But if we're just anti-optimizing the catalog storage out of some feeling that the result will be intellectually purer than some competing design, maybe we should reconsider. It's not stupid to optimize for common special cases, and making a column as NOT NULL is probably at least one and maybe several orders of magnitude more common than putting some other CHECK constraint on it. -- Robert Haas EDB: http://www.enterprisedb.com
So I reworked this to use a new contype value for the NOT NULL pg_constraint rows; I attach it here. I think it's fairly clean. 0001 is just a trivial change that seemed obvious as soon as I ran into the problem. 0002 is the most interesting part. Things that are curious: - Inheritance and primary keys. If you have a table with a primary key, and create a child of it, that child is going to have a NOT NULL in the column that is the primary key. - Inheritance and plain constraints. It is not allowed to remove the NOT NULL constraint from a child; currently, NO INHERIT constraints are not supported. I would say this is an useless feature, but perhaps not. 0003: Since nobody liked the idea of listing the constraints in psql \d's footer, I changed \d+ so that the "not null" column shows the name of the constraint if there is one, or the string "(primary key)" if the attnotnull marking for the column comes from the primary key. The new column is going to be quite wide in some cases; if we want to hide it further, we could add the mythical \d++ and have *that* list the constraint name, keeping \d+ as current. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Los trabajadores menos efectivos son sistematicamente llevados al lugar donde pueden hacer el menor daño posible: gerencia." (El principio Dilbert)
Вложения
Hmm, so it turned out that cfbot didn't like this because I didn't patch one of the compression.out alternate files. Fixed here. I think in the future I'm not going to submit the 0003 patch, because it's not very interesting while being way too bulky and also the one most likely to have conflicts. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Hay que recordar que la existencia en el cosmos, y particularmente la elaboración de civilizaciones dentro de él no son, por desgracia, nada idílicas" (Ijon Tichy)
Вложения
On Tue, Feb 28, 2023 at 08:15:37PM +0100, Alvaro Herrera wrote: > Since nobody liked the idea of listing the constraints in psql \d's > footer, I changed \d+ so that the "not null" column shows the name of > the constraint if there is one, or the string "(primary key)" if the > attnotnull marking for the column comes from the primary key. The new > column is going to be quite wide in some cases; if we want to hide it > further, we could add the mythical \d++ and have *that* list the > constraint name, keeping \d+ as current. One concern here is that the title "NOT NULL Constraint" is itself pretty wide, which is an issue for tables which have no not-null constraints. On Wed, Mar 01, 2023 at 01:03:48PM +0100, Alvaro Herrera wrote: > Hmm, so it turned out that cfbot didn't like this because I didn't patch > one of the compression.out alternate files. Fixed here. I think in the > future I'm not going to submit the 0003 patch, because it's not very > interesting while being way too bulky and also the one most likely to > have conflicts. I like \dt++, and it seems like the obvious thing to do here, to avoid changing lots of regression test output, which seems worth avoiding in any case, due to ensuing conflicts in other patches being developed, and in backpatching. Right now, \dt+ includes a bit too much output, including things like sizes, which makes it hard to test. Moving some things into \dt++ would make \dt+ more testable (and more usable BTW). Even if that's not true of (or not a good idea) for \dt+, I'm sure it applies to other slash commands. Currently, fourty-five (45) psql commands support verbose "plus" variants, and the sql regression tests exercise fifteen (15) of them. I proposed \dn++, \dA++, and \db++ in 2ndary patches here: https://commitfest.postgresql.org/42/3256/ I've considered sending a patch with "plusplus" commands as 001, to propose that on its own merits rather than in the context of \d[Abn]++ -- Justin
On 28.02.23 20:15, Alvaro Herrera wrote: > So I reworked this to use a new contype value for the NOT NULL > pg_constraint rows; I attach it here. I think it's fairly clean. > > 0001 is just a trivial change that seemed obvious as soon as I ran into > the problem. This looks harmless enough, but I wonder what the reason for it is. What command can cause this error (no test case?)? Is there ever a confusion about what table is in play? > 0002 is the most interesting part. Where did this syntax come from: --- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -77,6 +77,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI [ CONSTRAINT <replaceable class="parameter">constraint_name</replaceable> ] { CHECK ( <replaceable class="parameter">expression</replaceable> ) [ NO INHERIT ] | + NOT NULL <replaceable class="parameter">column_name</replaceable> | UNIQUE [ NULLS [ NOT ] DISTINCT ] ( <replaceable class="parameter">column_name</replaceable> [, ... ] ) <replaceable class="parameter">in> PRIMARY KEY ( <replaceable class="parameter">column_name</replaceable> [, ... ] ) <replaceable class="parameter">index_parameters</replac> EXCLUDE [ USING <replaceable class="parameter">index_method</replaceable> ] ( <replaceable class="parameter">exclude_element</replaceable> I don't see that in the standard. If we need it for something, we should at least document that it's an extension. The test tables in foreign_key.sql are declared with columns like id bigint NOT NULL PRIMARY KEY, which is a bit weird and causes expected output diffs in your patch. Is that interesting for this patch? Otherwise I suggest dropping the NOT NULL from those table definitions to avoid these extra diffs. > 0003: > Since nobody liked the idea of listing the constraints in psql \d's > footer, I changed \d+ so that the "not null" column shows the name of > the constraint if there is one, or the string "(primary key)" if the > attnotnull marking for the column comes from the primary key. The new > column is going to be quite wide in some cases; if we want to hide it > further, we could add the mythical \d++ and have *that* list the > constraint name, keeping \d+ as current. I think my rough preference here would be to leave the existing output style (column header "Nullable", content "not null") alone and display the constraint name somewhere in the footer optionally. In practice, the name of the constraint is rarely needed. I do like the idea of mentioning primary key-ness inside the table somehow. As you wrote elsewhere, we can leave this patch alone for now.
On 2023-Mar-03, Peter Eisentraut wrote: > On 28.02.23 20:15, Alvaro Herrera wrote: > > So I reworked this to use a new contype value for the NOT NULL > > pg_constraint rows; I attach it here. I think it's fairly clean. > > > > 0001 is just a trivial change that seemed obvious as soon as I ran into > > the problem. > > This looks harmless enough, but I wonder what the reason for it is. What > command can cause this error (no test case?)? Is there ever a confusion > about what table is in play? Hmm, I realize now that the only reason I have this is that I had a bug at some point: the case where it's not evident which table it is, is when you're adding a PK to a partitioned table and one of the partitions doesn't have the NOT NULL marking. But if you add a PK with the patch, the partitions are supposed to get the nullability marking automatically; the bug is that they didn't. So we don't need patch 0001 at all. > > 0002 is the most interesting part. Another thing I realized after posting, is that the constraint naming business is mistaken. It's currently written to work similarly to CHECK constraints, that is: each descendent needs to have the constraint named the same (this is so that descent works correctly when altering/dropping the constraint afterwards). But for NOT NULL constraints, that is not necessary, because when descending down the hierarchy, we can just match the constraint based on column name, since each column has at most one NOT NULL constraint. So the games with constraint renaming are altogether unnecessary and can be removed from the patch. We just need to ensure that coninhcount/conislocal is updated appropriately. > Where did this syntax come from: > > --- a/doc/src/sgml/ref/create_table.sgml > +++ b/doc/src/sgml/ref/create_table.sgml > @@ -77,6 +77,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | > UNLOGGED ] TABLE [ IF NOT EXI > > [ CONSTRAINT <replaceable class="parameter">constraint_name</replaceable> ] > { CHECK ( <replaceable class="parameter">expression</replaceable> ) [ NO > INHERIT ] | > + NOT NULL <replaceable class="parameter">column_name</replaceable> | > UNIQUE [ NULLS [ NOT ] DISTINCT ] ( <replaceable > class="parameter">column_name</replaceable> [, ... ] ) <replaceable > class="parameter">in> > PRIMARY KEY ( <replaceable class="parameter">column_name</replaceable> [, > ... ] ) <replaceable class="parameter">index_parameters</replac> > EXCLUDE [ USING <replaceable class="parameter">index_method</replaceable> > ] ( <replaceable class="parameter">exclude_element</replaceable> > > I don't see that in the standard. Yeah, I made it up because I needed table-level constraints for some reason that doesn't come to mind right now. > If we need it for something, we should at least document that it's an > extension. OK. > The test tables in foreign_key.sql are declared with columns like > > id bigint NOT NULL PRIMARY KEY, > > which is a bit weird and causes expected output diffs in your patch. Is > that interesting for this patch? Otherwise I suggest dropping the NOT NULL > from those table definitions to avoid these extra diffs. The behavior is completely different if you drop the primary key. If you don't have NOT NULL, then when you drop the PK the columns becomes nullable. If you do have a NOT NULL constraint in addition to the PK, and drop the PK, then the column remains non nullable. Now, if you want to suggest that dropping the PK ought to leave the column as NOT NULL (that is, it automatically acquires a NOT NULL constraint), then let's discuss that. But I understand the standard as saying otherwise. > > 0003: > > Since nobody liked the idea of listing the constraints in psql \d's > > footer, I changed \d+ so that the "not null" column shows the name of > > the constraint if there is one, or the string "(primary key)" if the > > attnotnull marking for the column comes from the primary key. The new > > column is going to be quite wide in some cases; if we want to hide it > > further, we could add the mythical \d++ and have *that* list the > > constraint name, keeping \d+ as current. > > I think my rough preference here would be to leave the existing output style > (column header "Nullable", content "not null") alone and display the > constraint name somewhere in the footer optionally. Well, there is resistance to showing the name of the constraint in the footer also because it's too verbose. In the end, I think a "super-verbose" mode is the most convincing way forward. (I think the list of partitions in the footer of a partitioned table is a terrible design. Let's not repeat that.) > In practice, the name of the constraint is rarely needed. That is true. > I do like the idea of mentioning primary key-ness inside the table somehow. Maybe change the "not null" to "primary key" in the Nullable column and nothing else. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en La Feria de las Tinieblas, R. Bradbury)
Here's v5. I removed the business of renaming constraints in child relations: recursing now just relies on matching column names. Each column has only one NOT NULL constraint; if you try to add another, nothing happens. All in all, this code is pretty similar to how we handle inheritance of columns, which I think is good. I added a mention that this funny syntax ALTER TABLE tab ADD CONSTRAINT NOT NULL col; is not standard. Maybe it's OK, but it seems a bit too prominent to me. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Вложения
On 15.03.23 23:44, Alvaro Herrera wrote: > Here's v5. I removed the business of renaming constraints in child > relations: recursing now just relies on matching column names. Each > column has only one NOT NULL constraint; if you try to add another, > nothing happens. All in all, this code is pretty similar to how we > handle inheritance of columns, which I think is good. This patch looks pretty okay to me now. It matches all the functional expectations. I suggest going through the tests carefully again and make sure all the changes are sensible and all the comments are correct. There are a few places where the behavior of tests has changed (intentionally) but the surrounding comments don't match anymore, or objects that previously weren't created now succeed but then affect following tests. Also, it seems some tests are left over from the first variant of this patch (where not-null constraints were converted to check constraints), and test names or comments should be updated to the current behavior. I suppose we don't need any changes in pg_dump, since ruleutils.c handles that? The information schema should be updated. I think the following views: - CHECK_CONSTRAINTS - CONSTRAINT_COLUMN_USAGE - DOMAIN_CONSTRAINTS - TABLE_CONSTRAINTS It looks like these have no test coverage; maybe that could be addressed at the same time.
On 27.03.23 15:55, Peter Eisentraut wrote: > The information schema should be updated. I think the following views: > > - CHECK_CONSTRAINTS > - CONSTRAINT_COLUMN_USAGE > - DOMAIN_CONSTRAINTS > - TABLE_CONSTRAINTS > > It looks like these have no test coverage; maybe that could be addressed > at the same time. Here are patches for this. I haven't included the expected files for the tests; this should be checked again that output is correct or the changes introduced by this patch set are as expected. The reason we didn't have tests for this before was probably in part because the information schema made up names for not-null constraints involving OIDs, so the test wouldn't have been stable. Feel free to integrate this, or we can add it on afterwards.
Вложения
On 2023-Mar-27, Peter Eisentraut wrote: > I suggest going through the tests carefully again and make sure all the > changes are sensible and all the comments are correct. There are a few > places where the behavior of tests has changed (intentionally) but the > surrounding comments don't match anymore, or objects that previously weren't > created now succeed but then affect following tests. Also, it seems some > tests are left over from the first variant of this patch (where not-null > constraints were converted to check constraints), and test names or comments > should be updated to the current behavior. Thanks for reviewing! Yeah, there were some obsolete tests. I fixed those, added a couple more, and while doing that I realized that failing to have NO INHERIT constraints may be seen as regressing feature-wise, because there would be no way to return to the situation where a parent table has a NOT NULL but the children don't necessarily. So I added that, and that led me to changing the code structure a bit more in order to support *not* copying the attnotnull flag in the cases where the parent only has it because of a NO INHERIT constraint. I'll go over this again tomorrow with fresh eyes, but I think it should be pretty close to ready. (Need to amend docs to note the new NO INHERIT option for NOT NULL table constraints, and make sure pg_dump complies.) Tests are currently running: https://cirrus-ci.com/build/6261827823206400 -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Las navajas y los monos deben estar siempre distantes" (Germán Poo)
Вложения
Hi, On 2023-04-06 01:33:56 +0200, Alvaro Herrera wrote: > I'll go over this again tomorrow with fresh eyes, but I think it should > be pretty close to ready. (Need to amend docs to note the new NO > INHERIT option for NOT NULL table constraints, and make sure pg_dump > complies.) Missed this thread somehow. This is not a review - I just want to say that I am very excited that we might finally catalogue NOT NULL constraints. This has been a long time in the making... Greetings, Andres Freund
On Wed, Apr 05, 2023 at 06:54:54PM -0700, Andres Freund wrote: > On 2023-04-06 01:33:56 +0200, Alvaro Herrera wrote: >> I'll go over this again tomorrow with fresh eyes, but I think it should >> be pretty close to ready. (Need to amend docs to note the new NO >> INHERIT option for NOT NULL table constraints, and make sure pg_dump >> complies.) > > Missed this thread somehow. This is not a review - I just want to say that I > am very excited that we might finally catalogue NOT NULL constraints. This has > been a long time in the making... +1! -- Michael
Вложения
On Thu, Apr 06, 2023 at 01:33:56AM +0200, Alvaro Herrera wrote: > - The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>), > + The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>, and > + except for the <literal>NOT NULL <replaceable>column_name</replaceable></literal> > + form to add a table constraint), The "except" part seems pretty incoherent to me :( > + if (isnull) > + elog(ERROR, "null conkey for NOT NULL constraint %u", conForm->oid); could use SysCacheGetAttrNotNull() > + if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) > + ereport(ERROR, > + errcode(ERRCODE_INVALID_TABLE_DEFINITION), > + errmsg("cannot add constraint to only the partitioned table when partitions exist"), > + errhint("Do not specify the ONLY keyword.")); > + else > + ereport(ERROR, > + errcode(ERRCODE_INVALID_TABLE_DEFINITION), > + errmsg("cannot add constraint to table with inheritance children"), missing "only" ? > + conrel = table_open(ConstraintRelationId, RowExclusiveLock); Should this be opened after the following error check ? > + arr = DatumGetArrayTypeP(adatum); /* ensure not toasted */ > + numkeys = ARR_DIMS(arr)[0]; > + if (ARR_NDIM(arr) != 1 || > + numkeys < 0 || > + ARR_HASNULL(arr) || > + ARR_ELEMTYPE(arr) != INT2OID) > + elog(ERROR, "conkey is not a 1-D smallint array"); > + attnums = (int16 *) ARR_DATA_PTR(arr); > + > + for (int i = 0; i < numkeys; i++) > + unconstrained_cols = lappend_int(unconstrained_cols, attnums[i]); > + } Does "arr" need to be freed ? > + * Since the above deletion has been made visible, we can now > + * search for any remaining constraints on this column (or these > + * columns, in the case we're dropping a multicol primary key.) > + * Then, verify whether any further NOT NULL or primary key exist, If I'm reading it right, I think it should say "exists" > +/* > + * When a primary key index on a partitioned table is to be attached an index > + * on a partition, the partition's columns should also be marked NOT NULL. > + * Ensure that is the case. I think the comment may be missing words, or backwards. The index on the *partitioned* table wouldn't be attached. Is the index on the *partition* that's attached *to* the former index. > +create table c() inherits(inh_p1, inh_p2, inh_p3, inh_p4); > +NOTICE: merging multiple inherited definitions of column "f1" > +NOTICE: merging multiple inherited definitions of column "f1" > +ERROR: relation "c" already exists Do you intend to make an error here ? Also, I think these table names may be too generic, and conflict with other parallel tests, now or in the future. > +create table d(a int not null, f1 int) inherits(inh_p3, c); > +ERROR: relation "d" already exists And here ? > +-- with explicitely specified not null constraints sp: explicitly -- Justin
On 2023-Apr-06, Justin Pryzby wrote: > On Thu, Apr 06, 2023 at 01:33:56AM +0200, Alvaro Herrera wrote: > > - The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>), > > + The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>, and > > + except for the <literal>NOT NULL <replaceable>column_name</replaceable></literal> > > + form to add a table constraint), > > The "except" part seems pretty incoherent to me :( Yeah, I feared that would be the case. I can't think of a wording that doesn't take two lines, so suggestions welcome. I handled your other comments, except these: > > + conrel = table_open(ConstraintRelationId, RowExclusiveLock); > > Should this be opened after the following error check ? Added new code in the middle when I found a small problem, so now the table_open is necessary there. (To wit: if we DROP NOT NULL a constraint that is both locally defined in the table and inherited, we should remove the "conislocal" flag and it's done. Previously, we were throwing an error that the constraint is inherited, but that's wrong.) > > + arr = DatumGetArrayTypeP(adatum); /* ensure not toasted */ > > Does "arr" need to be freed ? I see this pattern in one or two other places and we don't worry about such small allocations too much. (I copied this code almost verbatim from somewhere IIRC). Anyway, I found a couple of additional minor problems when playing with some additional corner case scenarios; I cleaned up the test cases, per Peter. Then I realized that pg_dump support was missing completely, so I filled that in. Sadly, the binary-upgrade mode is a bit of a mess and thus the pg_upgrade test is failing. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "La experiencia nos dice que el hombre peló millones de veces las patatas, pero era forzoso admitir la posibilidad de que en un caso entre millones, las patatas pelarían al hombre" (Ijon Tichy)
Вложения
I think this should fix the pg_upgrade issues. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "La conclusión que podemos sacar de esos estudios es que no podemos sacar ninguna conclusión de ellos" (Tanenbaum)
Вложения
On Fri, Apr 07, 2023 at 04:14:13AM +0200, Alvaro Herrera wrote: > On 2023-Apr-06, Justin Pryzby wrote: > > +ERROR: relation "c" already exists > > Do you intend to make an error here ? These still look like mistakes in the tests. > Also, I think these table names may be too generic, and conflict with > other parallel tests, now or in the future. > > > +create table d(a int not null, f1 int) inherits(inh_p3, c); > > +ERROR: relation "d" already exists > Sadly, the binary-upgrade mode is a bit of a mess and thus the > pg_upgrade test is failing.
Hi, I think there's some test instability: Fail: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=parula&dt=2023-04-07%2018%3A43%3A02 Subsequent success, without relevant changes: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=parula&dt=2023-04-07%2020%3A22%3A01 Followed by a failure: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=parula&dt=2023-04-07%2020%3A31%3A02 Similar failures on other animals: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=komodoensis&dt=2023-04-07%2020%3A27%3A43 https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=siskin&dt=2023-04-07%2020%3A09%3A25 There's also as second type of failure: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dragonet&dt=2023-04-07%2020%3A23%3A35 .. I suspect there's a naming conflict between tests in different groups. Greetings, Andres Freund
Hi, On 2023-04-07 13:38:43 -0700, Andres Freund wrote: > I suspect there's a naming conflict between tests in different groups. Yep: test: create_aggregate create_function_sql create_cast constraints triggers select inherit typed_table vacuum drop_if_existsupdatable_views roleattributes create_am hash_func errors infinite_recurse src/test/regress/sql/inherit.sql 851:create table child(f1 int not null, f2 text not null) inherits(inh_parent_1, inh_parent_2); src/test/regress/sql/triggers.sql 2127:create table child partition of parent for values in ('AAA'); 2266:create table child () inherits (parent); 2759:create table child () inherits (parent); The inherit.sql part is new. I'll see how hard it is to fix. Greetings, Andres Freund
On 2023-Apr-07, Andres Freund wrote: > src/test/regress/sql/triggers.sql > 2127:create table child partition of parent for values in ('AAA'); > 2266:create table child () inherits (parent); > 2759:create table child () inherits (parent); > > The inherit.sql part is new. Yeah. > I'll see how hard it is to fix. Running the tests for it now -- it's a short fix. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Learn about compilers. Then everything looks like either a compiler or a database, and now you have two problems but one of them is fun." https://twitter.com/thingskatedid/status/1456027786158776329
Hi, On 2023-04-07 23:00:01 +0200, Alvaro Herrera wrote: > On 2023-Apr-07, Andres Freund wrote: > > > src/test/regress/sql/triggers.sql > > 2127:create table child partition of parent for values in ('AAA'); > > 2266:create table child () inherits (parent); > > 2759:create table child () inherits (parent); > > > > The inherit.sql part is new. > > Yeah. > > > I'll see how hard it is to fix. > > Running the tests for it now -- it's a short fix. I just pushed a fix - sorry, I thought you might have stopped working for the day and CI finished with the modification a few seconds before your email arrived... Greetings, Andres Freund
On 2023-Apr-07, Andres Freund wrote: > I just pushed a fix - sorry, I thought you might have stopped working for the > day and CI finished with the modification a few seconds before your email > arrived... Ah, cool, no worries. I would have stopped indeed, but I had to stay around in case of any test failures. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "No me acuerdo, pero no es cierto. No es cierto, y si fuera cierto, no me acuerdo." (Augusto Pinochet a una corte de justicia)
Hi, On 2023-04-07 23:11:55 +0200, Alvaro Herrera wrote: > On 2023-Apr-07, Andres Freund wrote: > > > I just pushed a fix - sorry, I thought you might have stopped working for the > > day and CI finished with the modification a few seconds before your email > > arrived... > > Ah, cool, no worries. I would have stopped indeed, but I had to stay > around in case of any test failures. Looks like there's work for you if you want ;) https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2023-04-07%2018%3A52%3A13 But IMO fixing sepgsql can easily wait till tomorrow. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2023-04-07 23:11:55 +0200, Alvaro Herrera wrote: >> Ah, cool, no worries. I would have stopped indeed, but I had to stay >> around in case of any test failures. > Looks like there's work for you if you want ;) > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2023-04-07%2018%3A52%3A13 > But IMO fixing sepgsql can easily wait till tomorrow. I can deal with that one -- it's a bit annoying to work with sepgsql if you're not on a Red Hat platform. After quickly eyeing the diffs, I'm just going to take the new output as good. I'm not surprised that there are additional output messages given the additional catalog entries this made. I *am* a bit surprised that some messages seem to have disappeared --- are there places where this resulted in fewer catalog accesses than before? Nonetheless, there's no good reason to assume this test is exposing any bugs. regards, tom lane
Hi, On 2023-04-07 17:46:33 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2023-04-07 23:11:55 +0200, Alvaro Herrera wrote: > >> Ah, cool, no worries. I would have stopped indeed, but I had to stay > >> around in case of any test failures. > > > Looks like there's work for you if you want ;) > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2023-04-07%2018%3A52%3A13 > > > But IMO fixing sepgsql can easily wait till tomorrow. > > I can deal with that one -- it's a bit annoying to work with sepgsql > if you're not on a Red Hat platform. Indeed. I tried to get them running a while back, to enable the tests with meson, without lot of success. Then I realized that they're also not wired up in make... ;) > After quickly eyeing the diffs, I'm just going to take the new output > as good. I'm not surprised that there are additional output messages > given the additional catalog entries this made. I *am* a bit surprised > that some messages seem to have disappeared --- are there places where > this resulted in fewer catalog accesses than before? Nonetheless, > there's no good reason to assume this test is exposing any bugs. I wonder if the issue is that the new paths miss a hook invocation. @@ -160,11 +160,7 @@ ALTER TABLE regtest_table ALTER b SET DEFAULT 'XYZ'; -- not supported yet ALTER TABLE regtest_table ALTER b DROP DEFAULT; -- not supported yet ALTER TABLE regtest_table ALTER b SET NOT NULL; -LOG: SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema_2.regtest_table.b" permissive=0 -LOG: SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema.regtest_table_2.b" permissive=0 ALTER TABLE regtest_table ALTER b DROP NOT NULL; -LOG: SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema_2.regtest_table.b" permissive=0 -LOG: SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema.regtest_table_2.b" permissive=0 ALTER TABLE regtest_table ALTER b SET STATISTICS -1; LOG: SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema_2.regtest_table.b" permissive=0 LOG: SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema.regtest_table_2.b" permissive=0 The 'not supported yet' cases don't emit messages. Previously SET NOT NULL wasn't among that set, but seemingly it now is. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2023-04-07 17:46:33 -0400, Tom Lane wrote: >> After quickly eyeing the diffs, I'm just going to take the new output >> as good. I'm not surprised that there are additional output messages >> given the additional catalog entries this made. I *am* a bit surprised >> that some messages seem to have disappeared --- are there places where >> this resulted in fewer catalog accesses than before? Nonetheless, >> there's no good reason to assume this test is exposing any bugs. > I wonder if the issue is that the new paths miss a hook invocation. Perhaps. I'm content to silence the buildfarm for today; we can investigate more closely later. regards, tom lane
... BTW, shouldn't https://commitfest.postgresql.org/42/3869/ now get closed as committed? regards, tom lane
Hi, On 2023-04-07 18:26:28 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2023-04-07 17:46:33 -0400, Tom Lane wrote: > >> After quickly eyeing the diffs, I'm just going to take the new output > >> as good. I'm not surprised that there are additional output messages > >> given the additional catalog entries this made. I *am* a bit surprised > >> that some messages seem to have disappeared --- are there places where > >> this resulted in fewer catalog accesses than before? Nonetheless, > >> there's no good reason to assume this test is exposing any bugs. > > > I wonder if the issue is that the new paths miss a hook invocation. > > Perhaps. I'm content to silence the buildfarm for today; we can > investigate more closely later. Makes sense. I think https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2023-04-07%2021%3A16%3A04 might point out a problem with the pg_dump or pg_upgrade backward compat paths: --- C:\\prog\\bf/root/upgrade.drongo/HEAD/origin-REL9_5_STABLE.sql.fixed 2023-04-07 23:51:27.641328600 +0000 +++ C:\\prog\\bf/root/upgrade.drongo/HEAD/converted-REL9_5_STABLE-to-HEAD.sql.fixed 2023-04-07 23:51:27.672571900 +0000 @@ -416,9 +416,9 @@ -- Name: entry; Type: TABLE; Schema: public; Owner: buildfarm -- CREATE TABLE public.entry ( - accession text, - eid integer, - txid smallint + accession text NOT NULL, + eid integer NOT NULL, + txid smallint NOT NULL ); ALTER TABLE public.entry OWNER TO buildfarm; -- Looks like we're making up NOT NULL constraints when migrating from 9.5, for some reason? Greetings, Andres Freund
Hi, On 2023-04-07 17:19:42 -0700, Andres Freund wrote: > I think > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2023-04-07%2021%3A16%3A04 > might point out a problem with the pg_dump or pg_upgrade backward compat > paths: > > --- C:\\prog\\bf/root/upgrade.drongo/HEAD/origin-REL9_5_STABLE.sql.fixed 2023-04-07 23:51:27.641328600 +0000 > +++ C:\\prog\\bf/root/upgrade.drongo/HEAD/converted-REL9_5_STABLE-to-HEAD.sql.fixed 2023-04-07 23:51:27.672571900 +0000 > @@ -416,9 +416,9 @@ > -- Name: entry; Type: TABLE; Schema: public; Owner: buildfarm > -- > CREATE TABLE public.entry ( > - accession text, > - eid integer, > - txid smallint > + accession text NOT NULL, > + eid integer NOT NULL, > + txid smallint NOT NULL > ); > ALTER TABLE public.entry OWNER TO buildfarm; > -- > > Looks like we're making up NOT NULL constraints when migrating from 9.5, for > some reason? My compiler complains: ../../../../home/andres/src/postgresql/src/backend/catalog/heap.c: In function ‘AddRelationNotNullConstraints’: ../../../../home/andres/src/postgresql/src/backend/catalog/heap.c:2829:37: warning: ‘conname’ may be used uninitialized [-Wmaybe-uninitialized] 2829 | if (strcmp(lfirst(lc2), conname) == 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../../../home/andres/src/postgresql/src/backend/catalog/heap.c:2802:29: note: ‘conname’ was declared here 2802 | char *conname; | ^~~~~~~ I think the compiler may be right - I think the first use of conname might have been intended as constr->conname? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > I think > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2023-04-07%2021%3A16%3A04 > might point out a problem with the pg_dump or pg_upgrade backward compat > paths: Yeah, this patch has broken every single upgrade-from-back-branch test. I think there's a second problem, though: even without considering back branches, this has changed pg_dump output in a way that I fear is unacceptable. Consider for instance this table definition (from rules.sql): create table rule_and_refint_t1 ( id1a integer, id1b integer, primary key (id1a, id1b) ); This used to be dumped as CREATE TABLE public.rule_and_refint_t1 ( id1a integer NOT NULL, id1b integer NOT NULL ); ... ... load data ... ... ALTER TABLE ONLY public.rule_and_refint_t1 ADD CONSTRAINT rule_and_refint_t1_pkey PRIMARY KEY (id1a, id1b); In the new dispensation, pg_dump omits the NOT NULL clauses. Great, you say, that makes the output more like what the user wrote. I'm not so sure. This means that the ALTER TABLE will be compelled to perform a full-table scan to verify that there are no nulls in the already-loaded data before it can add the missing NOT NULL constraint. The old dump output was carefully designed to avoid the need for that scan. Admittedly, we have to do a scan anyway to build the index, so this is strictly less than a 2X penalty on the ALTER, but is that acceptable? It might be all right in the context of regular dump/restore, where we're surely doing a lot of per-row work anyway to load the data and make the index. In the context of pg_upgrade, though, it seems absolutely disastrous: there will now be a per-row cost where there was none before, and that is surely a deal-breaker. BTW, I note from testing that the NOT NULL clauses *are* still emitted in at least some cases when doing --binary-upgrade from an old version. (This may be directly related to the buildfarm failures, not sure.) That's no solution though, because now what you get in pg_constraint will differ depending on which way you upgraded, which seems unacceptable too. I'm inclined to think that this idea of suppressing the implied NOT NULL from PRIMARY KEY is a nonstarter and we should just go ahead and make such a constraint. Another idea could be for pg_dump to emit the NOT NULL, load data, do the ALTER ADD PRIMARY KEY, and then ALTER DROP NOT NULL. In any case, I wonder whether that's the sort of redesign we should be doing post-feature-freeze. It might be best to revert and try again in v17. regards, tom lane
On 2023-Apr-09, Tom Lane wrote: > In the new dispensation, pg_dump omits the NOT NULL clauses. > Great, you say, that makes the output more like what the user wrote. > I'm not so sure. This means that the ALTER TABLE will be compelled > to perform a full-table scan to verify that there are no nulls in the > already-loaded data before it can add the missing NOT NULL constraint. Yeah, I agree that this unintended consequence isn't very palatable. I think the other pg_upgrade problem is easily fixed (haven't tried yet), but having to rethink the pg_dump representation would likely take longer than we'd like. > I'm inclined to think that this idea of suppressing the implied > NOT NULL from PRIMARY KEY is a nonstarter and we should just > go ahead and make such a constraint. Another idea could be for > pg_dump to emit the NOT NULL, load data, do the ALTER ADD PRIMARY > KEY, and then ALTER DROP NOT NULL. I like that second idea, yeah. It might be tough to make it work, but I'll try. > In any case, I wonder whether that's the sort of redesign we should > be doing post-feature-freeze. It might be best to revert and try > again in v17. Yeah, sounds like reverting for now and retrying in v17 with the discussed changes might be better. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "La espina, desde que nace, ya pincha" (Proverbio africano)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> I'm inclined to think that this idea of suppressing the implied >> NOT NULL from PRIMARY KEY is a nonstarter and we should just >> go ahead and make such a constraint. Another idea could be for >> pg_dump to emit the NOT NULL, load data, do the ALTER ADD PRIMARY >> KEY, and then ALTER DROP NOT NULL. > I like that second idea, yeah. It might be tough to make it work, but > I'll try. Yeah, I've been thinking more about it, and this might also yield a workable solution for the TestUpgrade breakage. The idea would be, roughly, for pg_dump to emit NOT NULL column decoration in all the same places it does now, and then to drop it again immediately after doing ADD PRIMARY KEY if it judges that there was no other reason to have it. This gets rid of the inconsistency for --binary-upgrade which I think is what is causing the breakage. I also ran into something else I didn't much care for: regression=# create table foo(f1 int primary key, f2 int); CREATE TABLE regression=# create table foochild() inherits(foo); CREATE TABLE regression=# alter table only foo alter column f2 set not null; ERROR: cannot add constraint only to table with inheritance children HINT: Do not specify the ONLY keyword. Previous versions accepted this case, and I don't really see why we can't do so with this new implementation -- isn't this exactly what pg_constraint.connoinherit was invented to represent? Moreover, existing pg_dump files can contain precisely this construct, so blowing it off isn't going to be zero-cost. regards, tom lane
OK, so here's a new attempt to get this working correctly. This time I did try the new pg_upgrade when starting with a pg_dumpall produced by a server in branch 14 after running the regression tests. The pg_upgrade support is *really* finicky ... The main novelty in this version of the patch, is that we now emit "throwaway" NOT NULL constraints when a column is part of the primary key. Then, after the PK is created, we run a DROP for that constraint. That lets us create the PK without having to scan the table during pg_upgrade. (I thought about adding a new dump object, either one per table or just a single one for the whole dump, which would carry the ALTER TABLE .. DROP CONSTRAINT commands for those throwaway constraints. I decided that this is unnecessary, so the code the command in the same dump object that does ALTER TABLE ADD PRIMARY KEY seems good enough. If somebody sees a reason to do it differently, we can.) There's new funny business with RelationGetIndexList and primary keys of partitioned tables. With the patch, we continue to store the OID of the PK even when that index is marked invalid. The reason for this is pg_dump: when it does the ALTER TABLE to drop the NOT NULLs, the columns would become marked nullable, because since the PK is invalid, it's not considered to protect the columns. I guess it might be possible to implement this in some other way, but I found none that were reasonable. I didn't find that did had any undesirable side-effects anyway. Scanning this thread, I think I left one reported issue unfixed related to tables created LIKE others. I'll give it a look later. Other than that I think all bases are covered, but I intend to leave the patch open until near the end of the CF, in case someone wants to play with it. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "It takes less than 2 seconds to get to 78% complete; that's a good sign. A few seconds later it's at 90%, but it seems to have stuck there. Did somebody make percentages logarithmic while I wasn't looking?" http://smylers.hates-software.com/2005/09/08/1995c749.html
Вложения
Hi, On 2023-06-30 13:44:03 +0200, Alvaro Herrera wrote: > OK, so here's a new attempt to get this working correctly. Thanks for continuing to work on this! > The main novelty in this version of the patch, is that we now emit > "throwaway" NOT NULL constraints when a column is part of the primary > key. Then, after the PK is created, we run a DROP for that constraint. > That lets us create the PK without having to scan the table during > pg_upgrade. Have you considered extending the DDL statement for this purpose? We have ALTER TABLE ... ADD CONSTRAINT ... PRIMARY KEY USING INDEX ...; we could just do something similar for the NOT NULL constraint? Which would then delete the separate constraint NOT NULL constraint. Greetings, Andres Freund
On 2023-Jun-30, Andres Freund wrote: > On 2023-06-30 13:44:03 +0200, Alvaro Herrera wrote: > > > The main novelty in this version of the patch, is that we now emit > > "throwaway" NOT NULL constraints when a column is part of the primary > > key. Then, after the PK is created, we run a DROP for that constraint. > > That lets us create the PK without having to scan the table during > > pg_upgrade. > > Have you considered extending the DDL statement for this purpose? We have > ALTER TABLE ... ADD CONSTRAINT ... PRIMARY KEY USING INDEX ...; > we could just do something similar for the NOT NULL constraint? Which would > then delete the separate constraint NOT NULL constraint. Hmm, I hadn't. I think if we have to explicitly list the constraint that we want dropped, then it's pretty much the same than as if we used a comma-separated list of subcommands, like ALTER TABLE ... ADD CONSTRAINT .. PRIMARY KEY (a,b), DROP CONSTRAINT pgdump_throwaway_notnull_0, DROP CONSTRAINT pgdump_throwaway_notnull_1; However, I think it would be ideal if we *don't* have to specify the list of constraints: we would do this on any ALTER TABLE .. ADD CONSTRAINT PRIMARY KEY, without having any additional clause. But how to distinguish which NOT NULL markings to drop? Maybe we would have to specify a flag at NOT NULL constraint creation time. So pg_dump would emit something like CREATE TABLE foo (a int CONSTRAINT NOT NULL THROWAWAY); ... (much later) ... ALTER TABLE foo ADD CONSTRAINT .. PRIMARY KEY; and by the time this second command is run, those throwaway constraints are removed. The problems now are 1) how to make this CREATE statement more SQL-conformant (answer: make pg_dump emit a separate ALTER TABLE command for the constraint addition; it already knows how to do this, so it'd be very little code); but also 2) where to store the flag server-side flag that says this constraint has this property. I think it'd have to be a new pg_constraint column, and I don't like to add one for such a minor issue. On 2023-Jun-30, Alvaro Herrera wrote: > Scanning this thread, I think I left one reported issue unfixed related > to tables created LIKE others. I'll give it a look later. Other than > that I think all bases are covered, but I intend to leave the patch open > until near the end of the CF, in case someone wants to play with it. So it was [1] that I meant, where this example was provided: # create table t1 (c int primary key null unique); # create table t2 (like t1); # alter table t2 alter c drop not null; ERROR: no NOT NULL constraint found to drop The problem here is that because we didn't give INCLUDING INDEXES in the LIKE clause, we end up with a column marked NOT NULL for which we have no pg_constraint row. Okay, I thought, we can just make sure *not* to mark that case as not null; that works fine and looks reasonable. However, it breaks the following use case, which is already in use in the regression tests and possibly by users: CREATE TABLE pk (a int PRIMARY KEY) PARTITION BY RANGE (a); CREATE TABLE pk4 (LIKE pk); ALTER TABLE pk ATTACH PARTITION pk4 FOR VALUES FROM (3000) TO (4000); +ERROR: column "a" in child table must be marked NOT NULL The problem here is that we were assuming, by the time the third command is run, that the column had been marked NOT NULL by the second command. So my solution above is simply not acceptable. What we must do, in order to handle this backward-compatibly, is to ensure that a column part of a PK automatically gets a NOT NULL constraint for all the PK columns, for the case where INCLUDING INDEXES is not given. This is the same we do for regular INHERITS children and PKs. I'll go write this code now; should be simple enough. [1] https://postgr.es/m/CAMbWs48astPDb3K+L89wb8Yju0jM_Czm8svmU=Uzd+WM61Cr6Q@mail.gmail.com -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ <Schwern> It does it in a really, really complicated way <crab> why does it need to be complicated? <Schwern> Because it's MakeMaker.
On 30.06.23 13:44, Alvaro Herrera wrote: > OK, so here's a new attempt to get this working correctly. Attached is a small fixup patch for the documentation. Furthermore, there are a few outdated comments that are probably left over from previous versions of this patch set. * src/backend/catalog/pg_constraint.c Outdated comment: + /* only tuples for CHECK constraints should be given */ + Assert(((Form_pg_constraint) GETSTRUCT(constrTup))->contype == CONSTRAINT_NOTNULL); * src/backend/parser/gram.y Should processCASbits() set &n->skip_validation, like in the CHECK case? _outConstraint() looks at the field, so it seems relevant. * src/backend/parser/parse_utilcmd.c The updated comment says List *ckconstraints; /* CHECK and NOT NULL constraints */ but it seems to me that NOT NULL constraints are not actually added there but in nnconstraints instead. It would be nice if the field nnconstraints was listed after ckconstraints consistently throughout the file. It's a bit random right now. This comment is outdated: + /* + * For NOT NULL declarations, we need to mark the column as + * not nullable, and set things up to have a CHECK constraint + * created. Also, duplicate NOT NULL declarations are not + * allowed. + */ About this: case CONSTR_CHECK: cxt->ckconstraints = lappend(cxt->ckconstraints, constraint); + + /* + * XXX If the user says CHECK (IS NOT NULL), should we turn + * that into a regular NOT NULL constraint? + */ break; I think this was decided against. + /* + * Copy NOT NULL constraints, too (these do not require any option to have + * been given). + */ Shouldn't that be governed by the INCLUDING CONSTRAINTS option? Btw., there is some asymmetry here between check constraints and not-null constraints: Check constraints are in the tuple descriptor, but not-null constraints are not. Should that be unified? Or at least explained? * src/include/nodes/parsenodes.h This comment appears to be outdated: + * intermixed in tableElts, and constraints and notnullcols are NIL. After + * parse analysis, tableElts contains just ColumnDefs, notnullcols has been + * filled with not-nullable column names from various sources, and constraints + * contains just Constraint nodes (in fact, only CONSTR_CHECK nodes, in the + * present implementation). There is no "notnullcolls". * src/test/regress/parallel_schedule This change appears to be correct, but unrelated to this patch, so I suggest committing this separately. * src/test/regress/sql/create_table.sql -SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass; +SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::regclass ORDER BY coninhcount DESC, conname; Maybe add conname to the select list here as well, for consistency with nearby queries.
Вложения
On 2023-Jul-03, Peter Eisentraut wrote: > On 30.06.23 13:44, Alvaro Herrera wrote: > > OK, so here's a new attempt to get this working correctly. > > Attached is a small fixup patch for the documentation. > > Furthermore, there are a few outdated comments that are probably left over > from previous versions of this patch set. Thanks! I've incorporated your doc fixes and applied fixes for almost all the other issues you listed; and fixed a couple of additional issues, such as * adding a test to regress for an error message that wasn't covered (and removed the XXX comment about that) * remove a pointless variable addition to pg_dump (leftover from a previous implementation of constraint capture) * adapt the sepgsql tests again (we don't recurse to children when there's nothing to do, so an object hook invocation doesn't happen anymore -- I think) * made ATExecSetAttNotNull return the constraint address * more outdated comments adjustment in MergeAttributes Most importantly, I fixed table creation for LIKE inheritance, as I described upthread already. The one thing I have not touched is add ¬_valid to processCASbits() in gram.y; rather I added a comment that NOT VALID is not yet suported. I think adding support for that is a reasonably easy on top of this patch, but since it also requires more pg_dump support and stuff, I'd rather not mix it in at this point. The pg_upgrade support is already quite a house of cards and it drove me crazy. So, attached is v10. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Industry suffers from the managerial dogma that for the sake of stability and continuity, the company should be independent of the competence of individual employees." (E. Dijkstra)
Вложения
I left two questions unanswered here, so here I respond to them while giving one more revision of the patch. I realized that the AT_CheckNotNull stuff is now dead code, so in this version I remove it. I also changed on heap_getattr to SysCacheGetAttrNotNull, per a very old review comment from Justin that I hadn't acted upon. The other changes are minor code comments and test adjustments. At this point I think this is committable. On 2023-Jul-03, Peter Eisentraut wrote: > + /* > + * Copy NOT NULL constraints, too (these do not require any option to have > + * been given). > + */ > > Shouldn't that be governed by the INCLUDING CONSTRAINTS option? To clarify: this is in LIKE, such as CREATE TABLE (LIKE someother); and the reason we don't want to make this behavior depend on INCLUDING CONSTRAINTS, is backwards compatibility; NOT NULL markings have traditionally been propagated, so it can be used to create partitions based on the parent table, and if we made that require the option to be specified, that would no longer occur in the default case. Maybe we can change that behavior, but I'm pretty sure it would be resisted. > Btw., there is some asymmetry here between check constraints and > not-null constraints: Check constraints are in the tuple descriptor, > but not-null constraints are not. Should that be unified? Or at > least explained? Well, the reason check constraints are in the descriptor, is that they are needed to verify a table. NOT NULL constraint as catalog objects are (at present) only useful from a DDL point of view; they won't change the underlying implementation, which still depends on just the attnotnull markings. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Вложения
In this version I mistakenly included an unwanted change, which broke the test_ddl_deparse test. Here's v12 with that removed. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Las mujeres son como hondas: mientras más resistencia tienen, más lejos puedes llegar con ellas" (Jonas Nightingale, Leap of Faith)
Вложения
v13, because a conflict was just committed to alter_table.sql. Here I also call out the relcache.c change by making it a separate commit. I'm likely to commit it that way, too. To recap: the change is to have a partitioned table's index list include the primary key, even when said primary key is marked invalid. This turns out to be necessary for the currently proposed pg_dump strategy to work; if this is not in place, attaching the per-partition PK indexes to the parent index fails because it sees that the columns are not marked NOT NULL. I don't see any obvious problem with this change; but if someone does and this turns out to be unacceptable, then the pg_dump stuff would need some surgery. There are no other changes from v12. One thing I should probably get to, is fixing the constraint name comparison code in pg_dump. Right now it's a bit dumb and will get in silly trouble with overlength table/column names (nothing that would actually break, just that it will emit constraint names when there's no need to.) -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ Essentially, you're proposing Kevlar shoes as a solution for the problem that you want to walk around carrying a loaded gun aimed at your foot. (Tom Lane)
Вложения
On Wed, 12 Jul 2023 at 18:11, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > v13, because a conflict was just committed to alter_table.sql. > > Here I also call out the relcache.c change by making it a separate > commit. I'm likely to commit it that way, too. To recap: the change is > to have a partitioned table's index list include the primary key, even > when said primary key is marked invalid. This turns out to be necessary > for the currently proposed pg_dump strategy to work; if this is not in > place, attaching the per-partition PK indexes to the parent index fails > because it sees that the columns are not marked NOT NULL. > Hmm, looking at that change, it looks a little ugly. I think someone reading that code in the future will have no idea why it's including some invalid indexes, and not others. > There are no other changes from v12. One thing I should probably get > to, is fixing the constraint name comparison code in pg_dump. Right now > it's a bit dumb and will get in silly trouble with overlength > table/column names (nothing that would actually break, just that it will > emit constraint names when there's no need to.) > Yeah, that seems a bit ugly. Presumably, also, if something like a column rename happens, the constraint name will no longer match. I see that it's already been discussed, but I don't like the fact that there is no way to get hold of the new constraint names in psql. I think for the purposes of dropping named constraints, and also possible future stuff like NOT VALID / DEFERRABLE constraints, having some way to get their names will be important. Something else I noticed is that the result from "ALTER TABLE ... ALTER COLUMN ... DROP NOT NULL" is no longer easily predictable -- if there are multiple NOT NULL constraints on the column, it just drops one (chosen at random?) and leaves the others. I think that it should either drop all the constraints, or throw an error. Either way, I would expect that if DROP NOT NULL succeeds, the result is that the column is nullable. Regards, Dean
On 2023-Jul-13, Dean Rasheed wrote: > Hmm, looking at that change, it looks a little ugly. I think someone > reading that code in the future will have no idea why it's including > some invalid indexes, and not others. True. I've added a longish comment in 0001 to explain why we do this. 0002 has two bugfixes, described below. > On Wed, 12 Jul 2023 at 18:11, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > There are no other changes from v12. One thing I should probably get > > to, is fixing the constraint name comparison code in pg_dump. Right now > > it's a bit dumb and will get in silly trouble with overlength > > table/column names (nothing that would actually break, just that it will > > emit constraint names when there's no need to.) > > Yeah, that seems a bit ugly. Presumably, also, if something like a > column rename happens, the constraint name will no longer match. Well, we never rename constraints (except AFAIR for unique constraints when the unique index is renamed), and I'm not sure that it's a good idea to automatically rename a not null constraint when the column or the table are renamed. (I think trying to make pg_dump be smarter about the constraint name when the table/column names are very long, would require exporting makeObjectName() for frontend use. It looks an easy task, but I haven't done it.) (Maybe it would be reasonable to rename the NOT NULL constraint when the table or column are renamed, iff the original constraint name is the default one. Initially I lean towards not doing it, though.) Anyway, what does happen when the name doesn't match what pg_dump thinks is the default name (<table>_<column>_not_null) is that the constraint name is printed in the output. So if you have this table create table one (a int not null, b int not null); and rename column b to c, then pg_dump will print the table like this: CREATE TABLE public.one ( a integer NOT NULL, c integer CONSTRAINT one_b_not_null NOT NULL ); In other words, the name is preserved across a dump. I think this is not terrible. > I see that it's already been discussed, but I don't like the fact that > there is no way to get hold of the new constraint names in psql. I > think for the purposes of dropping named constraints, and also > possible future stuff like NOT VALID / DEFERRABLE constraints, having > some way to get their names will be important. Yeah, so there are two proposals: 1. Have \d+ replace the "not null" literal in the \d+ display with the constraint name; if the column is not nullable because of the primary key, it says "(primary key)" instead. There's a patch for this in the thread somewhere. 2. Same, but use \d++ for this purpose Using ++ would be a novelty in psql, so I'm hesitant to make that an integral part of the current proposal. However, to me (2) seems to most comfortable way forward, because while you're right that people do need the constraint name from time to time, this is very seldom the case, so polluting \d+ might inconvenience people for not much gain. And people didn't like having the constraint name in \d+. Do you have an opinion on these ideas? > Something else I noticed is that the result from "ALTER TABLE ... > ALTER COLUMN ... DROP NOT NULL" is no longer easily predictable -- if > there are multiple NOT NULL constraints on the column, it just drops > one (chosen at random?) and leaves the others. I think that it should > either drop all the constraints, or throw an error. Either way, I > would expect that if DROP NOT NULL succeeds, the result is that the > column is nullable. Hmm, there shouldn't be multiple NOT NULL constraints for the same column; if there's one, a further SET NOT NULL should do nothing. At some point the code was creating two constraints, but I realized that trying to support multiple constraints caused other problems, and it seems to serve no purpose, so I removed it. Maybe there are ways to end up with multiple constraints, but at this stage I would say that those are bugs to be fixed, rather than something we want to keep. ... oh, I did find a bug here -- indeed, ALTER TABLE tab ADD CONSTRAINT NOT NULL col; was not checking whether a constraint already existed, and created a duplicate. In v14-0002 I made that throw an error instead. And having done that, I discovered another bug: in test_ddl_deparse we CREATE TABLE LIKE from SERIAL PRIMARY KEY column, so that was creating two NOT NULL constraints, one for the lack of INCLUDING INDEXES on the PK, and another for the NOT NULL itself which comes implicit with SERIAL. So I fixed that too, by having transformTableLikeClause skip creating a NOT NULL for PK columns if we're going to create one for a NOT NULL constraint. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "El número de instalaciones de UNIX se ha elevado a 10, y se espera que este número aumente" (UPM, 1972)
Вложения
On Thu, 20 Jul 2023 at 16:31, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2023-Jul-13, Dean Rasheed wrote: > > > I see that it's already been discussed, but I don't like the fact that > > there is no way to get hold of the new constraint names in psql. I > > think for the purposes of dropping named constraints, and also > > possible future stuff like NOT VALID / DEFERRABLE constraints, having > > some way to get their names will be important. > > Yeah, so there are two proposals: > > 1. Have \d+ replace the "not null" literal in the \d+ display with the > constraint name; if the column is not nullable because of the primary > key, it says "(primary key)" instead. There's a patch for this in the > thread somewhere. > > 2. Same, but use \d++ for this purpose > > Using ++ would be a novelty in psql, so I'm hesitant to make that an > integral part of the current proposal. However, to me (2) seems to most > comfortable way forward, because while you're right that people do need > the constraint name from time to time, this is very seldom the case, so > polluting \d+ might inconvenience people for not much gain. And people > didn't like having the constraint name in \d+. > > Do you have an opinion on these ideas? > Hmm, I don't particularly like that approach, because I think it will be difficult to cram any additional details into the table, and also I don't know whether having multiple not null constraints for a particular column can be entirely ruled out. I may well be in the minority here, but I think the best way is to list them in a separate footer section, in the same way as CHECK constraints, allowing other constraint properties to be included. So it might look something like: \d foo Table "public.foo" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- a | integer | | not null | b | integer | | not null | c | integer | | not null | d | integer | | not null | Indexes: "foo_pkey" PRIMARY KEY, btree (a, b) Check constraints: "foo_a_check" CHECK (a > 0) "foo_b_check" CHECK (b > 0) NO INHERIT NOT VALID Not null constraints: "foo_c_not_null" NOT NULL c "foo_d_not_null" NOT NULL d NO INHERIT As for CHECK constraints, the contents of each constraint line would match the "table_constraint" SQL syntax needed to reconstruct the constraint. Doing it this way allows for things like NOT VALID and DEFERRABLE to be added in the future. I think that's probably too verbose for a plain "\d", but I think it would be OK with "\d+". Regards, Dean
On Thu, 20 Jul 2023 at 16:31, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2023-Jul-13, Dean Rasheed wrote: > > > Something else I noticed is that the result from "ALTER TABLE ... > > ALTER COLUMN ... DROP NOT NULL" is no longer easily predictable -- if > > there are multiple NOT NULL constraints on the column, it just drops > > one (chosen at random?) and leaves the others. I think that it should > > either drop all the constraints, or throw an error. Either way, I > > would expect that if DROP NOT NULL succeeds, the result is that the > > column is nullable. > > Hmm, there shouldn't be multiple NOT NULL constraints for the same > column; if there's one, a further SET NOT NULL should do nothing. At > some point the code was creating two constraints, but I realized that > trying to support multiple constraints caused other problems, and it > seems to serve no purpose, so I removed it. Maybe there are ways to end > up with multiple constraints, but at this stage I would say that those > are bugs to be fixed, rather than something we want to keep. > Hmm, I'm not so sure. I think perhaps multiple NOT NULL constraints on the same column should just be allowed, otherwise things might get confusing. For example: create table p1 (a int not null check (a > 0)); create table p2 (a int not null check (a > 0)); create table foo () inherits (p1, p2); causes foo to have 2 CHECK constraints, but only 1 NOT NULL constraint: \d foo Table "public.foo" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- a | integer | | not null | Check constraints: "p1_a_check" CHECK (a > 0) "p2_a_check" CHECK (a > 0) Inherits: p1, p2 select conname from pg_constraint where conrelid = 'foo'::regclass; conname --------------- p1_a_not_null p2_a_check p1_a_check (3 rows) which I find a little counter-intuitive / inconsistent. If I then drop the p1 constraints: alter table p1 drop constraint p1_a_check; alter table p1 drop constraint p1_a_not_null; I end up with column "a" still being not null, and the "p1_a_not_null" constraint still being there on foo, which seems even more counter-intuitive, because I just dropped that constraint, and it really should now be the "p2_a_not_null" constraint that makes "a" not null: \d foo Table "public.foo" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- a | integer | | not null | Check constraints: "p2_a_check" CHECK (a > 0) Inherits: p1, p2 select conname from pg_constraint where conrelid = 'foo'::regclass; conname --------------- p1_a_not_null p2_a_check (2 rows) I haven't thought through various other cases in any detail, but I can't help feeling that it would be simpler and more logical / consistent to just allow multiple NOT NULL constraints on a column, rather than trying to enforce a rule that only one is allowed. That way, I think it would be easier for the user to keep track of why a column is not null. So I'd say that ALTER TABLE ... ADD NOT NULL should always add a constraint, even if there already is one. For example ALTER TABLE ... ADD UNIQUE does nothing to prevent multiple unique constraints on the same column(s). It seems pretty dumb, but maybe there is a reason to allow it, and it doesn't feel like we should be second-guessing what the user wants. Regards, Dean
Something else I noticed: the error message from ALTER TABLE ... ADD CONSTRAINT in the case of a duplicate constraint name is not very friendly: ERROR: duplicate key value violates unique constraint "pg_constraint_conrelid_contypid_conname_index" DETAIL: Key (conrelid, contypid, conname)=(16540, 0, nn) already exists. To match the error message for other constraint types, this should be: ERROR: constraint "nn" for relation "foo" already exists Regards, Dean
On 2023-Jul-24, Dean Rasheed wrote: > Hmm, I don't particularly like that approach, because I think it will > be difficult to cram any additional details into the table, and also I > don't know whether having multiple not null constraints for a > particular column can be entirely ruled out. > > I may well be in the minority here, but I think the best way is to > list them in a separate footer section, in the same way as CHECK > constraints, allowing other constraint properties to be included. So > it might look something like: That's the first thing I proposed actually. I got one vote down from Robert Haas[1], but while the idea seems to have had support from Justin Pryzby (in \dt++) [2] and definitely did from Peter Eisentraut [3], I do not like it too much myself, mainly because the partition list has a very similar treatment and I find that one an annoyance. > and also I don't know whether having multiple not null constraints for > a particular column can be entirely ruled out. I had another look at the standard. In 11.26 (<drop table constraint definition>) it says that "If [the constraint being removed] causes some column COL to be known not nullable and no other constraint causes COL to be known not nullable, then the nullability characteristic of the column descriptor of COL is changed to possibly nullable". Which supports the idea that there might be multiple such constraints. (However, we could also read this as meaning that the PK could be one such constraint while NOT NULL is another one.) However, 11.16 (<drop column not null clause> as part of 11.12 <alter column definition>), says that DROP NOT NULL causes the indication of the column as NOT NULL to be removed. This, to me, says that if you do have multiple such constraints, you'd better remove them all with that command. All in all, I lean towards allowing just one as best as we can. [1] https://postgr.es/m/CA+Tgmobnoxt83y1QesBNVArhFm-fLwWkDUyiV84e+psayDwB7A@mail.gmail.com [2] https://postgr.es/m/20230301223214.GC4268%40telsasoft.com [3] https://postgr.es/m/1c4f3755-2d10-cae9-647f-91a9f006410e%40enterprisedb.com -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ “Cuando no hay humildad las personas se degradan” (A. Christie)
On 2023-Jul-24, Dean Rasheed wrote: > Hmm, I'm not so sure. I think perhaps multiple NOT NULL constraints on > the same column should just be allowed, otherwise things might get > confusing. For example: > create table p1 (a int not null check (a > 0)); create table p2 (a int not null check (a > 0)); create table foo () inherits (p1, p2); Have a look at the conislocal / coninhcount values. These should reflect the fact that the constraint has multiple sources; and the constraint does disappear if you drop it from both sources. > If I then drop the p1 constraints: > > alter table p1 drop constraint p1_a_check; > alter table p1 drop constraint p1_a_not_null; > > I end up with column "a" still being not null, and the "p1_a_not_null" > constraint still being there on foo, which seems even more > counter-intuitive, because I just dropped that constraint, and it > really should now be the "p2_a_not_null" constraint that makes "a" not > null: I can see that it might make sense to not inherit the constraint name in some cases. Perhaps: 1. never inherit a name. Each table has its own constraint name always 2. only inherit if there's a single parent 3. always inherit the name from the first parent (current implementation) > So I'd say that ALTER TABLE ... ADD NOT NULL should always add a > constraint, even if there already is one. For example ALTER TABLE ... > ADD UNIQUE does nothing to prevent multiple unique constraints on the > same column(s). It seems pretty dumb, but maybe there is a reason to > allow it, and it doesn't feel like we should be second-guessing what > the user wants. That was my initial implementation but I changed it to allowing a single constraint because of the way the standard describes SET NOT NULL; specifically, 11.15 <set column not null clause> says that "If the column descriptor of C does not contain an indication that C is defined as NOT NULL, then:" a constraint is added; otherwise (i.e., such an indication does exist), nothing happens. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "La virtud es el justo medio entre dos defectos" (Aristóteles)
On 2023-Jul-24, Dean Rasheed wrote: > Something else I noticed: the error message from ALTER TABLE ... ADD > CONSTRAINT in the case of a duplicate constraint name is not very > friendly: > > ERROR: duplicate key value violates unique constraint > "pg_constraint_conrelid_contypid_conname_index" > DETAIL: Key (conrelid, contypid, conname)=(16540, 0, nn) already exists. > > To match the error message for other constraint types, this should be: > > ERROR: constraint "nn" for relation "foo" already exists Hmm, how did you get this one? I can't reproduce it: 55490 17devel 3166154=# create table foo (a int constraint nn not null); CREATE TABLE 55490 17devel 3166154=# alter table foo add constraint nn not null a; ERROR: column "a" of table "foo" is already NOT NULL 55490 17devel 3166154=# drop table foo; DROP TABLE 55490 17devel 3166154=# create table foo (a int); CREATE TABLE Duración: 1,472 ms 55490 17devel 3166154=# alter table foo add constraint nn not null a, add constraint nn not null a; ERROR: column "a" of table "foo" is already NOT NULL -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "El número de instalaciones de UNIX se ha elevado a 10, y se espera que este número aumente" (UPM, 1972)
Hello, While discussing the matter of multiple constraints with Vik Fearing, I noticed that we were throwing an unnecessary error if you used CREATE TABLE foo (a int NOT NULL NOT NULL); That would die with "redundant NOT NULL declarations", but current master doesn't do that; and we don't do it for UNIQUE UNIQUE either. So I modified the patch to make it ignore the dupe and create a single constraint. This (and rebasing to current master) are the only changes in v15. I have not changed the psql presentation, but I'll do as soon as we have rough consensus on what to do. To reiterate, the options are: 1. Don't show the constraint names. This is what the current patch does 2. Show the constraint name in \d+ in the "nullable" column. I did this early on, to much booing. 3. Show the constraint name in \d++ (a new command) tabular output 4. Show the constraint name in the footer of \d+ I also did this at some point; there are some +1s and some -1s. 5. Show the constraint name in the footer of \d++ Many thanks, Dean, for the discussion so far. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'. After collecting 500 such letters, he mused, a university somewhere in Arizona would probably grant him a degree. (Don Knuth)
On 2023-Jul-24, Alvaro Herrera wrote: > That would die with "redundant NOT NULL declarations", but current > master doesn't do that; and we don't do it for UNIQUE UNIQUE either. > So I modified the patch to make it ignore the dupe and create a single > constraint. This (and rebasing to current master) are the only changes > in v15. Did I forget the attachments once more? Yup, I did. Here they are. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Вложения
On 7/24/23 18:42, Alvaro Herrera wrote: > 55490 17devel 3166154=# create table foo (a int constraint nn not null); > CREATE TABLE > 55490 17devel 3166154=# alter table foo add constraint nn not null a; > ERROR: column "a" of table "foo" is already NOT NULL Surely this should be a WARNING or INFO? I see no reason to ERROR here. -- Vik Fearing
On Mon, 24 Jul 2023 at 17:42, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2023-Jul-24, Dean Rasheed wrote: > > > Something else I noticed: the error message from ALTER TABLE ... ADD > > CONSTRAINT in the case of a duplicate constraint name is not very > > friendly: > > > > ERROR: duplicate key value violates unique constraint > > "pg_constraint_conrelid_contypid_conname_index" > > DETAIL: Key (conrelid, contypid, conname)=(16540, 0, nn) already exists. > > To reproduce this error, try to create 2 constraints with the same name on different columns: create table foo(a int, b int); alter table foo add constraint nn not null a; alter table foo add constraint nn not null b; I found another, separate issue: create table p1(a int not null); create table p2(a int); create table foo () inherits (p1,p2); alter table p2 add not null a; ERROR: column "a" of table "foo" is already NOT NULL whereas doing "alter table p2 alter column a set not null" works OK, merging the constraints as expected. Regards, Dean
On Mon, Jul 24, 2023 at 6:33 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > That's the first thing I proposed actually. I got one vote down from > Robert Haas[1], but while the idea seems to have had support from Justin > Pryzby (in \dt++) [2] and definitely did from Peter Eisentraut [3], I do > not like it too much myself, mainly because the partition list has a > very similar treatment and I find that one an annoyance. I think I might want to retract my earlier -1 vote. I mean, I agree with former me that having the \d+ output get a whole lot longer is not super-appealing. But I also agree with Dean that having this information available somewhere is probably important, and I also agree with your point that inventing \d++ for this isn't necessarily a good idea. I fear that will just result in having to type an extra plus sign any time you want to see all of the table details, to make sure that psql knows that you really mean it. So, maybe showing it in the \d+ output as Dean proposes is the least of evils. -- Robert Haas EDB: http://www.enterprisedb.com
On 2023-Jul-24, Robert Haas wrote: > I think I might want to retract my earlier -1 vote. I mean, I agree > with former me that having the \d+ output get a whole lot longer is > not super-appealing. But I also agree with Dean that having this > information available somewhere is probably important, and I also > agree with your point that inventing \d++ for this isn't necessarily a > good idea. I fear that will just result in having to type an extra > plus sign any time you want to see all of the table details, to make > sure that psql knows that you really mean it. So, maybe showing it in > the \d+ output as Dean proposes is the least of evils. Okay then, I've made these show up in the footer of \d+. This is in patch 0003 here. Please let me know what do you think of the regression changes. On 2023-Jul-24, Dean Rasheed wrote: > To reproduce this error, try to create 2 constraints with the same > name on different columns: > > create table foo(a int, b int); > alter table foo add constraint nn not null a; > alter table foo add constraint nn not null b; Ah, of course. Fixed. > I found another, separate issue: > > create table p1(a int not null); > create table p2(a int); > create table foo () inherits (p1,p2); > alter table p2 add not null a; > > ERROR: column "a" of table "foo" is already NOT NULL > > whereas doing "alter table p2 alter column a set not null" works OK, > merging the constraints as expected. True. I made it a non-error. I initially changed the message to INFO, as suggested by Vik nearby; but after noticing that SET NOT NULL just does the same thing with no message, I removed this message altogether, for consistence. Now that I did it, though, I wonder: if the user specified a constraint name, and that name does not match the existing constraint, maybe we should have an INFO or NOTICE or WARNING message that the requested constraint name was not satisfied. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Вложения
On Tue, Jul 25, 2023 at 8:36 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Okay then, I've made these show up in the footer of \d+. This is in > patch 0003 here. Please let me know what do you think of the regression > changes. Seems OK. I'm not really thrilled with the idea of every not-null constraint having a name, to be honest. Of all the kinds of constraints that we have in the system, NOT NULL constraints are probably the ones where naming them is least likely to be interesting, because they don't really have any interesting properties. A CHECK constraint has an expression; a foreign key constraint has columns that it applies to on each side plus the identity of the table and opclass information, but a NOT NULL constraint seems like it can never have any property other than which column. So it sort of seems like a waste to name it. But if we want it catalogued then we don't really have an option, so I suppose we just have to accept a bit of clutter as the price of doing business. -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, 25 Jul 2023 at 11:39, Robert Haas <robertmhaas@gmail.com> wrote:
I'm not really thrilled with the idea of every not-null constraint
having a name, to be honest. Of all the kinds of constraints that we
have in the system, NOT NULL constraints are probably the ones where
naming them is least likely to be interesting, because they don't
really have any interesting properties. A CHECK constraint has an
expression; a foreign key constraint has columns that it applies to on
each side plus the identity of the table and opclass information, but
a NOT NULL constraint seems like it can never have any property other
than which column. So it sort of seems like a waste to name it. But if
we want it catalogued then we don't really have an option, so I
suppose we just have to accept a bit of clutter as the price of doing
business.
I agree. I definitely do *not* want a bunch of NOT NULL constraint names cluttering up displays. Can we legislate that all NOT NULL implementing constraints are named by mashing together the table name, column name, and something to identify it as a NOT NULL constraint? Maybe even something like pg_not_null_[relname]_[attname] (with some escaping), using the pg_ prefix to make the name reserved similar to schemas and tables? And then don't show such constraints in \d, not even \d+ - just indicate it in the Nullable column of the column listing as done now. Show a NOT NULL constraint if there is something odd about it - for example, if it gets renamed, or not renamed when the table is renamed.
Sorry for the noise if this has already been decided otherwise.
On 2023-Jul-25, Isaac Morland wrote: > I agree. I definitely do *not* want a bunch of NOT NULL constraint names > cluttering up displays. Can we legislate that all NOT NULL implementing > constraints are named by mashing together the table name, column name, and > something to identify it as a NOT NULL constraint? All constraints are named like that already, and NOT NULL constraints just inherited the same idea. The names are <table>_<column>_not_null for NOT NULL constraints. pg_dump goes great lengths to avoid printing constraint names when they have this pattern. I do not want these constraint names cluttering the output either. That's why I propose moving them to a new \d++ command, where they will only bother you if you absolutely need them. But so far I have only one vote supporting that idea. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Tue, 25 Jul 2023 at 12:24, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2023-Jul-25, Isaac Morland wrote:
> I agree. I definitely do *not* want a bunch of NOT NULL constraint names
> cluttering up displays. Can we legislate that all NOT NULL implementing
> constraints are named by mashing together the table name, column name, and
> something to identify it as a NOT NULL constraint?
All constraints are named like that already, and NOT NULL constraints
just inherited the same idea. The names are <table>_<column>_not_null
for NOT NULL constraints. pg_dump goes great lengths to avoid printing
constraint names when they have this pattern.
OK, this is helpful. Can \d do the same thing? I use a lot of NOT NULL constraints and I very seriously do not want \d (including \d+) to have an extra line for almost every column. It's just noise, and while my screen is large, it's still not infinite.
I do not want these constraint names cluttering the output either.
That's why I propose moving them to a new \d++ command, where they will
only bother you if you absolutely need them. But so far I have only one
vote supporting that idea.
My suggestion is for \d+ to show NOT NULL constraints only if there is something weird going on (wrong name, duplicate constraints, …). If there is nothing weird about the constraint then explicitly listing it provides absolutely no information that is not given by "not null" in the "Nullable" column. Easier said than done I suppose. I'm just worried about my \d+ displays becoming less useful.
On Tue, Jul 25, 2023 at 1:33 PM Isaac Morland <isaac.morland@gmail.com> wrote: > My suggestion is for \d+ to show NOT NULL constraints only if there is something weird going on (wrong name, duplicateconstraints, …). If there is nothing weird about the constraint then explicitly listing it provides absolutely noinformation that is not given by "not null" in the "Nullable" column. Easier said than done I suppose. I'm just worriedabout my \d+ displays becoming less useful. I mean, the problem is that if you want to ALTER TABLE .. DROP CONSTRAINT, you need to know what the valid arguments to that command are, and the names of these constraints will be just as valid as the names of any other constraints. -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, 25 Jul 2023 at 14:59, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jul 25, 2023 at 1:33 PM Isaac Morland <isaac.morland@gmail.com> wrote:
> My suggestion is for \d+ to show NOT NULL constraints only if there is something weird going on (wrong name, duplicate constraints, …). If there is nothing weird about the constraint then explicitly listing it provides absolutely no information that is not given by "not null" in the "Nullable" column. Easier said than done I suppose. I'm just worried about my \d+ displays becoming less useful.
I mean, the problem is that if you want to ALTER TABLE .. DROP
CONSTRAINT, you need to know what the valid arguments to that command
are, and the names of these constraints will be just as valid as the
names of any other constraints.
Can't I just ALTER TABLE … DROP NOT NULL still?
OK, I suppose ALTER CONSTRAINT to change the deferrable status and validity (that is why we're doing this, right?) needs the constraint name. But the constraint name is formulaic by default, and my proposal is to suppress it only when it matches the formula, so you could just construct the constraint name using the documented formula if it's not explicitly listed.
I really don’t see it as a good use of space to add n lines to the \d+ display just to confirm that the "not null" designations in the "Nullable" column are implemented by named constraints with the expected names.
On Tue, Jul 25, 2023 at 3:07 PM Isaac Morland <isaac.morland@gmail.com> wrote: > OK, I suppose ALTER CONSTRAINT to change the deferrable status and validity (that is why we're doing this, right?) needsthe constraint name. But the constraint name is formulaic by default, and my proposal is to suppress it only when itmatches the formula, so you could just construct the constraint name using the documented formula if it's not explicitlylisted. > > I really don’t see it as a good use of space to add n lines to the \d+ display just to confirm that the "not null" designationsin the "Nullable" column are implemented by named constraints with the expected names. Yeah, I mean, I get that. That was my initial concern, too. But I also think if there's some complicated rule that determines what gets displayed and what doesn't, nobody's going to remember it, and then when you don't see something, you're never going to be sure exactly what's going on. Displaying everything is going to be clunky especially if, like me, you tend to be careful to mark columns NOT NULL when they are, but when something goes wrong, the last thing you want to do is run a \d command and have it show you incomplete information. I can't count the number of times that somebody's shown me the output of a query against pg_locks or pg_stat_activity that had been filtered to remove irrelevant information and it turned out that the hidden information was not so irrelevant as the person who wrote the query thought. It happens all the time. I don't want to create the same kind of situation here. -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, 25 Jul 2023 at 13:36, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Okay then, I've made these show up in the footer of \d+. This is in > patch 0003 here. Please let me know what do you think of the regression > changes. > The new \d+ output certainly makes testing and reviewing easier, though I do understand people's concerns that this may make the output significantly longer in many real-world cases. I don't think it would be a good idea to filter the list in any way though, because I think that will only lead to confusion. I think it should be all-or-nothing, though I'm not necessarily opposed to using something like \d++ to enable it, if that turns out to be the least-bad option. Going back to this example: drop table if exists p1, p2, foo; create table p1(a int not null check (a > 0)); create table p2(a int not null check (a > 0)); create table foo () inherits (p1,p2); \d+ foo Table "public.foo" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description --------+---------+-----------+----------+---------+---------+-------------+--------------+------------- a | integer | | not null | | plain | | | Check constraints: "p1_a_check" CHECK (a > 0) "p2_a_check" CHECK (a > 0) Not null constraints: "p1_a_not_null" NOT NULL "a" (inherited) Inherits: p1, p2 Access method: heap I remain of the opinion that that should create 2 NOT NULL constraints on foo, for consistency with CHECK constraints, and the misleading name that results if p1_a_not_null is dropped from p1. That way, the names of inherited NOT NULL constraints could be kept in sync, as they are for other constraint types, making it easier to keep track of where they come from, and it wouldn't be necessary to treat them differently (e.g., matching by column number, when dropping NOT NULL constraints). Doing a little more testing, I found some other issues. Given the following sequence: drop table if exists p,c; create table p(a int primary key); create table c() inherits (p); alter table p drop constraint p_pkey; p.a ends up being nullable, where previously it would have been left non-nullable. That change makes sense, and is presumably one of the benefits of tying the nullability of columns to pg_constraint entries. However, c.a remains non-nullable, with a NOT NULL constraint that claims to be inherited: \d+ c Table "public.c" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description --------+---------+-----------+----------+---------+---------+-------------+--------------+------------- a | integer | | not null | | plain | | | Not null constraints: "c_a_not_null" NOT NULL "a" (inherited) Inherits: p Access method: heap That's a problem, because now the NOT NULL constraint on c cannot be dropped (attempting to drop it on c errors out because it thinks it's inherited, but it can't be dropped via p, because p.a is already nullable). I wonder if NOT NULL constraints created as a result of inherited PKs should have names based on the PK name (e.g., <PK_name>_<col_name>_not_null), to make it more obvious where they came from. That would be more consistent with the way NOT NULL constraint names are inherited. Given the following sequence: drop table if exists p,c; create table p(a int); create table c() inherits (p); alter table p add primary key (a); c.a ends up non-nullable, but there is no pg_constraint entry enforcing the constraint: \d+ c Table "public.c" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description --------+---------+-----------+----------+---------+---------+-------------+--------------+------------- a | integer | | not null | | plain | | | Inherits: p Access method: heap Given a database containing these 2 tables: create table p(a int primary key); create table c() inherits (p); doing a pg_dump and restore fails to restore the NOT NULL constraint on c, because all constraints created by the dump are local to p. That's it for now. I'll try to do more testing later. Regards, Dean
Thanks for spending so much time with this patch -- really appreciated. On 2023-Jul-26, Dean Rasheed wrote: > On Tue, 25 Jul 2023 at 13:36, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > Okay then, I've made these show up in the footer of \d+. This is in > > patch 0003 here. Please let me know what do you think of the regression > > changes. > > The new \d+ output certainly makes testing and reviewing easier, > though I do understand people's concerns that this may make the output > significantly longer in many real-world cases. I don't think it would > be a good idea to filter the list in any way though, because I think > that will only lead to confusion. I think it should be all-or-nothing, > though I'm not necessarily opposed to using something like \d++ to > enable it, if that turns out to be the least-bad option. Yeah, at this point I'm inclined to get the \d+ version committed immediately after the main patch, and we can tweak the psql UI after the fact -- for instance so that they are only shown in \d++, or some other idea we may come across. > Going back to this example: > > drop table if exists p1, p2, foo; > create table p1(a int not null check (a > 0)); > create table p2(a int not null check (a > 0)); > create table foo () inherits (p1,p2); > I remain of the opinion that that should create 2 NOT NULL constraints > on foo, for consistency with CHECK constraints, and the misleading > name that results if p1_a_not_null is dropped from p1. That way, the > names of inherited NOT NULL constraints could be kept in sync, as they > are for other constraint types, making it easier to keep track of > where they come from, and it wouldn't be necessary to treat them > differently (e.g., matching by column number, when dropping NOT NULL > constraints). I think having two constraints is more problematic, UI-wise. Previous versions of this patchset did it that way, and it's not great: for example ALTER TABLE ALTER COLUMN DROP NOT NULL fails and tells you to choose which exact constraint you want to drop and use DROP CONSTRAINT instead. And when searching for the not-null constraints for a column, the code had to consider the case of there being multiple ones, which led to strange contortions. Allowing a single one is simpler and covers all important cases well. Anyway, you still can't drop the doubly-inherited constraint directly, because it'll complain that it is an inherited constraint. So you have to deinherit first and only then can you drop the constraint. Now, one possible improvement here would be to ignore the parent constraint's name, and have 'foo' recompute its own constraint name from scratch, inheriting the name only if one of the parents had a manually-specified constraint name (and we would choose the first one, if there's more than one). I think complicating things more than that is unnecessary -- particularly considering that legacy inheritance is, well, legacy, and I doubt people are relying too much on it. > Given the following sequence: > > drop table if exists p,c; > create table p(a int primary key); > create table c() inherits (p); > alter table p drop constraint p_pkey; > > p.a ends up being nullable, where previously it would have been left > non-nullable. That change makes sense, and is presumably one of the > benefits of tying the nullability of columns to pg_constraint entries. Right. > However, c.a remains non-nullable, with a NOT NULL constraint that > claims to be inherited: > > \d+ c > Table "public.c" > Column | Type | Collation | Nullable | Default | Storage | > Compression | Stats target | Description > --------+---------+-----------+----------+---------+---------+-------------+--------------+------------- > a | integer | | not null | | plain | > | | > Not null constraints: > "c_a_not_null" NOT NULL "a" (inherited) > Inherits: p > Access method: heap > > That's a problem, because now the NOT NULL constraint on c cannot be > dropped (attempting to drop it on c errors out because it thinks it's > inherited, but it can't be dropped via p, because p.a is already > nullable). Oh, I think the bug here is just that this constraint should not claim to be inherited, but standalone. So you can drop it afterwards; but if you drop it and end up with NULL values in your PK-labelled column in the parent table, that's on you. > I wonder if NOT NULL constraints created as a result of inherited PKs > should have names based on the PK name (e.g., > <PK_name>_<col_name>_not_null), to make it more obvious where they > came from. That would be more consistent with the way NOT NULL > constraint names are inherited. Hmm, interesting idea. I'll play with it. (It may quickly lead to constraint names that are too long, though.) > Given the following sequence: > > drop table if exists p,c; > create table p(a int); > create table c() inherits (p); > alter table p add primary key (a); > > c.a ends up non-nullable, but there is no pg_constraint entry > enforcing the constraint: > > \d+ c > Table "public.c" > Column | Type | Collation | Nullable | Default | Storage | > Compression | Stats target | Description > --------+---------+-----------+----------+---------+---------+-------------+--------------+------------- > a | integer | | not null | | plain | > | | > Inherits: p > Access method: heap Oh, this one's a bad omission. I'll fix it. > Given a database containing these 2 tables: > > create table p(a int primary key); > create table c() inherits (p); > > doing a pg_dump and restore fails to restore the NOT NULL constraint > on c, because all constraints created by the dump are local to p. Strange. I'll see about fixing this one too. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La primera ley de las demostraciones en vivo es: no trate de usar el sistema. Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)
On 2023-Jul-26, Alvaro Herrera wrote: > On 2023-Jul-26, Dean Rasheed wrote: > > > The new \d+ output certainly makes testing and reviewing easier, > > though I do understand people's concerns that this may make the output > > significantly longer in many real-world cases. > > Yeah, at this point I'm inclined to get the \d+ version committed > immediately after the main patch, and we can tweak the psql UI after the > fact -- for instance so that they are only shown in \d++, or some other > idea we may come across. (For example, maybe we could add \dtc [PATTERN] or some such, that lists all the constraints of all kinds in tables matching PATTERN.) -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "If you want to have good ideas, you must have many ideas. Most of them will be wrong, and what you have to learn is which ones to throw away." (Linus Pauling)
> > Given the following sequence: > > > > drop table if exists p,c; > > create table p(a int primary key); > > create table c() inherits (p); > > alter table p drop constraint p_pkey; > > However, c.a remains non-nullable, with a NOT NULL constraint that > > claims to be inherited: > > > > \d+ c > > Table "public.c" > > Column | Type | Collation | Nullable | Default | Storage | > > Compression | Stats target | Description > > --------+---------+-----------+----------+---------+---------+-------------+--------------+------------- > > a | integer | | not null | | plain | > > | | > > Not null constraints: > > "c_a_not_null" NOT NULL "a" (inherited) > > Inherits: p > > Access method: heap > > > > That's a problem, because now the NOT NULL constraint on c cannot be > > dropped (attempting to drop it on c errors out because it thinks it's > > inherited, but it can't be dropped via p, because p.a is already > > nullable). So I implemented a fix for this (namely: fix the inhcount to be 0 initially), and it works well, but it does cause a definitional problem: any time we create a child table that inherits from another table that has a primary key, all the columns in the child table will get normal, visible, droppable NOT NULL constraints. Thus, pg_dump for example will output that constraint exactly as if the user had specified it in the child's CREATE TABLE command. By itself this doesn't bother me, though I admit it seems a little odd. When you restore such a setup from pg_dump, things work perfectly -- I mean, you don't get a second constraint. But if you do drop the constraint, then it will be reinstated by the next pg_dump as if you hadn't dropped it, by way of it springing to life from the PK. To avoid that, one option would be to make this NN constraint undroppable ... but I don't see how. One option might be to add a pg_depend row that links the NOT NULL constraint to its PK constraint. But this will be a strange case that occurs nowhere else, since other NOT NULL constraint don't have such pg_depend rows. Also, I won't know how pg_dump likes this until I implement it. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On 24.07.23 12:32, Alvaro Herrera wrote: > However, 11.16 (<drop column not null clause> as part of 11.12 <alter > column definition>), says that DROP NOT NULL causes the indication of > the column as NOT NULL to be removed. This, to me, says that if you do > have multiple such constraints, you'd better remove them all with that > command. All in all, I lean towards allowing just one as best as we > can. Another clue is in 11.15 <set column not null clause>, which says 1) Let C be the column identified by the <column name> CN in the containing <alter column definition>. If the column descriptor of C does not contain an indication that C is defined as NOT NULL, then: [do things] Otherwise it does nothing. So there can only be one such constraint per table.
On 2023-Jul-28, Alvaro Herrera wrote: > To avoid that, one option would be to make this NN constraint > undroppable ... but I don't see how. One option might be to add a > pg_depend row that links the NOT NULL constraint to its PK constraint. > But this will be a strange case that occurs nowhere else, since other > NOT NULL constraint don't have such pg_depend rows. Also, I won't know > how pg_dump likes this until I implement it. I've been completing the implementation for this. It seems to work reasonably okay; pg_dump requires somewhat strange contortions, but they are similar to what we do in flagInhTables already, so I don't feel too bad about that. What *is* odd and bothersome is that it also causes a problem dropping the child table. For example, CREATE TABLE parent (a int primary key); CREATE TABLE child () INHERITS (parent); \d+ child Tabla «public.child» Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión │ Almacenamiento │ Compresión │ Estadísticas │ Descripción ─────────┼─────────┼──────────────┼──────────┼─────────────┼────────────────┼────────────┼──────────────┼───────────── a │ integer │ │ not null │ │ plain │ │ │ Not null constraints: "child_a_not_null" NOT NULL "a" Hereda: parent Método de acceso: heap This is the behavior that I think we wanted to prevent drop of the child constraint, and it seems okay to me: =# alter table child drop constraint child_a_not_null; ERROR: cannot drop constraint child_a_not_null on table child because constraint parent_pkey on table parent requires it SUGERENCIA: You can drop constraint parent_pkey on table parent instead. But the problem is this: =# drop table child; ERROR: cannot drop table child because other objects depend on it DETALLE: constraint parent_pkey on table parent depends on table child SUGERENCIA: Use DROP ... CASCADE to drop the dependent objects too. To be clear, what my patch is doing is add one new dependency: dep │ ref │ deptype ────────────────────────────────────────────┼────────────────────────────────────────┼───────── type foo │ table foo │ i table foo │ schema public │ n constraint foo_pkey on table foo │ column a of table foo │ a type bar │ table bar │ i table bar │ schema public │ n table bar │ table foo │ n constraint bar_a_not_null on table bar │ column a of table bar │ a constraint child_a_not_null on table child │ column a of table child │ a constraint child_a_not_null on table child │ constraint parent_pkey on table parent │ i the last row here is what is new. I'm not sure what's the right fix. Maybe I need to invert the direction of that dependency. Even with that fixed, I'd still need to write more code so that ALTER TABLE INHERIT adds the link (I already patched the DROP INHERIT part). Not sure what else might I be missing. Separately, I also noticed that some code that's currently dropconstraint_internal needs to be moved to DropConstraintById, because if the PK is dropped for some other reason than ALTER TABLE DROP CONSTRAINT, some ancillary actions are not taken. Sigh. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Are you not unsure you want to delete Firefox? [Not unsure] [Not not unsure] [Cancel] http://smylers.hates-software.com/2008/01/03/566e45b2.html
On Fri, 4 Aug 2023 at 19:10, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2023-Jul-28, Alvaro Herrera wrote: > > > To avoid that, one option would be to make this NN constraint > > undroppable ... but I don't see how. One option might be to add a > > pg_depend row that links the NOT NULL constraint to its PK constraint. > > But this will be a strange case that occurs nowhere else, since other > > NOT NULL constraint don't have such pg_depend rows. Also, I won't know > > how pg_dump likes this until I implement it. > > I've been completing the implementation for this. It seems to work > reasonably okay; pg_dump requires somewhat strange contortions, but they > are similar to what we do in flagInhTables already, so I don't feel too > bad about that. > > What *is* odd and bothersome is that it also causes a problem dropping > the child table. > Hmm, thinking about this some more, I think this might be the wrong approach to fixing the original problem. I think it was probably OK that the NOT NULL constraint on the child was marked as inherited, but I think what should have happened is that dropping the PRIMARY KEY constraint on the parent should have caused the NOT NULL constraint on the child to have been deleted (in the same way as it would have been, if it had been a NOT NULL constraint on the parent). Regards, Dean
On 2023-Aug-05, Dean Rasheed wrote: > Hmm, thinking about this some more, I think this might be the wrong > approach to fixing the original problem. I think it was probably OK > that the NOT NULL constraint on the child was marked as inherited, but > I think what should have happened is that dropping the PRIMARY KEY > constraint on the parent should have caused the NOT NULL constraint on > the child to have been deleted (in the same way as it would have been, > if it had been a NOT NULL constraint on the parent). Yeah, something like that. However, if the child had a NOT NULL constraint of its own, then it should not be deleted when the PK-on-parent is, but merely marked as no longer inherited. (This is also what happens with a straight NOT NULL constraint.) I think what this means is that at some point during the deletion of the PK we must remove the dependency link rather than letting it be followed. I'm not yet sure how to do this. Anyway, I was at the same time fixing the other problem you reported with inheritance (namely, adding a PK ends up with the child column being marked NOT NULL but no corresponding constraint). At some point I wondered if the easy way out wouldn't be to give up on the idea that creating a PK causes the child columns to be marked not-nullable. However, IIRC I decided against that because it breaks restoring of old dumps, so it wouldn't be acceptable. To make matters worse: pg_dump creates the PK as ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... ) note the ONLY there. It seems I'm forced to cause the PK to affect children even though ONLY is given. This is undesirable but I don't see a way out of that. It is all a bit of a rat's nest. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)
On Sat, 5 Aug 2023 at 18:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Yeah, something like that. However, if the child had a NOT NULL > constraint of its own, then it should not be deleted when the > PK-on-parent is, but merely marked as no longer inherited. (This is > also what happens with a straight NOT NULL constraint.) I think what > this means is that at some point during the deletion of the PK we must > remove the dependency link rather than letting it be followed. I'm not > yet sure how to do this. > I'm not sure that adding that new dependency was the right thing to do. I think perhaps this could just be made to work using conislocal and coninhcount to track whether the child constraint needs to be deleted, or just updated. > Anyway, I was at the same time fixing the other problem you reported > with inheritance (namely, adding a PK ends up with the child column > being marked NOT NULL but no corresponding constraint). > > At some point I wondered if the easy way out wouldn't be to give up on > the idea that creating a PK causes the child columns to be marked > not-nullable. However, IIRC I decided against that because it breaks > restoring of old dumps, so it wouldn't be acceptable. > > To make matters worse: pg_dump creates the PK as > > ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... ) > > note the ONLY there. It seems I'm forced to cause the PK to affect > children even though ONLY is given. This is undesirable but I don't see > a way out of that. > > It is all a bit of a rat's nest. > I wonder if that could be made to work in the same way as inherited CHECK constraints -- dump the child's inherited NOT NULL constraints, and then manually update conislocal in pg_constraint. Regards, Dean
On 05.08.23 21:50, Dean Rasheed wrote: >> Anyway, I was at the same time fixing the other problem you reported >> with inheritance (namely, adding a PK ends up with the child column >> being marked NOT NULL but no corresponding constraint). >> >> At some point I wondered if the easy way out wouldn't be to give up on >> the idea that creating a PK causes the child columns to be marked >> not-nullable. However, IIRC I decided against that because it breaks >> restoring of old dumps, so it wouldn't be acceptable. >> >> To make matters worse: pg_dump creates the PK as >> >> ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... ) >> >> note the ONLY there. It seems I'm forced to cause the PK to affect >> children even though ONLY is given. This is undesirable but I don't see >> a way out of that. >> >> It is all a bit of a rat's nest. >> > > I wonder if that could be made to work in the same way as inherited > CHECK constraints -- dump the child's inherited NOT NULL constraints, > and then manually update conislocal in pg_constraint. I wonder whether the root of these problems is that we mix together primary key constraints and not-null constraints. I understand that right now, with the proposed patch, when a table inherits from a parent table with a primary key constraint, we generate not-null constraints on the child, in order to enforce the not-nullness. What if we did something like this instead: In the child table, we don't generate a not-null constraint, but instead a primary key constraint entry. But we mark the primary key constraint somehow to say, this is just for the purpose of inheritance, don't enforce uniqueness, but enforce not-nullness. Would that work?
On 2023-Aug-09, Peter Eisentraut wrote: > I wonder whether the root of these problems is that we mix together primary > key constraints and not-null constraints. I understand that right now, with > the proposed patch, when a table inherits from a parent table with a primary > key constraint, we generate not-null constraints on the child, in order to > enforce the not-nullness. What if we did something like this instead: In > the child table, we don't generate a not-null constraint, but instead a > primary key constraint entry. But we mark the primary key constraint > somehow to say, this is just for the purpose of inheritance, don't enforce > uniqueness, but enforce not-nullness. Would that work? Hmm. One table can have many parents, and many of them can have primary keys. If we tried to model it the way you suggest, the child table would need to have several primary keys. I don't think this would work. But I think I just need to stare at the dependency graph a little while longer. Maybe I just need to add some extra edges to make it work correctly. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On 2023-Aug-05, Dean Rasheed wrote: > On Sat, 5 Aug 2023 at 18:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > Yeah, something like that. However, if the child had a NOT NULL > > constraint of its own, then it should not be deleted when the > > PK-on-parent is, but merely marked as no longer inherited. (This is > > also what happens with a straight NOT NULL constraint.) I think what > > this means is that at some point during the deletion of the PK we must > > remove the dependency link rather than letting it be followed. I'm not > > yet sure how to do this. > > I'm not sure that adding that new dependency was the right thing to > do. I think perhaps this could just be made to work using conislocal > and coninhcount to track whether the child constraint needs to be > deleted, or just updated. Right, in the end I got around to that point of view. I abandoned the idea of adding these dependency links, and I'm back at relying on the coninhcount/conislocal markers. But there were a couple of bugs in the accounting for that, so I've fixed some of those, but it's not yet complete: - ALTER TABLE parent ADD PRIMARY KEY needs to create NOT NULL constraints in children. I added this, but I'm not yet sure it works correctly (for example, if a child already has a NOT NULL constraint, we need to bump its inhcount, but we don't.) - ALTER TABLE parent ADD PRIMARY KEY USING index Not sure if this is just as above or needs separate handling - ALTER TABLE DROP PRIMARY KEY needs to decrement inhcount or drop the constraint if there are no other sources for that constraint to exist. I've adjusted the drop constraint code to do this. - ALTER TABLE INHERIT needs to create a constraint on the new child, if parent has PK. Not implemented - ALTER TABLE NO INHERIT needs to delink any constraints (decrement inhcount, possibly drop the constraint). I also need to add tests for those scenarios, because I think there aren't any for most of them. There's also another a pg_upgrade problem: we now get spurious ALTER TABLE SET NOT NULL commands in a dump after pg_upgrade for the columns that get the constraint from a primary key. (This causes a pg_upgrade test failure). I need to adjust pg_dump to suppress those; I think something like flagInhTables would do. (I had mentioned that I needed to move code from dropconstraint_internal to RemoveConstraintById. However, now I can't figure out exactly what case was having a problem, so I've left it alone.) Here's v17, which is a step forward, but several holes remain. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "I can't go to a restaurant and order food because I keep looking at the fonts on the menu. Five minutes later I realize that it's also talking about food" (Donald Knuth)
Вложения
On Fri, 11 Aug 2023 at 14:54, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Right, in the end I got around to that point of view. I abandoned the > idea of adding these dependency links, and I'm back at relying on the > coninhcount/conislocal markers. But there were a couple of bugs in the > accounting for that, so I've fixed some of those, but it's not yet > complete: > > - ALTER TABLE parent ADD PRIMARY KEY > needs to create NOT NULL constraints in children. I added this, but > I'm not yet sure it works correctly (for example, if a child already > has a NOT NULL constraint, we need to bump its inhcount, but we > don't.) > - ALTER TABLE parent ADD PRIMARY KEY USING index > Not sure if this is just as above or needs separate handling > - ALTER TABLE DROP PRIMARY KEY > needs to decrement inhcount or drop the constraint if there are no > other sources for that constraint to exist. I've adjusted the drop > constraint code to do this. > - ALTER TABLE INHERIT > needs to create a constraint on the new child, if parent has PK. Not > implemented > - ALTER TABLE NO INHERIT > needs to delink any constraints (decrement inhcount, possibly drop > the constraint). > I think perhaps for ALTER TABLE INHERIT, it should check that the child has a NOT NULL constraint, and error out if not. That's the current behaviour, and also matches other constraints types (e.g., CHECK constraints). More generally though, I'm worried that this is starting to get very complicated. I wonder if there might be a different, simpler approach. One vague idea is to have a new attribute on the column that counts the number of constraints (local and inherited PK and NOT NULL constraints) that make the column not null. Something else I noticed when reading the SQL standard is that a user-defined CHECK (col IS NOT NULL) constraint should be recognised by the system as also making the column not null (setting its "nullability characteristic" to "known not nullable"). I think that's more than just an artefact of how they say NOT NULL constraints should be implemented, because the effect of such a CHECK constraint should be exposed in the "columns" view of the information schema -- the value of "is_nullable" should be "NO" if the column is "known not nullable". In this sense, the standard does allow multiple not null constraints on a column, independently of whether the column is "defined as NOT NULL". My understanding of the standard is that ALTER COLUMN ... SET/DROP NOT NULL change whether or not the column is "defined as NOT NULL", and manage a single system-generated constraint, but there may be any number of other user-defined constraints that also make the column "known not nullable", and they need to be tracked in some way. I'm also wondering whether creating a pg_constraint entry for *every* not-nullable column is actually going too far. If we were to distinguish between "defined as NOT NULL" and being not null as a result of one or more constraints, in the way that the standard seems to suggest, perhaps the former (likely to be much more common) could simply be a new attribute stored on the column. I think we actually only need to create pg_constraint entries if a constraint name or any additional constraint properties such as NOT VALID are specified. That would lead to far fewer new constraints, less catalog bloat, and less noise in the \d output. Regards, Dean
On 2023-Aug-15, Dean Rasheed wrote: > I think perhaps for ALTER TABLE INHERIT, it should check that the > child has a NOT NULL constraint, and error out if not. That's the > current behaviour, and also matches other constraints types (e.g., > CHECK constraints). Yeah, I reached the same conclusion yesterday while trying it out, so that's what I implemented. I'll post later today. > More generally though, I'm worried that this is starting to get very > complicated. I wonder if there might be a different, simpler approach. > One vague idea is to have a new attribute on the column that counts > the number of constraints (local and inherited PK and NOT NULL > constraints) that make the column not null. Hmm. I grant that this is different, but I don't see that it is simpler. > Something else I noticed when reading the SQL standard is that a > user-defined CHECK (col IS NOT NULL) constraint should be recognised > by the system as also making the column not null (setting its > "nullability characteristic" to "known not nullable"). I agree with this view actually, but I've refrained from implementing it(*) because our SQL-standards people have advised against it. Insider knowledge? I don't know. I think this is a comparatively smaller consideration though, and we can adjust for it afterwards. (*) Rather: at some point I removed the implementation of that from the patch. > I'm also wondering whether creating a pg_constraint entry for *every* > not-nullable column is actually going too far. If we were to > distinguish between "defined as NOT NULL" and being not null as a > result of one or more constraints, in the way that the standard seems > to suggest, perhaps the former (likely to be much more common) could > simply be a new attribute stored on the column. I think we actually > only need to create pg_constraint entries if a constraint name or any > additional constraint properties such as NOT VALID are specified. That > would lead to far fewer new constraints, less catalog bloat, and less > noise in the \d output. There is a problem if we do this, though, which is that we cannot use the constraints for the things that we want them for -- for example, remove_useless_groupby_columns() would like to use unique constraints, not just primary keys; but it depends on the NOT NULL rows being there for invalidation reasons (namely: if the NOT NULL constraint is dropped, we need to be able to replan. Without catalog rows, we don't have a mechanism to let that happen). If we don't add all those redundant catalog rows, then this is all for naught. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'. After collecting 500 such letters, he mused, a university somewhere in Arizona would probably grant him a degree. (Don Knuth)
On 15.08.23 11:57, Dean Rasheed wrote: > Something else I noticed when reading the SQL standard is that a > user-defined CHECK (col IS NOT NULL) constraint should be recognised > by the system as also making the column not null (setting its > "nullability characteristic" to "known not nullable"). I think that's > more than just an artefact of how they say NOT NULL constraints should > be implemented, because the effect of such a CHECK constraint should > be exposed in the "columns" view of the information schema -- the > value of "is_nullable" should be "NO" if the column is "known not > nullable". Nullability determination is different from not-null constraints. The nullability characteristic of a column can be derived from multiple sources, including not-null constraints, check constraints, primary key constraints, domain constraints, as well as more complex rules in case of views, joins, etc. But this is all distinct and separate from the issue of not-null constraints that we are discussing here.
I have two small patches that you can integrate into your patch set: The first just changes the punctuation of "Not-null constraints" in the psql output to match what the documentation mostly uses. The second has some changes to ddl.sgml to reflect that not-null constraints are now named and can be operated on like other constraints. You might want to read that again to make sure it matches your latest intentions, but I think it catches all the places that are required to change.
Вложения
Okay, so here's another version of this, where I fixed the creation of NOT NULLs derived from PKs. It turned out that what I was doing wasn't doing recursion correctly, so for example if you have NOT NULLs in grand-child tables they would not be marked as inherited from the PK (thus wrongly droppable). I had to rewrite it to go through ATPrepCmd and friends; and we had no way to indicate inheritance that way, so I had to add an "int inhcount" to the Constraint node. (I think it might be OK to make it just a "bool inherited" instead). There is one good thing about this, which is that currently AddRelationNewConstraints() has a strange "bool is_local" parameter (added by commit cd902b331d, 2008), which is somewhat strange, and which we could remove to instead use this new Constraint->inhcount mechanism to pass down the flag. Also: it turns out that you can do this CREATE TABLE parent (a int); CREATE TABLE child (NOT NULL a) INHERITS (parent); that is, the column has no local definition on the child, but the constraint does. This required some special fixes but also works correctly now AFAICT. On 2023-Aug-16, Peter Eisentraut wrote: > I have two small patches that you can integrate into your patch set: > > The first just changes the punctuation of "Not-null constraints" in the psql > output to match what the documentation mostly uses. > > The second has some changes to ddl.sgml to reflect that not-null constraints > are now named and can be operated on like other constraints. You might want > to read that again to make sure it matches your latest intentions, but I > think it catches all the places that are required to change. I've incorporated both of those, verbatim for now; I'll give the docs another look tomorrow. On 2023-Aug-11, Alvaro Herrera wrote: > - ALTER TABLE parent ADD PRIMARY KEY > needs to create NOT NULL constraints in children. I added this, but > I'm not yet sure it works correctly (for example, if a child already > has a NOT NULL constraint, we need to bump its inhcount, but we > don't.) > - ALTER TABLE parent ADD PRIMARY KEY USING index > Not sure if this is just as above or needs separate handling > - ALTER TABLE DROP PRIMARY KEY > needs to decrement inhcount or drop the constraint if there are no > other sources for that constraint to exist. I've adjusted the drop > constraint code to do this. > - ALTER TABLE INHERIT > needs to create a constraint on the new child, if parent has PK. Not > implemented > - ALTER TABLE NO INHERIT > needs to delink any constraints (decrement inhcount, possibly drop > the constraint). > > I also need to add tests for those scenarios, because I think there > aren't any for most of them. I've added tests for the ones I caught missing, including leaving some tables to exercise the pg_upgrade side of things. > There's also another a pg_upgrade problem: we now get spurious ALTER > TABLE SET NOT NULL commands in a dump after pg_upgrade for the columns > that get the constraint from a primary key. I fixed this too. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Вложения
I went over the whole patch and made a very large number of additional cleanups[1], to the point where I think this is truly ready for commit now. There are some relatively minor things that could still be subject of debate, such as what to name constraints that derive from PKs or from multiple inheritance parents. I have one commented out Assert() because of that. But other than those and a couple of not-terribly-important XXX comments, this is as ready as it'll ever be. I'll put it through CI soon. It's been a while since I tested using pg_upgrade from older versions, so I'll do that too. If no problems emerge, I intend to get this committed soon. [1] https://github.com/alvherre/postgres/tree/catalog-notnull-9 -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "If you want to have good ideas, you must have many ideas. Most of them will be wrong, and what you have to learn is which ones to throw away." (Linus Pauling)
Вложения
I have now pushed this again. Hopefully it'll stick this time. We may want to make some further tweaks to the behavior in some cases -- for example, don't disallow ALTER TABLE DROP NOT NULL when the constraint is both inherited and has a local definition; the other option is to mark the constraint as no longer having a local definition. I left it the other way because that's what CHECK does; maybe we would like to change both at once. I ran it through CI, and the pg_upgrade test with a dump from 14's regression test database and everything worked well, but it's been a while since I tested the sepgsql part of it, so that might the first thing to explode. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On 2023-Aug-25, Alvaro Herrera wrote: > I have now pushed this again. Hopefully it'll stick this time. Hmm, failed under the Czech locale[1]; apparently "inh_grandchld" sorts earlier than "inh_child1" there. I think I'll rename inh_grandchld to inh_child3 or something like that. [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hippopotamus&dt=2023-08-25%2011%3A33%3A07 -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On 25.08.23 13:38, Alvaro Herrera wrote: > I have now pushed this again. Hopefully it'll stick this time. > > We may want to make some further tweaks to the behavior in some cases -- > for example, don't disallow ALTER TABLE DROP NOT NULL when the > constraint is both inherited and has a local definition; the other > option is to mark the constraint as no longer having a local definition. > I left it the other way because that's what CHECK does; maybe we would > like to change both at once. > > I ran it through CI, and the pg_upgrade test with a dump from 14's > regression test database and everything worked well, but it's been a > while since I tested the sepgsql part of it, so that might the first > thing to explode. It looks like we forgot about domain constraints? For example, create domain testdomain as int not null; should create a row in pg_constraint?
On 2023-Aug-28, Peter Eisentraut wrote: > It looks like we forgot about domain constraints? For example, > > create domain testdomain as int not null; > > should create a row in pg_constraint? Well, at some point I purposefully left them out; they were sufficiently different from the ones in tables that doing both things at the same time was not saving any effort. I guess we could try to bake them in now. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Doing what he did amounts to sticking his fingers under the hood of the implementation; if he gets his fingers burnt, it's his problem." (Tom Lane)
Hi Alvaro,
25.08.2023 14:38, Alvaro Herrera wrote:
25.08.2023 14:38, Alvaro Herrera wrote:
I have now pushed this again. Hopefully it'll stick this time.
I've found that after that commit the following query:
CREATE TABLE t(a int PRIMARY KEY) PARTITION BY RANGE (a);
CREATE TABLE tp1(a int);
ALTER TABLE t ATTACH PARTITION tp1 FOR VALUES FROM (0) to (1);
triggers a server crash:
Core was generated by `postgres: law regression [local] ALTER TABLE '.
Program terminated with signal SIGSEGV, Segmentation fault.
warning: Section `.reg-xstate/2194811' in core file too small.
#0 0x0000556007711d77 in MergeAttributesIntoExisting (child_rel=0x7fc30ba309d8,
parent_rel=0x7fc30ba33f18) at tablecmds.c:15771
15771 if (!((Form_pg_constraint) GETSTRUCT(contup))->connoinherit)
(gdb) bt
#0 0x0000556007711d77 in MergeAttributesIntoExisting (child_rel=0x7fc30ba309d8,
parent_rel=0x7fc30ba33f18) at tablecmds.c:15771
#1 0x00005560077118d4 in CreateInheritance (child_rel=0x7fc30ba309d8, parent_rel=0x7fc30ba33f18)
at tablecmds.c:15631
...
(gdb) print contup
$1 = (HeapTuple) 0x0
On b0e96f311~1 I get:
ERROR: column "a" in child table must be marked NOT NULL
Best regards,
Alexander
On 2023-Mar-29, Peter Eisentraut wrote: > On 27.03.23 15:55, Peter Eisentraut wrote: > > The information schema should be updated. I think the following views: > > > > - CHECK_CONSTRAINTS > > - CONSTRAINT_COLUMN_USAGE > > - DOMAIN_CONSTRAINTS > > - TABLE_CONSTRAINTS > > > > It looks like these have no test coverage; maybe that could be addressed > > at the same time. > > Here are patches for this. I haven't included the expected files for the > tests; this should be checked again that output is correct or the changes > introduced by this patch set are as expected. > > The reason we didn't have tests for this before was probably in part because > the information schema made up names for not-null constraints involving > OIDs, so the test wouldn't have been stable. > > Feel free to integrate this, or we can add it on afterwards. I'm eyeing patch 0002 here. I noticed that in view check_constraints it defines the not-null constraint definition as substrings over the pg_get_constraintdef() function[q1], so I wondered whether it might be better to join to pg_attribute instead. I see two options: 1. add a scalar subselect in the select list for each constraint [q2] 2. add a LEFT JOIN to pg_attribute to the main FROM list [q3] ON con.conrelid=att.attrelid AND con.conkey[1] = con.attrelid With just the regression test tables in place, these forms are all pretty much the same in execution time. I then created 20k tables with 6 columns each and NOT NULL constraint on each column[4]. That's not a huge amount but it's credible for a medium-size database with a bunch of partitions (it's amazing what passes for a medium-size database these days). I was surprised to find out that q3 (~130ms) is three times faster than q2 (~390ms), which is in turn more than twice faster than your proposed q1 (~870ms). So unless you have another reason to prefer it, I think we should use q3 here. In constraint_column_usage, you're adding a relkind to the catalog scan that goes through pg_depend for CHECK constraints. Here we can rely on a simple conkey[1] check and a separate UNION ALL arm[q5]; this is also faster when there are many tables. The third view definition looks ok. It's certainly very nice to be able to delete XXX comments there. [q1] SELECT current_database()::information_schema.sql_identifier AS constraint_catalog, rs.nspname::information_schema.sql_identifier AS constraint_schema, con.conname::information_schema.sql_identifier AS constraint_name, CASE con.contype WHEN 'c'::"char" THEN "left"(SUBSTRING(pg_get_constraintdef(con.oid) FROM 8), '-1'::integer) WHEN 'n'::"char" THEN SUBSTRING(pg_get_constraintdef(con.oid) FROM 10) || ' IS NOT NULL'::text ELSE NULL::text END::information_schema.character_data AS check_clause FROM pg_constraint con LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace LEFT JOIN pg_class c ON c.oid = con.conrelid LEFT JOIN pg_type t ON t.oid = con.contypid WHERE pg_has_role(COALESCE(c.relowner, t.typowner), 'USAGE'::text) AND (con.contype = ANY (ARRAY['c'::"char", 'n'::"char"])); [q2] SELECT current_database()::information_schema.sql_identifier AS constraint_catalog, rs.nspname::information_schema.sql_identifier AS constraint_schema, con.conname::information_schema.sql_identifier AS constraint_name, CASE con.contype WHEN 'c'::"char" THEN "left"(SUBSTRING(pg_get_constraintdef(con.oid) FROM 8), '-1'::integer) WHEN 'n'::"char" THEN FORMAT('CHECK (%s IS NOT NULL)', (SELECT attname FROM pg_attribute WHERE attrelid = conrelid AND attnum = conkey[1])) ELSE NULL::text END::information_schema.character_data AS check_clause FROM pg_constraint con LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace LEFT JOIN pg_class c ON c.oid = con.conrelid LEFT JOIN pg_type t ON t.oid = con.contypid WHERE pg_has_role(COALESCE(c.relowner, t.typowner), 'USAGE'::text) AND (con.contype = ANY (ARRAY['c'::"char", 'n'::"char"])); [q3] SELECT current_database()::information_schema.sql_identifier AS constraint_catalog, rs.nspname::information_schema.sql_identifier AS constraint_schema, con.conname::information_schema.sql_identifier AS constraint_name, CASE con.contype WHEN 'c'::"char" THEN "left"(SUBSTRING(pg_get_constraintdef(con.oid) FROM 8), '-1'::integer) WHEN 'n'::"char" THEN FORMAT('CHECK (%s IS NOT NULL)', at.attname) ELSE NULL::text END::information_schema.character_data AS check_clause FROM pg_constraint con LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace LEFT JOIN pg_class c ON c.oid = con.conrelid LEFT JOIN pg_type t ON t.oid = con.contypid LEFT JOIN pg_attribute at ON (con.conrelid = at.attrelid AND con.conkey[1] = at.attnum) WHERE pg_has_role(COALESCE(c.relowner, t.typowner), 'USAGE'::text) AND (con.contype = ANY (ARRAY['c'::"char", 'n'::"char"])); [4] do $$ begin for i in 0 .. 20000 loop execute format('create table t_%s (a1 int not null, a2 int not null, a3 int not null, a4 int not null, a5 int not null, a6 int not null);', i); if i % 1000 = 0 then commit; end if; end loop; end $$; [q5] SELECT CAST(current_database() AS sql_identifier) AS table_catalog, CAST(tblschema AS sql_identifier) AS table_schema, CAST(tblname AS sql_identifier) AS table_name, CAST(colname AS sql_identifier) AS column_name, CAST(current_database() AS sql_identifier) AS constraint_catalog, CAST(cstrschema AS sql_identifier) AS constraint_schema, CAST(cstrname AS sql_identifier) AS constraint_name FROM ( /* check constraints */ SELECT DISTINCT nr.nspname, r.relname, r.relowner, a.attname, nc.nspname, c.conname FROM pg_namespace nr, pg_class r, pg_attribute a, pg_depend d, pg_namespace nc, pg_constraint c WHERE nr.oid = r.relnamespace AND r.oid = a.attrelid AND d.refclassid = 'pg_catalog.pg_class'::regclass AND d.refobjid = r.oid AND d.refobjsubid = a.attnum AND d.classid = 'pg_catalog.pg_constraint'::regclass AND d.objid = c.oid AND c.connamespace = nc.oid AND c.contype = 'c' AND r.relkind IN ('r', 'p') AND NOT a.attisdropped UNION ALL /* not-null constraints */ SELECT DISTINCT nr.nspname, r.relname, r.relowner, a.attname, nc.nspname, c.conname FROM pg_namespace nr, pg_class r, pg_attribute a, pg_namespace nc, pg_constraint c WHERE nr.oid = r.relnamespace AND r.oid = a.attrelid AND r.oid = c.conrelid AND a.attnum = c.conkey[1] AND c.connamespace = nc.oid AND c.contype = 'n' AND r.relkind in ('r', 'p') AND not a.attisdropped UNION ALL /* unique/primary key/foreign key constraints */ SELECT nr.nspname, r.relname, r.relowner, a.attname, nc.nspname, c.conname FROM pg_namespace nr, pg_class r, pg_attribute a, pg_namespace nc, pg_constraint c WHERE nr.oid = r.relnamespace AND r.oid = a.attrelid AND nc.oid = c.connamespace AND r.oid = CASE c.contype WHEN 'f' THEN c.confrelid ELSE c.conrelid END AND a.attnum = ANY (CASE c.contype WHEN 'f' THEN c.confkey ELSE c.conkey END) AND NOT a.attisdropped AND c.contype IN ('p', 'u', 'f') AND r.relkind IN ('r', 'p') ) AS x (tblschema, tblname, tblowner, colname, cstrschema, cstrname) WHERE pg_has_role(x.tblowner, 'USAGE') ; -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Hello Alexander, Thanks for testing. On 2023-Aug-31, Alexander Lakhin wrote: > 25.08.2023 14:38, Alvaro Herrera wrote: > > I have now pushed this again. Hopefully it'll stick this time. > > I've found that after that commit the following query: > CREATE TABLE t(a int PRIMARY KEY) PARTITION BY RANGE (a); > CREATE TABLE tp1(a int); > ALTER TABLE t ATTACH PARTITION tp1 FOR VALUES FROM (0) to (1); > > triggers a server crash: Hmm, that's some weird code I left there all right. Can you please try this patch? (Not final; I'll review it more completely later, particularly to add this test case.) -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ <Schwern> It does it in a really, really complicated way <crab> why does it need to be complicated? <Schwern> Because it's MakeMaker.
Вложения
31.08.2023 13:26, Alvaro Herrera wrote: > Hmm, that's some weird code I left there all right. Can you please try > this patch? (Not final; I'll review it more completely later, > particularly to add this test case.) Yes, your patch fixes the issue. I get the same error now: ERROR: column "a" in child table must be marked NOT NULL Thank you! Best regards, Alexander
On 2023-Aug-31, Alvaro Herrera wrote: > Hmm, that's some weird code I left there all right. Can you please try > this patch? (Not final; I'll review it more completely later, > particularly to add this test case.) The change in MergeAttributesIntoExisting turned out to be close but not quite there, so I pushed another version of the fix. In case you're wondering, the change in MergeConstraintsIntoExisting is a related but different case, for which I added the other test case you see there. I also noticed, while looking at this, that there's another problem when a child has a NO INHERIT not-null constraint and the parent has a primary key in the same column. It should refuse, or take over by marking it no longer NO INHERIT. But it just accepts silently and all appears to be good. The problems appear when you add a child to that child. I'll look into this later; it's not exactly the same code. At least it's not a crasher. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Looking at your 0001 patch, which adds tests for some of the information_schema views, I think it's a bad idea to put them in whatever other regression .sql files; they would be subject to concurrent changes depending on what other tests are being executed in the same parallel test. I suggest to put them all in a separate .sql file, and schedule that to run in the last concurrent group, together with the tablespace test. This way, it would capture all the objects left over by other test files. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On 31.08.23 12:02, Alvaro Herrera wrote: > In constraint_column_usage, you're adding a relkind to the catalog scan > that goes through pg_depend for CHECK constraints. Here we can rely on > a simple conkey[1] check and a separate UNION ALL arm[q5]; this is also > faster when there are many tables. > > The third view definition looks ok. It's certainly very nice to be able > to delete XXX comments there. The following information schema views are affected by the not-null constraint catalog entries: 1. CHECK_CONSTRAINTS 2. CONSTRAINT_COLUMN_USAGE 3. DOMAIN_CONSTRAINTS 4. TABLE_CONSTRAINTS Note that 1 and 3 also contain domain constraints. So as long as the domain not-null constraints are not similarly catalogued, we can't delete the separate not-null union branch. (3 never had one, so arguably a bit buggy.) I think we can fix up 4 by just deleting the not-null union branch. For 2, the simple fix is also easy, but there are some other options, as you discuss above. How do you want to proceed?
On 2023-Sep-05, Peter Eisentraut wrote: > The following information schema views are affected by the not-null > constraint catalog entries: > > 1. CHECK_CONSTRAINTS > 2. CONSTRAINT_COLUMN_USAGE > 3. DOMAIN_CONSTRAINTS > 4. TABLE_CONSTRAINTS > > Note that 1 and 3 also contain domain constraints. So as long as the domain > not-null constraints are not similarly catalogued, we can't delete the > separate not-null union branch. (3 never had one, so arguably a bit buggy.) > > I think we can fix up 4 by just deleting the not-null union branch. > > For 2, the simple fix is also easy, but there are some other options, as you > discuss above. > > How do you want to proceed? I posted as a patch in a separate thread[1]. Let me fix up the definitions for views 1 and 3 for domains per your comments, and I'll post in that thread again. [1] https://postgr.es/m/202309041710.psytrxlsiqex@alvherre.pgsql -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)
On 2023-Sep-05, Peter Eisentraut wrote: > The following information schema views are affected by the not-null > constraint catalog entries: > > 1. CHECK_CONSTRAINTS > 2. CONSTRAINT_COLUMN_USAGE > 3. DOMAIN_CONSTRAINTS > 4. TABLE_CONSTRAINTS > > Note that 1 and 3 also contain domain constraints. After looking at what happens for domain constraints in older versions (I tested 15, but I suppose this applies everywhere), I notice that we don't seem to handle them anywhere that I can see. My quick exercise is just create domain nnint as int not null; create table foo (a nnint); and then verify that this constraint shows nowhere -- it's not in DOMAIN_CONSTRAINTS for starters, which is I think the most obvious place. And nothing is shown in CHECK_CONSTRAINTS nor TABLE_CONSTRAINTS either. This did ever work in the past? I tested with 9.3 and didn't see anything there either. I am hesitant to try to add domain not-null constraint support to information_schema in the same commit as these changes. I think this should be fixed separately. (Note that if, in older versions, you change the table to be create table foo (a nnint NOT NULL); then you do get a row in table_constraints, but nothing in check_constraints. With my proposed definition this constraint appears in check_constraints, table_constraints and constraint_column_usage.) On 2023-Sep-04, Tom Lane wrote: > I object very very strongly to this proposed test method. It > completely undoes the work I did in v15 (cc50080a8 and related) > to make the core regression test scripts mostly independent of each > other. Even without considering the use-case of running a subset of > the tests, the new test's expected output will constantly be needing > updates as side effects of unrelated changes. You're absolutely right, this would be disastrous. A better alternative is that the new test file creates a few objects for itself, either by using a separate role or by using a separate schema, and we examine the information_schema display for those objects only. Then it'll be better isolated. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ Subversion to GIT: the shortest path to happiness I've ever heard of (Alexey Klyukin)
On 2023-Sep-05, Alvaro Herrera wrote: > After looking at what happens for domain constraints in older versions > (I tested 15, but I suppose this applies everywhere), I notice that we > don't seem to handle them anywhere that I can see. My quick exercise is > just > > create domain nnint as int not null; > create table foo (a nnint); > > and then verify that this constraint shows nowhere -- it's not in > DOMAIN_CONSTRAINTS for starters, which is I think the most obvious place. > And nothing is shown in CHECK_CONSTRAINTS nor TABLE_CONSTRAINTS either. Looking now at what to do for CHECK_CONSTRAINTS with domain constraints, I admit I'm completely confused about what this view is supposed to show. Currently, we show the constraint name and a definition like "CHECK (column IS NOT NULL)". But since the table name is not given, it is not possible to know to what table the column name refers to. For domains, we could show "CHECK (VALUE IS NOT NULL)" but again with no indication of what domain it applies to, or anything at all that would make this useful in any way whatsoever. So this whole thing seems pretty futile and I'm disinclined to waste much time on it. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On 9/5/23 19:15, Alvaro Herrera wrote: > On 2023-Sep-05, Alvaro Herrera wrote: > > Looking now at what to do for CHECK_CONSTRAINTS with domain constraints, > I admit I'm completely confused about what this view is supposed to > show. Currently, we show the constraint name and a definition like > "CHECK (column IS NOT NULL)". But since the table name is not given, it > is not possible to know to what table the column name refers to. For > domains, we could show "CHECK (VALUE IS NOT NULL)" but again with no > indication of what domain it applies to, or anything at all that would > make this useful in any way whatsoever. Constraint names are supposed to be unique per schema[1] so the view contains the minimum required information to identify the constraint. > So this whole thing seems pretty futile and I'm disinclined to waste > much time on it. Until PostgreSQL either A) obeys the spec on this uniqueness, or B) decides to deviate from the information_schema spec; this view will be completely useless for actually getting any useful information. I would like to see us do A because it is the right thing to do. Our autogenerated names obey this rule, but who knows how many duplicate names per schema are out there in the wild from people specifying their own names. I don't know what the project would think about doing B. [1] SQL:2023-2 11.4 <table constraint definition> Syntax Rule 4 -- Vik Fearing
On Tue, Sep 5, 2023 at 2:50 PM Vik Fearing <vik@postgresfriends.org> wrote:
On 9/5/23 19:15, Alvaro Herrera wrote:
> On 2023-Sep-05, Alvaro Herrera wrote:
>
> Looking now at what to do for CHECK_CONSTRAINTS with domain constraints,
> I admit I'm completely confused about what this view is supposed to
> show. Currently, we show the constraint name and a definition like
> "CHECK (column IS NOT NULL)". But since the table name is not given, it
> is not possible to know to what table the column name refers to. For
> domains, we could show "CHECK (VALUE IS NOT NULL)" but again with no
> indication of what domain it applies to, or anything at all that would
> make this useful in any way whatsoever.
Constraint names are supposed to be unique per schema[1] so the view
contains the minimum required information to identify the constraint.
I'm presuming that the view constraint_column_usage [1] is an integral part of all this though I haven't taken the time to figure out exactly how we are implementing it today.
I'm not all that for either A or B since the status quo seems workable. Though ideally if the system has unique names per schema then everything should just work - having the views produce duplicated information (as opposed to nothing) if they are used when the DBA doesn't enforce the standard's requirements seems plausible.
David J.
On 9/6/23 00:14, David G. Johnston wrote: > > I'm not all that for either A or B since the status quo seems workable. Pray tell, how is it workable? The view does not identify a specific constraint because we don't obey the rules on one side and we do obey the rules on the other side. It is completely useless and unworkable. > Though ideally if the system has unique names per schema then everything > should just work - having the views produce duplicated information (as > opposed to nothing) if they are used when the DBA doesn't enforce the > standard's requirements seems plausible. Let us not engage in victim blaming. Postgres is the problem here. -- Vik Fearing
Vik Fearing <vik@postgresfriends.org> writes: > On 9/6/23 00:14, David G. Johnston wrote: >> I'm not all that for either A or B since the status quo seems workable. > Pray tell, how is it workable? The view does not identify a specific > constraint because we don't obey the rules on one side and we do obey > the rules on the other side. It is completely useless and unworkable. What solution do you propose? Starting to enforce the spec's rather arbitrary requirement that constraint names be unique per-schema is a complete nonstarter. Changing the set of columns in a spec-defined view is also a nonstarter, or at least we've always taken it as such. If you'd like to see some forward progress in this area, maybe you could lobby the SQL committee to make constraint names unique per-table not per-schema, and then make the information_schema changes that would be required to support that. In general though, the fact that we have any DDL extensions at all compared to the standard means that there will be Postgres databases that are not adequately represented by the information_schema views. I'm not sure it's worth being more outraged about constraint names than anything else. Or do you also want us to rip out (for starters) unique indexes on expressions, or unique partial indexes? regards, tom lane
On 9/6/23 02:53, Tom Lane wrote: > Vik Fearing <vik@postgresfriends.org> writes: >> On 9/6/23 00:14, David G. Johnston wrote: >>> I'm not all that for either A or B since the status quo seems workable. > >> Pray tell, how is it workable? The view does not identify a specific >> constraint because we don't obey the rules on one side and we do obey >> the rules on the other side. It is completely useless and unworkable. > > What solution do you propose? Starting to enforce the spec's rather > arbitrary requirement that constraint names be unique per-schema is > a complete nonstarter. Changing the set of columns in a spec-defined > view is also a nonstarter, or at least we've always taken it as such. I both semi-agree and semi-disagree that these are nonstarters. One of them has to give. > If you'd like to see some forward progress in this area, maybe you > could lobby the SQL committee to make constraint names unique per-table > not per-schema, and then make the information_schema changes that would > be required to support that. I could easily do that; but now you are asking to denormalize the standard, because the constraints could be from tables, domains, or assertions. I don't think that will go over well, starting with my own opinion. And for this reason, I do not believe that this is a "rather arbitrary requirement". > In general though, the fact that we have any DDL extensions at all > compared to the standard means that there will be Postgres databases > that are not adequately represented by the information_schema views. Sure. > I'm not sure it's worth being more outraged about constraint names > than anything else. Or do you also want us to rip out (for starters) > unique indexes on expressions, or unique partial indexes? Indexes of any kind are not part of the standard so these examples are basically invalid. SQL:2023-11 Schemata is not the part I am most familiar with, but I don't even see where regular multi-column unique constraints are listed out, so that is both a lack in the standard and a knockdown of this argument. I am happy to be shown wrong about this. -- Vik Fearing
Vik Fearing <vik@postgresfriends.org> writes: > On 9/6/23 02:53, Tom Lane wrote: >> What solution do you propose? Starting to enforce the spec's rather >> arbitrary requirement that constraint names be unique per-schema is >> a complete nonstarter. Changing the set of columns in a spec-defined >> view is also a nonstarter, or at least we've always taken it as such. > I both semi-agree and semi-disagree that these are nonstarters. One of > them has to give. [ shrug... ] if you stick to a SQL-compliant schema setup, then the information_schema views will serve for introspection. If you don't, they won't, and you'll need to look at Postgres-specific catalog data. This compromise has served for twenty years or so, and I'm not in a hurry to change it. I think the odds of changing to the spec's restriction without enormous pushback are nil, and I do not think that the benefit could possibly be worth the ensuing pain to users. (It's not even the absolute pain level that is a problem, so much as the asymmetry: the pain would fall exclusively on users who get no benefit, because they weren't relying on these views anyway. If you think that's an easy sell, you're mistaken.) It could possibly be a little more palatable to add column(s) to the information_schema views, but I'm having a hard time seeing how that moves the needle. The situation would still be precisely describable as "if you stick to a SQL-compliant schema setup, then the standard columns of the information_schema views will serve for introspection. If you don't, they won't, and you'll need to look at Postgres-specific columns". That doesn't seem like a big improvement. Also, given your point about normalization, how would we define the additions exactly? regards, tom lane
On 05.09.23 18:24, Alvaro Herrera wrote: > On 2023-Sep-05, Peter Eisentraut wrote: > >> The following information schema views are affected by the not-null >> constraint catalog entries: >> >> 1. CHECK_CONSTRAINTS >> 2. CONSTRAINT_COLUMN_USAGE >> 3. DOMAIN_CONSTRAINTS >> 4. TABLE_CONSTRAINTS >> >> Note that 1 and 3 also contain domain constraints. > > After looking at what happens for domain constraints in older versions > (I tested 15, but I suppose this applies everywhere), I notice that we > don't seem to handle them anywhere that I can see. My quick exercise is > just > > create domain nnint as int not null; > create table foo (a nnint); > > and then verify that this constraint shows nowhere -- it's not in > DOMAIN_CONSTRAINTS for starters, which is I think the most obvious place. > And nothing is shown in CHECK_CONSTRAINTS nor TABLE_CONSTRAINTS either. > > This did ever work in the past? I tested with 9.3 and didn't see > anything there either. No, this was never implemented. (As I wrote in my other message on the other thread, arguably a bit buggy.) We could fix this separately, unless we are going to implement catalogued domain not-null constraints soon.
On 9/6/23 05:40, Tom Lane wrote: > Vik Fearing <vik@postgresfriends.org> writes: >> On 9/6/23 02:53, Tom Lane wrote: >>> What solution do you propose? Starting to enforce the spec's rather >>> arbitrary requirement that constraint names be unique per-schema is >>> a complete nonstarter. Changing the set of columns in a spec-defined >>> view is also a nonstarter, or at least we've always taken it as such. > >> I both semi-agree and semi-disagree that these are nonstarters. One of >> them has to give. > > [ shrug... ] if you stick to a SQL-compliant schema setup, then the > information_schema views will serve for introspection. If you don't, > they won't, and you'll need to look at Postgres-specific catalog data. As someone who regularly asks people to cite chapter and verse of the standard, do you not see this as a problem? If there is /one thing/ I wish we were 100% compliant on, it's information_schema. > This compromise has served for twenty years or so, and I'm not in a > hurry to change it. Has it? Or is this just the first time someone has complained? > I think the odds of changing to the spec's > restriction without enormous pushback are nil, and I do not think > that the benefit could possibly be worth the ensuing pain to users. That is a valid opinion, and probably one that will win out for quite a while. > (It's not even the absolute pain level that is a problem, so much > as the asymmetry: the pain would fall exclusively on users who get > no benefit, because they weren't relying on these views anyway. > If you think that's an easy sell, you're mistaken.) I am curious how many people we are selling this to. In my career as a consultant, I have never once come across anyone specifying their own constraint names. That is certainly anecdotal, and by no means means it doesn't happen, but my personal experience says that it is very low. And since our generated names obey the spec (see ChooseConstraintName() which even says some apps depend on this), I don't see making this change being a big problem in the real world. Mind you, I am not pushing (right now) to make this change; I am just saying that it is the right thing to do. > It could possibly be a little more palatable to add column(s) to the > information_schema views, but I'm having a hard time seeing how that > moves the needle. The situation would still be precisely describable > as "if you stick to a SQL-compliant schema setup, then the standard > columns of the information_schema views will serve for introspection. > If you don't, they won't, and you'll need to look at Postgres-specific > columns". That doesn't seem like a big improvement. Also, given your > point about normalization, how would we define the additions exactly? This is precisely my point. -- Vik Fearing
Hi Alvaro, 25.08.2023 14:38, Alvaro Herrera wrote: > I have now pushed this again. Hopefully it'll stick this time. I've discovered that that commit added several recursive functions, and some of them are not protected from stack overflow. Namely, with "max_locks_per_transaction = 600" and default ulimit -s (8192), I observe server crashes with the following scripts: # ATExecSetNotNull() (n=40000; printf "create table t0 (a int, b int);"; for ((i=1;i<=$n;i++)); do printf "create table t$i() inherits(t$(( $i - 1 ))); "; done; printf "alter table t0 alter b set not null;" ) | psql >psql.log # dropconstraint_internal() (n=20000; printf "create table t0 (a int, b int not null);"; for ((i=1;i<=$n;i++)); do printf "create table t$i() inherits(t$(( $i - 1 ))); "; done; printf "alter table t0 alter b drop not null;" ) | psql >psql.log # set_attnotnull() (n=110000; printf "create table tp (a int, b int, primary key(a, b)) partition by range (a); create table tp0 (a int primary key, b int) partition by range (a);"; for ((i=1;i<=$n;i++)); do printf "create table tp$i partition of tp$(( $i - 1 )) for values from ($i) to (1000000) partition by range (a);"; done; printf "alter table tp attach partition tp0 for values from (0) to (1000000);") | psql >psql.log # this takes half an hour on my machine May be you would find appropriate to add check_stack_depth() to these functions. (ATAddCheckNNConstraint() is protected because it calls AddRelationNewConstraints(), which in turn calls StoreRelCheck() -> CreateConstraintEntry() -> recordDependencyOnSingleRelExpr() -> find_expr_references_walker() -> expression_tree_walker() -> expression_tree_walker() -> check_stack_depth().) (There were patches prepared for similar cases [1], but they don't cover new functions, of course, and I'm not sure how to handle all such instances.) [1] https://commitfest.postgresql.org/45/4239/ Best regards, Alexander
On 2023-Oct-12, Alexander Lakhin wrote: Hello, > I've discovered that that commit added several recursive functions, and > some of them are not protected from stack overflow. True. I reproduced the first two, but didn't attempt to reproduce the third one -- patching all these to check for stack depth is cheap protection. I also patched ATAddCheckNNConstraint: > (ATAddCheckNNConstraint() is protected because it calls > AddRelationNewConstraints(), which in turn calls StoreRelCheck() -> > CreateConstraintEntry() -> recordDependencyOnSingleRelExpr() -> > find_expr_references_walker() -> expression_tree_walker() -> > expression_tree_walker() -> check_stack_depth().) because it seems uselessly risky to rely on depth checks that exist on completely unrelated pieces of code, when the function visibly recurses on itself. Especially so since the test cases that demonstrate crashes are so expensive to run, which means we're not going to detect it if at some point that other stack depth check stops being called for whatever reason. BTW probably the tests could be made much cheaper by running the server with a lower "ulimit -s" setting. I didn't try. I noticed one more crash while trying to "drop table" one of the hierarchies your scripts create. But it's a preexisting issue which needs a backpatched fix, and I think Egor already reported it in the other thread. Thank you -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Industry suffers from the managerial dogma that for the sake of stability and continuity, the company should be independent of the competence of individual employees." (E. Dijkstra)
Hi Alvaro,
25.08.2023 14:38, Alvaro Herrera wrote:
> I have now pushed this again. Hopefully it'll stick this time.
> I have now pushed this again. Hopefully it'll stick this time.
Starting from b0e96f31, pg_upgrade fails with inherited NOT NULL constraint:
For example upgrade from 9c13b6814a (or REL_12_STABLE .. REL_16_STABLE) to b0e96f31 (or master) with following two tables (excerpt from src/test/regress/sql/rules.sql)
create table test_0 (id serial primary key);
create table test_1 (id integer primary key) inherits (test_0);
create table test_1 (id integer primary key) inherits (test_0);
I get the failure:
Setting frozenxid and minmxid counters in new cluster ok
Restoring global objects in the new cluster ok
Restoring database schemas in the new cluster
test
Restoring global objects in the new cluster ok
Restoring database schemas in the new cluster
test
*failure*
Consult the last few lines of "new/pg_upgrade_output.d/20240125T151231.112/log/pg_upgrade_dump_16384.log" for
the probable cause of the failure.
Failure, exiting
Consult the last few lines of "new/pg_upgrade_output.d/20240125T151231.112/log/pg_upgrade_dump_16384.log" for
the probable cause of the failure.
Failure, exiting
In log:
pg_restore: connecting to database for restore
pg_restore: creating DATABASE "test"
pg_restore: connecting to new database "test"
pg_restore: creating DATABASE PROPERTIES "test"
pg_restore: connecting to new database "test"
pg_restore: creating pg_largeobject "pg_largeobject"
pg_restore: creating COMMENT "SCHEMA "public""
pg_restore: creating TABLE "public.test_0"
pg_restore: creating SEQUENCE "public.test_0_id_seq"
pg_restore: creating SEQUENCE OWNED BY "public.test_0_id_seq"
pg_restore: creating TABLE "public.test_1"
pg_restore: creating DEFAULT "public.test_0 id"
pg_restore: executing SEQUENCE SET test_0_id_seq
pg_restore: creating CONSTRAINT "public.test_0 test_0_pkey"
pg_restore: creating CONSTRAINT "public.test_1 test_1_pkey"
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 3200; 2606 16397 CONSTRAINT test_1 test_1_pkey andrew
pg_restore: error: could not execute query: ERROR: cannot drop inherited constraint "pgdump_throwaway_notnull_0" of relation "test_1"
Command was:
-- For binary upgrade, must preserve pg_class oids and relfilenodes
SELECT pg_catalog.binary_upgrade_set_next_index_pg_class_oid('16396'::pg_catalog.oid);
SELECT pg_catalog.binary_upgrade_set_next_index_relfilenode('16396'::pg_catalog.oid);
ALTER TABLE ONLY "public"."test_1"
ADD CONSTRAINT "test_1_pkey" PRIMARY KEY ("id");
ALTER TABLE ONLY "public"."test_1" DROP CONSTRAINT pgdump_throwaway_notnull_0;
pg_restore: creating DATABASE "test"
pg_restore: connecting to new database "test"
pg_restore: creating DATABASE PROPERTIES "test"
pg_restore: connecting to new database "test"
pg_restore: creating pg_largeobject "pg_largeobject"
pg_restore: creating COMMENT "SCHEMA "public""
pg_restore: creating TABLE "public.test_0"
pg_restore: creating SEQUENCE "public.test_0_id_seq"
pg_restore: creating SEQUENCE OWNED BY "public.test_0_id_seq"
pg_restore: creating TABLE "public.test_1"
pg_restore: creating DEFAULT "public.test_0 id"
pg_restore: executing SEQUENCE SET test_0_id_seq
pg_restore: creating CONSTRAINT "public.test_0 test_0_pkey"
pg_restore: creating CONSTRAINT "public.test_1 test_1_pkey"
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 3200; 2606 16397 CONSTRAINT test_1 test_1_pkey andrew
pg_restore: error: could not execute query: ERROR: cannot drop inherited constraint "pgdump_throwaway_notnull_0" of relation "test_1"
Command was:
-- For binary upgrade, must preserve pg_class oids and relfilenodes
SELECT pg_catalog.binary_upgrade_set_next_index_pg_class_oid('16396'::pg_catalog.oid);
SELECT pg_catalog.binary_upgrade_set_next_index_relfilenode('16396'::pg_catalog.oid);
ALTER TABLE ONLY "public"."test_1"
ADD CONSTRAINT "test_1_pkey" PRIMARY KEY ("id");
ALTER TABLE ONLY "public"."test_1" DROP CONSTRAINT pgdump_throwaway_notnull_0;
Thanks!
On Thu, Jan 25, 2024 at 3:06 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
I have now pushed this again. Hopefully it'll stick this time.
We may want to make some further tweaks to the behavior in some cases --
for example, don't disallow ALTER TABLE DROP NOT NULL when the
constraint is both inherited and has a local definition; the other
option is to mark the constraint as no longer having a local definition.
I left it the other way because that's what CHECK does; maybe we would
like to change both at once.
I ran it through CI, and the pg_upgrade test with a dump from 14's
regression test database and everything worked well, but it's been a
while since I tested the sepgsql part of it, so that might the first
thing to explode.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Hello Alvaro, Please look at an anomaly introduced with b0e96f311. The following script: CREATE TABLE a (); CREATE TABLE b (i int) INHERITS (a); CREATE TABLE c () INHERITS (a, b); ALTER TABLE a ADD COLUMN i int NOT NULL; results in: NOTICE: merging definition of column "i" for child "b" NOTICE: merging definition of column "i" for child "c" ERROR: tuple already updated by self (This is similar to bug #18297, but ATExecAddColumn() isn't guilty in this case.) Best regards, Alexander
On Fri, Feb 02, 2024 at 07:00:01PM +0300, Alexander Lakhin wrote: > results in: > NOTICE: merging definition of column "i" for child "b" > NOTICE: merging definition of column "i" for child "c" > ERROR: tuple already updated by self > > (This is similar to bug #18297, but ATExecAddColumn() isn't guilty in this > case.) Still I suspect that the fix should be similar, soI'll go put a coin on a missing CCI(). -- Michael
Вложения
On 2024-Feb-05, Michael Paquier wrote: > On Fri, Feb 02, 2024 at 07:00:01PM +0300, Alexander Lakhin wrote: > > results in: > > NOTICE: merging definition of column "i" for child "b" > > NOTICE: merging definition of column "i" for child "c" > > ERROR: tuple already updated by self > > > > (This is similar to bug #18297, but ATExecAddColumn() isn't guilty in this > > case.) > > Still I suspect that the fix should be similar, so I'll go put a coin > on a missing CCI(). Hmm, let me have a look, I can probably get this one fixed today before embarking on a larger fix elsewhere in the same feature. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was amazing when I first started using it at 7.2, and I'm continually astounded by learning new features and techniques made available by the continuing work of the development team." Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php
On 2024-Feb-05, Alvaro Herrera wrote: > Hmm, let me have a look, I can probably get this one fixed today before > embarking on a larger fix elsewhere in the same feature. You know what -- this missing CCI has a much more visible impact, which is that the attnotnull marker that a primary key imposes on a partition is propagated early. So this regression test no longer fails: create table cnn2_parted(a int primary key) partition by list (a); create table cnn2_part1(a int); alter table cnn2_parted attach partition cnn2_part1 for values in (1); Here, in the existing code the ALTER TABLE ATTACH fails with the error message that ERROR: primary key column "a" is not marked NOT NULL but with the patch, this no longer occurs. I'm not sure that this behavior change is desirable ... I have vague memories of people complaining that this sort of error was not very welcome ... but on the other hand it seems now pretty clear that if it *is* desirable, then its implementation is no good, because a single added CCI breaks it. I'm leaning towards accepting the behavior change, but I'd like to investigate a little bit more first, but what do others think? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Вложения
On 2024-Feb-05, Alvaro Herrera wrote: > Subject: [PATCH v1] Fix failure to merge NOT NULL constraints in inheritance > > set_attnotnull() was not careful to CommandCounterIncrement() in cases > of multiple recursion. Omission in b0e96f311985. Eh, this needs to read "multiple inheritance" rather than "multiple recursion". (I'd also need to describe the change for the partitioning cases in the commit message.) -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Someone said that it is at least an order of magnitude more work to do production software than a prototype. I think he is wrong by at least an order of magnitude." (Brian Kernighan)
On 2024-Feb-05, Alvaro Herrera wrote: > So this regression test no longer fails: > > create table cnn2_parted(a int primary key) partition by list (a); > create table cnn2_part1(a int); > alter table cnn2_parted attach partition cnn2_part1 for values in (1); > Here, in the existing code the ALTER TABLE ATTACH fails with the error > message that > ERROR: primary key column "a" is not marked NOT NULL > but with the patch, this no longer occurs. I think this change is OK. In the partition, the primary key is created in the partition anyway (as expected) which marks the column as attnotnull[*], and the table is scanned for presence of NULLs if there's no not-null constraint, and not scanned if there's one. (The actual scan is inevitable anyway because we must check the partition constraint). This seems the behavior we want. [*] This attnotnull constraint is lost if you DETACH the partition and drop the primary key, which is also the behavior we want. While playing with it I noticed this other behavior change from 16, create table pa (a int primary key) partition by list (a); create table pe (a int unique); alter table pa attach partition pe for values in (1, null); In 16, we get the error: ERROR: column "a" in child table must be marked NOT NULL which is correct (because the PK requires not-null). In master we just let that through, but that seems to be a separate bug. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Saca el libro que tu religión considere como el indicado para encontrar la oración que traiga paz a tu alma. Luego rebootea el computador y ve si funciona" (Carlos Duclós)
On 2024-Feb-05, Alvaro Herrera wrote: > While playing with it I noticed this other behavior change from 16, > > create table pa (a int primary key) partition by list (a); > create table pe (a int unique); > alter table pa attach partition pe for values in (1, null); > > In 16, we get the error: > ERROR: column "a" in child table must be marked NOT NULL > which is correct (because the PK requires not-null). In master we just > let that through, but that seems to be a separate bug. Hmm, so my initial reaction was to make the constraint-matching code ignore the constraint in the partition-to-be if it's not the same type (this is what patch 0002 here does) ... but what ends up happening is that we create a separate, identical constraint+index for the primary key. I don't like that behavior too much myself, as it seems too magical and surprising, since it could cause the ALTER TABLE ATTACH operation of a large partition become costly and slower, since it needs to create an index instead of merely scanning the whole data. I'll look again at the idea of raising an error if the not-null constraint is not already present. That seems safer (and also, it's what we've been doing all along). -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Вложения
On Mon, Feb 5, 2024 at 5:51 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2024-Feb-05, Alvaro Herrera wrote: > > > Hmm, let me have a look, I can probably get this one fixed today before > > embarking on a larger fix elsewhere in the same feature. > > You know what -- this missing CCI has a much more visible impact, which > is that the attnotnull marker that a primary key imposes on a partition > is propagated early. So this regression test no longer fails: > > create table cnn2_parted(a int primary key) partition by list (a); > create table cnn2_part1(a int); > alter table cnn2_parted attach partition cnn2_part1 for values in (1); > > Here, in the existing code the ALTER TABLE ATTACH fails with the error > message that > ERROR: primary key column "a" is not marked NOT NULL > but with the patch, this no longer occurs. > > I'm not sure that this behavior change is desirable ... I have vague > memories of people complaining that this sort of error was not very > welcome ... but on the other hand it seems now pretty clear that if it > *is* desirable, then its implementation is no good, because a single > added CCI breaks it. > > I'm leaning towards accepting the behavior change, but I'd like to > investigate a little bit more first, but what do others think? > if you place CommandCounterIncrement inside the `if (recurse)` branch, then the regression test will be ok. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 9f516967..25e225c2 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7719,6 +7719,9 @@ set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse, false)); retval |= set_attnotnull(wqueue, childrel, childattno, recurse, lockmode); + + CommandCounterIncrement();
(I think I had already argued this point, but I don't see it in the archives, so here it is again). On 2024-Feb-07, jian he wrote: > if you place CommandCounterIncrement inside the `if (recurse)` branch, > then the regression test will be ok. Yeah, but don't you think this is too magical? I mean, randomly added CCIs in the execution path for other reasons would break this. Worse -- how can we _ensure_ that no CCIs occur at all? I mean, it's possible that an especially crafted multi-subcommand ALTER TABLE could contain just the right CCI to break things in the opposite way. The difference in behavior would be difficult to justify. (For good or ill, ALTER TABLE ATTACH PARTITION cannot run in a multi-subcommand ALTER TABLE, so this concern might be misplaced. Still, more certainty seems better than less.) I've pushed both these patches now, adding what seemed a reasonable set of test cases. If there still are cases behaving in unexpected ways, please let me know. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "La espina, desde que nace, ya pincha" (Proverbio africano)
On 2024-Jan-25, Andrew Bille wrote: > Starting from b0e96f31, pg_upgrade fails with inherited NOT NULL constraint: > For example upgrade from 9c13b6814a (or REL_12_STABLE .. REL_16_STABLE) to > b0e96f31 (or master) with following two tables (excerpt from > src/test/regress/sql/rules.sql) > > create table test_0 (id serial primary key); > create table test_1 (id integer primary key) inherits (test_0); I have pushed a fix which should hopefully fix this problem (d9f686a72e). Please give this a look. Thanks for reporting the issue. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "I apologize for the confusion in my previous responses. There appears to be an error." (ChatGPT)
Hello Alvaro, 18.04.2024 16:39, Alvaro Herrera wrote: > I have pushed a fix which should hopefully fix this problem > (d9f686a72e). Please give this a look. Thanks for reporting the issue. Please look at an assertion failure, introduced with d9f686a72: CREATE TABLE t(a int, NOT NULL a NO INHERIT); CREATE TABLE t2() INHERITS (t); ALTER TABLE t ADD CONSTRAINT nna NOT NULL a; TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() || CheckRelationLockedByMe(r, AccessShareLock, true)"), File: "relation.c", Line: 67, PID: 2980258 On d9f686a72~1 this script results in: ERROR: cannot change NO INHERIT status of inherited NOT NULL constraint "t_a_not_null" on relation "t" Best regards, Alexander
Hi Alexander, On 2024-Apr-18, Alexander Lakhin wrote: > 18.04.2024 16:39, Alvaro Herrera wrote: > > I have pushed a fix which should hopefully fix this problem > > (d9f686a72e). Please give this a look. Thanks for reporting the issue. > > Please look at an assertion failure, introduced with d9f686a72: > CREATE TABLE t(a int, NOT NULL a NO INHERIT); > CREATE TABLE t2() INHERITS (t); > > ALTER TABLE t ADD CONSTRAINT nna NOT NULL a; > TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() || > CheckRelationLockedByMe(r, AccessShareLock, true)"), File: "relation.c", > Line: 67, PID: 2980258 Ah, of course -- we're missing acquiring locks during the prep phase for the recursive case of ADD CONSTRAINT. So we just need to add find_all_inheritors() to do so in the AT_AddConstraint case in ATPrepCmd(). However these naked find_all_inheritors() call look a bit ugly to me, so I couldn't resist the temptation of adding a static function ATLockAllDescendants to clean it up a bit. I'll also add your script to the tests and push shortly. > On d9f686a72~1 this script results in: > ERROR: cannot change NO INHERIT status of inherited NOT NULL constraint "t_a_not_null" on relation "t" Right. Now I'm beginning to wonder if allowing ADD CONSTRAINT to mutate a pre-existing NO INHERIT constraint into a inheritable constraint (while accepting a constraint name in the command that we don't heed) is really what we want. Maybe we should throw some error when the affected constraint is the topmost one, and only accept the inheritance status change when we're recursing. Also I just noticed that in 9b581c534186 (which introduced this error message) I used ERRCODE_DATATYPE_MISMATCH ... Is that really appropriate here? -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Once again, thank you and all of the developers for your hard work on PostgreSQL. This is by far the most pleasant management experience of any database I've worked on." (Dan Harris) http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php
Вложения
On 2024-Apr-22, Alvaro Herrera wrote: > > On d9f686a72~1 this script results in: > > ERROR: cannot change NO INHERIT status of inherited NOT NULL constraint "t_a_not_null" on relation "t" > > Right. Now I'm beginning to wonder if allowing ADD CONSTRAINT to mutate > a pre-existing NO INHERIT constraint into a inheritable constraint > (while accepting a constraint name in the command that we don't heed) is > really what we want. Maybe we should throw some error when the affected > constraint is the topmost one, and only accept the inheritance status > change when we're recursing. So I added a restriction that we only accept such a change when recursively adding a constraint, or during binary upgrade. This should limit the damage: you're no longer able to change an existing constraint from NO INHERIT to YES INHERIT merely by doing another ALTER TABLE ADD CONSTRAINT. One thing that has me a little nervous about this whole business is whether we're set up to error out where some child table down the hierarchy has nulls, and we add a not-null constraint to it but fail to do a verification scan. I tried a couple of cases and AFAICS it works correctly, but maybe there are other cases I haven't thought about where it doesn't. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "You're _really_ hosed if the person doing the hiring doesn't understand relational systems: you end up with a whole raft of programmers, none of whom has had a Date with the clue stick." (Andrew Sullivan) https://postgr.es/m/20050809113420.GD2768@phlogiston.dyndns.org
Вложения
24.04.2024 20:36, Alvaro Herrera wrote: > So I added a restriction that we only accept such a change when > recursively adding a constraint, or during binary upgrade. This should > limit the damage: you're no longer able to change an existing constraint > from NO INHERIT to YES INHERIT merely by doing another ALTER TABLE ADD > CONSTRAINT. > > One thing that has me a little nervous about this whole business is > whether we're set up to error out where some child table down the > hierarchy has nulls, and we add a not-null constraint to it but fail to > do a verification scan. I tried a couple of cases and AFAICS it works > correctly, but maybe there are other cases I haven't thought about where > it doesn't. > Thank you for the fix! While studying the NO INHERIT option, I've noticed that the documentation probably misses it's specification for NOT NULL: https://www.postgresql.org/docs/devel/sql-createtable.html where column_constraint is: ... [ CONSTRAINT constraint_name ] { NOT NULL | NULL | CHECK ( expression ) [ NO INHERIT ] | Also, I've found a weird behaviour with a non-inherited NOT NULL constraint for a partitioned table: CREATE TABLE pt(a int NOT NULL NO INHERIT) PARTITION BY LIST (a); CREATE TABLE dp(a int NOT NULL); ALTER TABLE pt ATTACH PARTITION dp DEFAULT; ALTER TABLE pt DETACH PARTITION dp; fails with: ERROR: relation 16389 has non-inherited constraint "dp_a_not_null" Though with an analogous check constraint, I get: CREATE TABLE pt(a int, CONSTRAINT nna CHECK (a IS NOT NULL) NO INHERIT) PARTITION BY LIST (a); ERROR: cannot add NO INHERIT constraint to partitioned table "pt" Best regards, Alexander
On 2024-Apr-25, Alexander Lakhin wrote: > While studying the NO INHERIT option, I've noticed that the documentation > probably misses it's specification for NOT NULL: > https://www.postgresql.org/docs/devel/sql-createtable.html > > where column_constraint is: > ... > [ CONSTRAINT constraint_name ] > { NOT NULL | > NULL | > CHECK ( expression ) [ NO INHERIT ] | Hmm, okay, will fix. > Also, I've found a weird behaviour with a non-inherited NOT NULL > constraint for a partitioned table: > CREATE TABLE pt(a int NOT NULL NO INHERIT) PARTITION BY LIST (a); > CREATE TABLE dp(a int NOT NULL); > ALTER TABLE pt ATTACH PARTITION dp DEFAULT; > ALTER TABLE pt DETACH PARTITION dp; > fails with: > ERROR: relation 16389 has non-inherited constraint "dp_a_not_null" Ugh. Maybe a way to handle this is to disallow NO INHERIT in constraints on partitioned tables altogether. I mean, they are a completely useless gimmick, aren't they? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On 2024-Apr-25, Alvaro Herrera wrote: > > Also, I've found a weird behaviour with a non-inherited NOT NULL > > constraint for a partitioned table: > > CREATE TABLE pt(a int NOT NULL NO INHERIT) PARTITION BY LIST (a); > Ugh. Maybe a way to handle this is to disallow NO INHERIT in > constraints on partitioned tables altogether. I mean, they are a > completely useless gimmick, aren't they? Here are two patches that I intend to push soon (hopefully tomorrow). -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "No me acuerdo, pero no es cierto. No es cierto, y si fuera cierto, no me acuerdo." (Augusto Pinochet a una corte de justicia)
Вложения
Hello Alvaro, 01.05.2024 20:49, Alvaro Herrera wrote: > Here are two patches that I intend to push soon (hopefully tomorrow). > Thank you for fixing those issues! Could you also clarify, please, how CREATE TABLE ... LIKE is expected to work with NOT NULL constraints? I wonder whether EXCLUDING CONSTRAINTS (ALL) should cover not-null constraints too. What I'm seeing now, is that: CREATE TABLE t1 (i int, CONSTRAINT nn NOT NULL i); CREATE TABLE t2 (LIKE t1 EXCLUDING ALL); \d+ t2 -- ends with: Not-null constraints: "nn" NOT NULL "i" Or a similar case with PRIMARY KEY: CREATE TABLE t1 (i int PRIMARY KEY); CREATE TABLE t2 (LIKE t1 EXCLUDING CONSTRAINTS EXCLUDING INDEXES); \d+ t2 -- leaves: Not-null constraints: "t2_i_not_null" NOT NULL "i" Best regards, Alexander
Hello Alexander On 2024-May-02, Alexander Lakhin wrote: > Could you also clarify, please, how CREATE TABLE ... LIKE is expected to > work with NOT NULL constraints? It should behave identically to 16. If in 16 you end up with a not-nullable column, then in 17 you should get a not-null constraint. > I wonder whether EXCLUDING CONSTRAINTS (ALL) should cover not-null > constraints too. What I'm seeing now, is that: > CREATE TABLE t1 (i int, CONSTRAINT nn NOT NULL i); > CREATE TABLE t2 (LIKE t1 EXCLUDING ALL); > \d+ t2 > -- ends with: > Not-null constraints: > "nn" NOT NULL "i" In 16, this results in Table "public.t2" Column │ Type │ Collation │ Nullable │ Default │ Storage │ Compression │ Stats target │ Description ────────┼─────────┼───────────┼──────────┼─────────┼─────────┼─────────────┼──────────────┼───────────── i │ integer │ │ not null │ │ plain │ │ │ Access method: heap so the fact that we have a not-null constraint in pg17 is correct. > Or a similar case with PRIMARY KEY: > CREATE TABLE t1 (i int PRIMARY KEY); > CREATE TABLE t2 (LIKE t1 EXCLUDING CONSTRAINTS EXCLUDING INDEXES); > \d+ t2 > -- leaves: > Not-null constraints: > "t2_i_not_null" NOT NULL "i" Here you also end up with a not-nullable column in 16, so I made it do that. Now you could argue that EXCLUDING CONSTRAINTS is explicit in saying that we don't want the constraints; but in that case why did 16 mark the columns as not-null? The answer seems to be that the standard requires this. Look at 11.3 <table definition> syntax rule 9) b) iii) 4): 4) If the nullability characteristic included in LCDi is known not nullable, then let LNCi be NOT NULL; otherwise, let LNCi be the zero-length character string. where LCDi is "1) Let LCDi be the column descriptor of the i-th column of LT." and then 5) Let CDi be the <column definition> LCNi LDTi LNCi Now, you could claim that the standard doesn't mention INCLUDING/EXCLUDING CONSTRAINTS, therefore since we have come up with its definition then we should make it affect not-null constraints. However, there's also this note: NOTE 520 — <column constraint>s, except for NOT NULL, are not included in CDi; <column constraint definition>s are effectively transformed to <table constraint definition>s and are thereby also excluded. which is explicitly saying that not-null constraints are treated differently; in essence, with INCLUDING CONSTRAINTS we choose to affect the constraints that the standard says to ignore. Thanks for looking! -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Learn about compilers. Then everything looks like either a compiler or a database, and now you have two problems but one of them is fun." https://twitter.com/thingskatedid/status/1456027786158776329
02.05.2024 19:21, Alvaro Herrera wrote: > Now, you could claim that the standard doesn't mention > INCLUDING/EXCLUDING CONSTRAINTS, therefore since we have come up with > its definition then we should make it affect not-null constraints. > However, there's also this note: > > NOTE 520 — <column constraint>s, except for NOT NULL, are not included in > CDi; <column constraint definition>s are effectively transformed to <table > constraint definition>s and are thereby also excluded. > > which is explicitly saying that not-null constraints are treated > differently; in essence, with INCLUDING CONSTRAINTS we choose to affect > the constraints that the standard says to ignore. Thank you for very detailed and convincing explanation! Now I see what the last sentence here (from [1]) means: INCLUDING CONSTRAINTS CHECK constraints will be copied. No distinction is made between column constraints and table constraints. _Not-null constraints are always copied to the new table._ (I hadn't paid enough attention to it, because this exact paragraph is also presented in previous versions...) [1] https://www.postgresql.org/docs/devel/sql-createtable.html Best regards, Alexander
Hello, At Wed, 1 May 2024 19:49:35 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in > Here are two patches that I intend to push soon (hopefully tomorrow). This commit added and edited two error messages, resulting in using slightly different wordings "in" and "on" for relation constraints. + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"", === + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"", I think we usually use on in this case. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 2024-May-07, Kyotaro Horiguchi wrote: > Hello, > > At Wed, 1 May 2024 19:49:35 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in > > Here are two patches that I intend to push soon (hopefully tomorrow). > > This commit added and edited two error messages, resulting in using > slightly different wordings "in" and "on" for relation constraints. > > + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"", > === > + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"", Thank you, I hadn't noticed the inconsistency -- I fix this in the attached series. While trying to convince myself that I could mark the remaining open item for this work closed, I discovered that pg_dump fails to produce working output for some combinations. Notably, if I create Andrew Bille's example in 16: create table test_0 (id serial primary key); create table test_1 (id integer primary key) inherits (test_0); then current master's pg_dump produces output that the current server fails to restore, failing the PK creation in test_0: ALTER TABLE ONLY public.test_0 ADD CONSTRAINT test_0_pkey PRIMARY KEY (id); ERROR: cannot change NO INHERIT status of NOT NULL constraint "pgdump_throwaway_notnull_0" in relation "test_1" because we have already created the NOT NULL NO INHERIT constraint in test_1 when we created it, and because of d45597f72fe5, we refuse to change it into a regular inheritable constraint, which the PK in its parent table needs. I spent a long time trying to think how to fix this, and I had despaired wanting to write that I would need to revert the whole NOT NULL business for pg17 -- but that was until I realized that we don't actually need this NOT NULL NO INHERIT business except during pg_upgrade, and that simplifies things enough to give me confidence that the whole feature can be kept. Because, remember: the idea of those NO INHERIT "throwaway" constraints is that we can skip reading the data when we create the PRIMARY KEY during binary upgrade. We don't actually need the NO INHERIT constraints for anything during regular pg_dump. So what we can do, is restrict the usage of NOT NULL NO INHERIT so that they occur only during pg_upgrade. I think this will make Justin P. happier, because we no longer have these unsightly NOT NULL NO INHERIT nonstandard syntax in dumps. The attached patch series does that. Actually, it does a little more, but it's not really much: 0001: fix the typos pointed out by Kyotaro. 0002: A mechanical code movement that takes some ugly ballast out of getTableAttrs into its own routine. I realized that this new code was far too ugly and messy to be in the middle of filling the tbinfo struct of attributes. If you use "git show --color-moved --color-moved-ws=ignore-all-space" with this commit you can see that nothing happens apart from the code move. 0003: pgindent, fixes the comments just moved to account for different indentation depth. 0004: moves again the moved PQfnumber() calls back to getTableAttrs(), for efficiency (we don't want to search the result for those resnums for every single attribute of all tables being dumped). 0005: This is the actual code change I describe above. We restrict use_throwaway_nulls so that it's only set during binary upgrade mode. This changes pg_dump output; in the normal case, we no longer have NOT NULL NO INHERIT. I added one test stanza to verify that pg_upgrade retains these clauses, where they are critical. 0006: Tighten up what d45597f72fe5 did, in that outside of binary upgrade mode, we no longer accept changes to NOT NULL NO INHERIT constraints so that they become INHERIT. Previously we accepted that during recursion, but this isn't really very principled. (I had accepted this because pg_dump required it for some other cases). This changes some test output, and I also simplify some test cases that were testing stuff that's no longer interesting. (To push, I'll squash 0002+0003+0004 as a single one, and perhaps 0005 with them; I produced them like this only to make them easy to see what's changing.) I also have a pending patch for 16 that adds tables like the problematic ones so that they remain for future pg_upgrade testing. With the changes in this series, the whole thing finally works AFAICT. I did notice one more small bit of weirdness, which is that at the end of the process you may end up with constraints that retain the throwaway name. This doesn't seem at all critical, considering that you can't drop them anyway and such names do not survive a further dump (because they are marked as inherited constraint without a "local" definition, so they're not dumped separately). I would still like to fix it, but it seems to require unduly contortions so I may end up not doing anything about it. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Вложения
- 0001-Fix-typos-in-error-messages.patch
- 0002-Mechanical-move-of-not-null-code-out-of-getTableAttr.patch
- 0003-pgindent-run.patch
- 0004-Take-PQfnumber-calls-out-of-the-routine.patch
- 0005-Use-NOT-NULL-NO-INHERIT-only-in-pg_upgrade-mode.patch
- 0006-Don-t-accept-NO-INHERIT-changes-to-INHERIT-in-normal.patch
On Wed, May 8, 2024 at 4:42 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > I spent a long time trying to think how to fix this, and I had despaired > wanting to write that I would need to revert the whole NOT NULL business > for pg17 -- but that was until I realized that we don't actually need > this NOT NULL NO INHERIT business except during pg_upgrade, and that > simplifies things enough to give me confidence that the whole feature > can be kept. Yeah, I have to admit that the ongoing bug fixing here has started to make me a bit nervous, but I also can't totally follow everything that's under discussion, so I don't want to rush to judgement. I feel like we might need some documentation or a README or something that explains the takeaway from the recent commits dealing with no-inherit constraints. None of those commits updated the documentation, which may be fine, but neither the resulting behavior nor the reasoning behind it is obvious. It's not enough for it to be correct -- it has to be understandable enough to the hive mind that we can maintain it going forward. -- Robert Haas EDB: http://www.enterprisedb.com
On 2024-May-09, Robert Haas wrote: > Yeah, I have to admit that the ongoing bug fixing here has started to > make me a bit nervous, but I also can't totally follow everything > that's under discussion, so I don't want to rush to judgement. 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. 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). 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. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On 2024-May-11, Alvaro Herrera wrote: > I have found two more problems that [] require some more work to fix, > so I've decided to cut my losses now and revert the whole. Here's the revert patch, which I intend to push early tomorrow. Commits reverted are: 21ac38f498b33f0231842238b83847ec63dfe07b d45597f72fe53a53f6271d5ba4e7acf8fc9308a1 13daa33fa5a6d340f9be280db14e7b07ed11f92e 0cd711271d42b0888d36f8eda50e1092c2fed4b3 d72d32f52d26c9588256de90b9bc54fe312cee60 d9f686a72ee91f6773e5d2bc52994db8d7157a8e c3709100be73ad5af7ff536476d4d713bca41b1a 3af7217942722369a6eb7629e0fb1cbbef889a9b b0f7dd915bca6243f3daf52a81b8d0682a38ee3b ac22a9545ca906e70a819b54e76de38817c93aaf d0ec2ddbe088f6da35444fad688a62eae4fbd840 9b581c53418666205938311ef86047aa3c6b741f b0e96f311985bceba79825214f8e43f65afa653a with some significant conflict fixes (mostly in the last one). -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "El sentido de las cosas no viene de las cosas, sino de las inteligencias que las aplican a sus problemas diarios en busca del progreso." (Ernesto Hernández-Novich)
Вложения
On Sat, May 11, 2024 at 5:40 AM Alvaro Herrera <alvherre@alvh.no-ip.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
On 2024-May-13, Robert Haas wrote: > On Sat, May 11, 2024 at 5:40 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > 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. True. It may be that by setting up the upgrade SQL script differently, we don't need to make the distinction at all. I hope to be able to do that. > 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? The problematic point is the need to add NOT NULL constraints during table creation that don't exist in the table being dumped, for performance of primary key creation -- I called this a throwaway constraint. We needed to be able to drop those constraints after the PK was created. These were marked NO INHERIT to allow them to be dropped, which is easier if the children don't have them. This all worked fine. However, at some point we realized that we needed to add NOT NULL constraints in child tables for the columns in which the parent had a primary key. Then things become messy because we had the throwaway constraints on one hand and the not-nulls that descend from the PK on the other hand, where one was NO INHERIT and the other wasn't; worse if the child also has a primary key. It turned out that we didn't have any mechanism to transform a NO INHERIT constraint into a regular one that would be inherited. I added one, didn't like the way it worked, tried to restrict it but that caused other problems; this is the mess that led to the revert (pg_dump in normal mode would emit scripts that fail for some legitimate cases). One possible way forward might be to make pg_dump smarter by adding one more query to know the relationship between constraints that must be dropped and those that don't. Another might be to allow multiple not-null constraints on the same column (one inherits, the other doesn't, and you can drop them independently). There may be others. > 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. NOT VALID constraints would be extremely useful, for one thing (because then you don't need to exclusively-lock the table during a long scan in order to add a constraint), and it's just one step away from having these constraints be catalogued. It was also fixing some inconsistent handling of inheritance cases. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Mon, May 13, 2024 at 9:44 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > The problematic point is the need to add NOT NULL constraints during > table creation that don't exist in the table being dumped, for > performance of primary key creation -- I called this a throwaway > constraint. We needed to be able to drop those constraints after the PK > was created. These were marked NO INHERIT to allow them to be dropped, > which is easier if the children don't have them. This all worked fine. This seems really weird to me. Why is it necessary? I mean, in existing releases, if you declare a column as PRIMARY KEY, the columns included in the key are forced to be NOT NULL, and you can't change that for so long as they are included in the PRIMARY KEY. So I would have thought that after this patch, you'd end up with the same thing. One way of doing that would be to make the PRIMARY KEY depend on the now-catalogued NOT NULL constraints, and the other way would be to keep it as an ad-hoc prohibition, same as now. In PostgreSQL 16, I get a dump like this: CREATE TABLE public.foo ( a integer NOT NULL, b text ); COPY public.foo (a, b) FROM stdin; \. ALTER TABLE ONLY public.foo ADD CONSTRAINT foo_pkey PRIMARY KEY (a); If I'm dumping from an existing release, I don't see why any of that needs to change. The NOT NULL decoration should lead to a system-generated constraint name. If I'm dumping from a new release, the NOT NULL decoration needs to be replaced with CONSTRAINT existing_constraint_name NOT NULL. But I don't see why I need to end up with what the patch generates, which seems to be something like CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT. That kind of thing suggests that we're changing around the order of operations in pg_dump, probably by adding the NOT NULL constraints at a later stage than currently, and I think the proper solution is most likely to be to avoid doing that in the first place. > However, at some point we realized that we needed to add NOT NULL > constraints in child tables for the columns in which the parent had a > primary key. Then things become messy because we had the throwaway > constraints on one hand and the not-nulls that descend from the PK on > the other hand, where one was NO INHERIT and the other wasn't; worse if > the child also has a primary key. This seems like another problem that is created by changing the order of operations in pg_dump. > > 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. > > NOT VALID constraints would be extremely useful, for one thing (because > then you don't need to exclusively-lock the table during a long scan in > order to add a constraint), and it's just one step away from having > these constraints be catalogued. It was also fixing some inconsistent > handling of inheritance cases. I agree that NOT VALID constraints would be very useful. I'm a little scared by the idea of fixing inconsistent handling of inheritance cases, just for fear that there may be more things relying on the inconsistent behavior than we realize. I feel like this is an area where it's easy for changes to be scarier than they at first seem. I still have memories of discovering some of the current behavior back in the mid-2000s when I was learning PostgreSQL (and databases generally). It struck me as fiddly back then, and it still does. I feel like there are probably some behaviors that look like arbitrary decisions but are actually very important for some undocumented reason. That's not to say that we shouldn't try to make improvements, just that it may be hard to get right. -- Robert Haas EDB: http://www.enterprisedb.com
On 2024-May-13, Robert Haas wrote: > On Mon, May 13, 2024 at 9:44 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > The problematic point is the need to add NOT NULL constraints during > > table creation that don't exist in the table being dumped, for > > performance of primary key creation -- I called this a throwaway > > constraint. We needed to be able to drop those constraints after the PK > > was created. These were marked NO INHERIT to allow them to be dropped, > > which is easier if the children don't have them. This all worked fine. > > This seems really weird to me. Why is it necessary? I mean, in > existing releases, if you declare a column as PRIMARY KEY, the columns > included in the key are forced to be NOT NULL, and you can't change > that for so long as they are included in the PRIMARY KEY. The point is that a column can be in a primary key and not have an explicit not-null constraint. This is different from having a column be NOT NULL and having a primary key on top. In both cases the attnotnull flag is set; the difference between these two scenarios is what happens if you drop the primary key. If you do not have an explicit not-null constraint, then the attnotnull flag is lost as soon as you drop the primary key. You don't have to do DROP NOT NULL for that to happen. This means that if you have a column that's in the primary key but does not have an explicit not-null constraint, then we shouldn't make one up. (Which we would, if we were to keep an unadorned NOT NULL that we can't drop at the end of the dump.) > So I would have thought that after this patch, you'd end up with the > same thing. At least as I interpret the standard, you wouldn't. > One way of doing that would be to make the PRIMARY KEY depend on the > now-catalogued NOT NULL constraints, and the other way would be to > keep it as an ad-hoc prohibition, same as now. That would be against what [I think] the standard says. > But I don't see why I need to end up with what the patch generates, > which seems to be something like CONSTRAINT pgdump_throwaway_notnull_0 > NOT NULL NO INHERIT. That kind of thing suggests that we're changing > around the order of operations in pg_dump, probably by adding the NOT > NULL constraints at a later stage than currently, and I think the > proper solution is most likely to be to avoid doing that in the first > place. The point of the throwaway constraints is that they don't remain after the dump has restored completely. They are there only so that we don't have to scan the data looking for possible nulls when we create the primary key. We have a DROP CONSTRAINT for the throwaway not-nulls as soon as the PK is created. We're not changing any order of operations as such. > That's not to say that we shouldn't try to make improvements, just > that it may be hard to get right. Sure, that's why this patch has now been reverted twice :-) and has been in the works for ... how many years now? -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Mon, May 13, 2024 at 12:45 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > The point is that a column can be in a primary key and not have an > explicit not-null constraint. This is different from having a column be > NOT NULL and having a primary key on top. In both cases the attnotnull > flag is set; the difference between these two scenarios is what happens > if you drop the primary key. If you do not have an explicit not-null > constraint, then the attnotnull flag is lost as soon as you drop the > primary key. You don't have to do DROP NOT NULL for that to happen > > This means that if you have a column that's in the primary key but does > not have an explicit not-null constraint, then we shouldn't make one up. > (Which we would, if we were to keep an unadorned NOT NULL that we can't > drop at the end of the dump.) It seems to me that the practical thing to do about this problem is just decide not to solve it. I mean, it's currently the case that if you establish a PRIMARY KEY when you create a table, the columns of that key are marked NOT NULL and remain NOT NULL even if the primary key is later dropped. So, if that didn't change, we would be no less compliant with the SQL standard (or your reading of it) than we are now. And if you do really want to make that change, why not split it out into its own patch, so that the patch that does $SUBJECT is changing the minimal number of other things at the same time? That way, reverting something might not involve reverting everything, plus you could have a separate design discussion about what that fix ought to look like, separate from the issues that are truly inherent to cataloging NOT NULL constraints per se. What I meant about changing the order of operations is that, currently, the database knows that the column is NOT NULL before the COPY happens, and I don't think we can change that. I think you agree -- that's why you invented the throwaway constraints. As far as I can see, the problems all have to do with getting the "throwaway" part to happen correctly. It can't be a problem to just mark the relevant columns NOT NULL in the relevant tables -- we already do that. But if you want to discard some of those NOT NULL markings once the PRIMARY KEY is added, you have to know which ones to discard. If we just consider the most straightforward scenario where somebody does a full dump-and-restore, getting that right may be annoying, but it seems like it surely has to be possible. The dump will just have to understand which child tables (or, more generally, descendent tables) got a NOT NULL marking on a column because of the PK and which ones had an explicit marking in the old database and do the right thing in each case. But what if somebody does a selective restore of one table from a partitioning hierarchy? Currently, the columns that would have been part of the primary key end up NOT NULL, but the primary key itself is not restored because it can't be. What will happen in this new system? If you don't apply any NOT NULL constraints to those columns, then a user who restores one partition from an old dump and tries to reattach it to the correct partitioned table has to recheck the NOT NULL constraint, unlike now. If you apply a normal-looking garden-variety NOT NULL constraint to that column, you've invented a constraint that didn't exist in the source database. And if you apply a throwaway NOT NULL constraint but the user never attaches that table anywhere, then the throwaway constraint survives. None of those options sound very good to me. Another scenario: Say that you have a table with a PRIMARY KEY. For some reason, you want to drop the primary key and then add it back. Well, with this definitional change, as soon as you drop it, you forget that the underlying columns don't contain any nulls, so when you add it back, you have to check them again. I don't know who would find that behavior an improvement over what we have today. So I don't really think it's a great idea to change this behavior, but even if it is, is it such a good idea that we want to sink the whole patch set repeatedly over it, as has already happened twice now? I feel that if we did what Tom suggested a year ago in https://www.postgresql.org/message-id/3801207.1681057430@sss.pgh.pa.us -- "I'm inclined to think that this idea of suppressing the implied NOT NULL from PRIMARY KEY is a nonstarter and we should just go ahead and make such a constraint" -- there's a very good chance that a revert would have been avoided here and it would still be just as valid to think of revisiting this particular question in a future release as it is now. -- Robert Haas EDB: http://www.enterprisedb.com
On 2024-May-13, Robert Haas wrote: > It seems to me that the practical thing to do about this problem is > just decide not to solve it. I mean, it's currently the case that if > you establish a PRIMARY KEY when you create a table, the columns of > that key are marked NOT NULL and remain NOT NULL even if the primary > key is later dropped. So, if that didn't change, we would be no less > compliant with the SQL standard (or your reading of it) than we are > now. [...] > So I don't really think it's a great idea to change this behavior, but > even if it is, is it such a good idea that we want to sink the whole > patch set repeatedly over it, as has already happened twice now? I > feel that if we did what Tom suggested a year ago in > https://www.postgresql.org/message-id/3801207.1681057430@sss.pgh.pa.us > -- "I'm inclined to think that this idea of suppressing the implied > NOT NULL from PRIMARY KEY is a nonstarter and we should just go ahead > and make such a constraint" [...] Hmm, I hadn't interpreted Tom's message the way you suggest, and you may be right that it might be a good way forward. I'll keep this in mind for next time. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "No es bueno caminar con un hombre muerto"
On Sun, May 12, 2024 at 04:56:09PM +0200, Álvaro Herrera wrote: > On 2024-May-11, Alvaro Herrera wrote: > > > I have found two more problems that [] require some more work to fix, > > so I've decided to cut my losses now and revert the whole. > > Here's the revert patch, which I intend to push early tomorrow. > > Commits reverted are: > 21ac38f498b33f0231842238b83847ec63dfe07b > d45597f72fe53a53f6271d5ba4e7acf8fc9308a1 > 13daa33fa5a6d340f9be280db14e7b07ed11f92e > 0cd711271d42b0888d36f8eda50e1092c2fed4b3 > d72d32f52d26c9588256de90b9bc54fe312cee60 > d9f686a72ee91f6773e5d2bc52994db8d7157a8e > c3709100be73ad5af7ff536476d4d713bca41b1a > 3af7217942722369a6eb7629e0fb1cbbef889a9b > b0f7dd915bca6243f3daf52a81b8d0682a38ee3b > ac22a9545ca906e70a819b54e76de38817c93aaf > d0ec2ddbe088f6da35444fad688a62eae4fbd840 > 9b581c53418666205938311ef86047aa3c6b741f > b0e96f311985bceba79825214f8e43f65afa653a > > with some significant conflict fixes (mostly in the last one). Turns out these commits generated a single release note item, which I have now removed with the attached committed patch. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Вложения
On Mon, May 13, 2024 at 09:00:28AM -0400, Robert Haas wrote: > > 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. I assume Alvaro is saying that pg_upgrade has only a single session, which is unique and might make things easier for him. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On 2024-May-14, Bruce Momjian wrote: > Turns out these commits generated a single release note item, which I > have now removed with the attached committed patch. Hmm, but the commits about not-null constraints for domains were not reverted, only the ones for constraints on relations. I think the release notes don't properly address the ones on domains. I think it's at least these two commits: > -Author: Peter Eisentraut <peter@eisentraut.org> > -2024-03-20 [e5da0fe3c] Catalog domain not-null constraints > -Author: Peter Eisentraut <peter@eisentraut.org> > -2024-04-15 [9895b35cb] Fix ALTER DOMAIN NOT NULL syntax It may still be a good idea to make a note about those, at least to point out that information_schema now lists them. For example, pg11 release notes had this item <!-- 2018-02-07 [32ff26911] Add more information_schema columns --> <para> Add <literal>information_schema</literal> columns related to table constraints and triggers (Peter Eisentraut) </para> <para> Specifically, <structname>triggers</structname>.<structfield>action_order</structfield>, <structname>triggers</structname>.<structfield>action_reference_old_table</structfield>, and <structname>triggers</structname>.<structfield>action_reference_new_table</structfield> are now populated, where before they were always null. Also, <structname>table_constraints</structname>.<structfield>enforced</structfield> now exists but is not yet usefully populated. </para> -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On 15.05.24 09:50, Alvaro Herrera wrote: > On 2024-May-14, Bruce Momjian wrote: > >> Turns out these commits generated a single release note item, which I >> have now removed with the attached committed patch. > > Hmm, but the commits about not-null constraints for domains were not > reverted, only the ones for constraints on relations. I think the > release notes don't properly address the ones on domains. I think it's > at least these two commits: > >> -Author: Peter Eisentraut <peter@eisentraut.org> >> -2024-03-20 [e5da0fe3c] Catalog domain not-null constraints >> -Author: Peter Eisentraut <peter@eisentraut.org> >> -2024-04-15 [9895b35cb] Fix ALTER DOMAIN NOT NULL syntax I'm confused that these were kept. The first one was specifically to make the catalog representation of domain not-null constraints consistent with table not-null constraints. But the table part was reverted, so now the domain constraints are inconsistent again. The second one refers to the first one, but it might also fix some additional older issue, so it would need more investigation.
On Wed, May 15, 2024 at 09:50:36AM +0200, Álvaro Herrera wrote: > On 2024-May-14, Bruce Momjian wrote: > > > Turns out these commits generated a single release note item, which I > > have now removed with the attached committed patch. > > Hmm, but the commits about not-null constraints for domains were not > reverted, only the ones for constraints on relations. I think the > release notes don't properly address the ones on domains. I think it's > at least these two commits: > > > -Author: Peter Eisentraut <peter@eisentraut.org> > > -2024-03-20 [e5da0fe3c] Catalog domain not-null constraints > > -Author: Peter Eisentraut <peter@eisentraut.org> > > -2024-04-15 [9895b35cb] Fix ALTER DOMAIN NOT NULL syntax > > It may still be a good idea to make a note about those, at least to > point out that information_schema now lists them. For example, pg11 > release notes had this item Let me explain what I did to adjust the release notes. I took your commit hashes, which were longer than mine, and got the commit subject text from them. I then searched the release notes to see which commit subjects existed in the document. Only the first three did, and the release note item has five commits. The then tested if the last two patches could be reverted, and 'patch' thought they could be, so that confirmed they were not reverted. However, there was no text in the release note item that corresponded to the commits, so I just removed the entire item. What I now think happened is that the last two commits were considered part of the larger NOT NULL change, and not worth mentioning separately, but now that the NOT NULL part is reverted, we might need to mention them. I rarely handle such complex cases so I don't think I was totally correct in my handling. Let's get a reply to Peter Eisentraut's question and we can figure out what to do. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.