Thread
Commits
-
Suppress "may be used uninitialized" warnings from older compilers.
- fc5e966f73f0 18.0 landed
-
Elide not-null constraint checks on child tables during PK creation
- 11ff192b5bb7 18.0 landed
-
Remove unnecessary code to handle CONSTR_NOTNULL
- 5b291d1c9c09 18.0 landed
-
Silence compilers about extractNotNullColumn()
- ff239c3bf4e8 18.0 landed
-
Add pg_constraint rows for not-null constraints
- 14e87ffa5c54 18.0 landed
-
not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-08-31T03:58:33Z
Hello Here I present another attempt at making not-null constraints be catalogued. This is largely based on the code reverted at 9ce04b50e120, except that we now have a not-null constraint automatically created for every column of a primary key, and such constraint cannot be removed while the PK exists. Thanks to this, a lot of rather ugly code is gone, both in pg_dump and in backend -- in particular the handling of NO INHERIT, which was needed for pg_dump. Noteworthy psql difference: because there are now even more not-null constraints than before, the \d+ display would be far too noisy if we just let it grow. So here I've made it omit any constraints that underlie the primary key. This should be OK since you can't do much with those constraints while the PK is still there. If you drop the PK, the next \d+ will show those constraints. One thing that regretfully I haven't yet had time for, is paring down the original test code: a lot of it is verifying the old semantics, particularly for NO INHERIT constraints, which had grown ugly special cases. It now mostly raises errors; or the tests are simply redundant. I'm going to remove that stuff as soon as I'm back on my normal work timezone. sepgsql is untested. I'm adding this to the September commitfest. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "¿Cómo puedes confiar en algo que pagas y que no ves, y no confiar en algo que te dan y te lo muestran?" (Germán Poo)
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2024-09-01T05:45:57Z
Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年8月31日周六 11:59写道： > Hello > > Here I present another attempt at making not-null constraints be > catalogued. This is largely based on the code reverted at 9ce04b50e120, > except that we now have a not-null constraint automatically created for > every column of a primary key, and such constraint cannot be removed > while the PK exists. Thanks to this, a lot of rather ugly code is gone, > both in pg_dump and in backend -- in particular the handling of NO > INHERIT, which was needed for pg_dump. > > Noteworthy psql difference: because there are now even more not-null > constraints than before, the \d+ display would be far too noisy if we > just let it grow. So here I've made it omit any constraints that > underlie the primary key. This should be OK since you can't do much > with those constraints while the PK is still there. If you drop the PK, > the next \d+ will show those constraints. > > One thing that regretfully I haven't yet had time for, is paring down > the original test code: a lot of it is verifying the old semantics, > particularly for NO INHERIT constraints, which had grown ugly special > cases. It now mostly raises errors; or the tests are simply redundant. > I'm going to remove that stuff as soon as I'm back on my normal work > timezone. > > sepgsql is untested. > > I'm adding this to the September commitfest. Thanks for working on this again. AT_PASS_OLD_COL_ATTRS, I didn't see any other code that used it. I don't think it's necessary. I will take the time to look over the attached patch. -- Tender Wang
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2024-09-02T10:33:18Z
Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年8月31日周六 11:59写道： > Hello > > Here I present another attempt at making not-null constraints be > catalogued. This is largely based on the code reverted at 9ce04b50e120, > except that we now have a not-null constraint automatically created for > every column of a primary key, and such constraint cannot be removed > while the PK exists. Thanks to this, a lot of rather ugly code is gone, > both in pg_dump and in backend -- in particular the handling of NO > INHERIT, which was needed for pg_dump. > > Noteworthy psql difference: because there are now even more not-null > constraints than before, the \d+ display would be far too noisy if we > just let it grow. So here I've made it omit any constraints that > underlie the primary key. This should be OK since you can't do much > with those constraints while the PK is still there. If you drop the PK, > the next \d+ will show those constraints. > > One thing that regretfully I haven't yet had time for, is paring down > the original test code: a lot of it is verifying the old semantics, > particularly for NO INHERIT constraints, which had grown ugly special > cases. It now mostly raises errors; or the tests are simply redundant. > I'm going to remove that stuff as soon as I'm back on my normal work > timezone. > > sepgsql is untested. > > I'm adding this to the September commitfest. > The attached patch adds List *nnconstraints, which store the not-null definition, in struct CreateStmt. This makes me a little confused about List *constraints in struct CreateStmt. Actually, the List constraints store ckeck constraint, and it will be better if the comments can reflect that. Re-naming it to List *ckconstraints seems more reasonable. But a lot of codes that use stmt->constraints will be changed. Since AddRelationNewConstraints() can now add not-null column constraint, the comments about AddRelationNewConstraints() should tweak a little. "All entries in newColDefaults will be processed. Entries in newConstraints will be processed only if they are CONSTR_CHECK type." Now, the type of new constraints may be not-null constraints. If the column has already had one not-null constraint, and we add same not-null constraint again. Then the code will call AdjustNotNullInheritance1() in AddRelationNewConstraints(). The comments before entering AdjustNotNullInheritance1() in AddRelationNewConstraints() look confusing to me. Because constraint is not inherited. -- Tender Wang
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2024-09-03T07:17:33Z
Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年8月31日周六 11:59写道： > Hello > > Here I present another attempt at making not-null constraints be > catalogued. This is largely based on the code reverted at 9ce04b50e120, > except that we now have a not-null constraint automatically created for > every column of a primary key, and such constraint cannot be removed > while the PK exists. Thanks to this, a lot of rather ugly code is gone, > both in pg_dump and in backend -- in particular the handling of NO > INHERIT, which was needed for pg_dump. > > Noteworthy psql difference: because there are now even more not-null > constraints than before, the \d+ display would be far too noisy if we > just let it grow. So here I've made it omit any constraints that > underlie the primary key. This should be OK since you can't do much > with those constraints while the PK is still there. If you drop the PK, > the next \d+ will show those constraints. > > One thing that regretfully I haven't yet had time for, is paring down > the original test code: a lot of it is verifying the old semantics, > particularly for NO INHERIT constraints, which had grown ugly special > cases. It now mostly raises errors; or the tests are simply redundant. > I'm going to remove that stuff as soon as I'm back on my normal work > timezone. > > sepgsql is untested. > > I'm adding this to the September commitfest. > The test case in constraints.sql, as below: CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL); ^^^^^^^^^^ There are two not-null definitions, and is the second one redundant? When we drop the column not-null constraint, we will enter ATExecDropNotNull(). Then, it calls findNotNullConstraint() to get the constraint tuple. We already have attnum before the call findNotNullConstraint(). Can we directly call findNotNullConstraintAttnum()? In RemoveConstraintById(), I see below comments: "For not-null and primary key constraints, obtain the list of columns affected, so that we can reset their attnotnull flags below." However, there are no related codes that reflect the above comments. -- Thanks, Tender Wang -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-09T01:36:00Z
On Sat, Aug 31, 2024 at 11:59 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Hello > > Here I present another attempt at making not-null constraints be > catalogued. This is largely based on the code reverted at 9ce04b50e120, > except that we now have a not-null constraint automatically created for > every column of a primary key, and such constraint cannot be removed > while the PK exists. Thanks to this, a lot of rather ugly code is gone, > both in pg_dump and in backend -- in particular the handling of NO > INHERIT, which was needed for pg_dump. > > Noteworthy psql difference: because there are now even more not-null > constraints than before, the \d+ display would be far too noisy if we > just let it grow. So here I've made it omit any constraints that > underlie the primary key. This should be OK since you can't do much > with those constraints while the PK is still there. If you drop the PK, > the next \d+ will show those constraints. > hi. my brief review. create table t1(a int, b int, c int not null, primary key(a, b)); \d+ t1 ERROR: operator is not unique: smallint[] <@ smallint[] LINE 8: coalesce(NOT ARRAY[at.attnum] <@ (SELECT conkey FROM pg_cata... ^ HINT: Could not choose a best candidate operator. You might need to add explicit type casts. the regression test still passed, i have no idea why. anyway, the following changes make the above ERROR disappear. also seems more lean. printfPQExpBuffer(&buf, /* FIXME the coalesce trick looks silly. What's a better way? */ "SELECT co.conname, at.attname, co.connoinherit, co.conislocal,\n" "co.coninhcount <> 0\n" "FROM pg_catalog.pg_constraint co JOIN\n" "pg_catalog.pg_attribute at ON\n" "(at.attrelid = co.conrelid AND at.attnum = co.conkey[1])\n" "WHERE co.contype = 'n' AND\n" "co.conrelid = '%s'::pg_catalog.regclass AND\n" "coalesce(NOT ARRAY[at.attnum] <@ (SELECT conkey FROM pg_catalog.pg_constraint\n" " WHERE contype = 'p' AND conrelid = '%s'::regclass), true)\n" "ORDER BY at.attnum", oid, oid); change to printfPQExpBuffer(&buf, "SELECT co.conname, at.attname, co.connoinherit, co.conislocal,\n" "co.coninhcount <> 0\n" "FROM pg_catalog.pg_constraint co JOIN\n" "pg_catalog.pg_attribute at ON\n" "(at.attrelid = co.conrelid AND at.attnum = co.conkey[1])\n" "WHERE co.contype = 'n' AND\n" "co.conrelid = '%s'::pg_catalog.regclass AND\n" "NOT EXISTS (SELECT 1 FROM pg_catalog.pg_constraint co1 where co1.contype = 'p'\n" "AND at.attnum = any(co1.conkey) AND co1.conrelid = '%s'::pg_catalog.regclass)\n" "ORDER BY at.attnum", oid, oid); steal idea from https://stackoverflow.com/a/75614278/15603477 ============ create type comptype as (r float8, i float8); create domain dcomptype1 as comptype not null no inherit; with cte as ( SELECT oid, conrelid::regclass, conname FROM pg_catalog.pg_constraint where contypid in ('dcomptype1'::regtype)) select pg_get_constraintdef(oid) from cte; current output is NOT NULL but it's not the same as CREATE TABLE ATACC1 (a int, not null a no inherit); with cte as ( SELECT oid, conrelid::regclass, conname FROM pg_catalog.pg_constraint where conrelid in ('ATACC1'::regclass)) select pg_get_constraintdef(oid) from cte; NOT NULL a NO INHERIT i don't really sure the meaning of "on inherit" in "create domain dcomptype1 as comptype not null no inherit;" ==================== bold idea. print out the constraint name: violates not-null constraint \"%s\" for the following code: ereport(ERROR, (errcode(ERRCODE_NOT_NULL_VIOLATION), errmsg("null value in column \"%s\" of relation \"%s\" violates not-null constraint", NameStr(att->attname), RelationGetRelationName(orig_rel)), val_desc ? errdetail("Failing row contains %s.", val_desc) : 0, errtablecol(orig_rel, attrChk))); ==================== in extractNotNullColumn we can Assert(colnum > 0); create table t3(a int , b int , c int ,not null a, not null c, not null b, not null tableoid); this should not be allowed? foreach(lc, RelationGetNotNullConstraints(RelationGetRelid(relation), false)) { AlterTableCmd *atsubcmd; atsubcmd = makeNode(AlterTableCmd); atsubcmd->subtype = AT_AddConstraint; atsubcmd->def = (Node *) lfirst_node(Constraint, lc); atsubcmds = lappend(atsubcmds, atsubcmd); } forgive me for being hypocritical. I guess this is not a good coding pattern. one reason would be: if you do: = list *a = RelationGetNotNullConstraints(RelationGetRelid(relation), false); foreach(lc, a) = then you can call pprint(a). + /* + * If INCLUDING INDEXES is not given and a primary key exists, we need to + * add not-null constraints to the columns covered by the PK (except those + * that already have one.) This is required for backwards compatibility. + */ + if ((table_like_clause->options & CREATE_TABLE_LIKE_INDEXES) == 0) + { + Bitmapset *pkcols; + int x = -1; + Bitmapset *donecols = NULL; + ListCell *lc; + + /* + * Obtain a bitmapset of columns on which we'll add not-null + * constraints in expandTableLikeClause, so that we skip this for + * those. + */ + foreach(lc, RelationGetNotNullConstraints(RelationGetRelid(relation), true)) + { + CookedConstraint *cooked = (CookedConstraint *) lfirst(lc); + + donecols = bms_add_member(donecols, cooked->attnum); + } + + pkcols = RelationGetIndexAttrBitmap(relation, + INDEX_ATTR_BITMAP_PRIMARY_KEY); + while ((x = bms_next_member(pkcols, x)) >= 0) + { + Constraint *notnull; + AttrNumber attnum = x + FirstLowInvalidHeapAttributeNumber; + String *colname; + Form_pg_attribute attForm; + + /* ignore if we already have one for this column */ + if (bms_is_member(attnum, donecols)) + continue; + + attForm = TupleDescAttr(tupleDesc, attnum - 1); + colname = makeString(pstrdup(NameStr(attForm->attname))); + notnull = makeNotNullConstraint(colname); + + cxt->nnconstraints = lappend(cxt->nnconstraints, notnull); + } + } this part (" if (bms_is_member(attnum, donecols))" will always be true? donecols is all not-null attnums, pkcols is pk not-null attnums. so pkcols info will always be included in donecols. i placed a "elog(INFO, "%s we are in", __func__);" above "attForm = TupleDescAttr(tupleDesc, attnum - 1);" all regression tests still passed. -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-09T08:31:33Z
On Mon, Sep 2, 2024 at 6:33 PM Tender Wang <tndrwang@gmail.com> wrote: > > > > The attached patch adds List *nnconstraints, which store the not-null definition, in struct CreateStmt. > This makes me a little confused about List *constraints in struct CreateStmt. Actually, the List constraints > store ckeck constraint, and it will be better if the comments can reflect that. Re-naming it to List *ckconstraints > seems more reasonable. But a lot of codes that use stmt->constraints will be changed. > hi. seems you forgot to attach the patch? I also noticed this minor issue. I have no preference for Renaming it to List *ckconstraints. +1 for better comments. maybe reword to >>> List *constraints; /* CHECK constraints (list of Constraint nodes) */ >>> On Tue, Sep 3, 2024 at 3:17 PM Tender Wang <tndrwang@gmail.com> wrote: > > The test case in constraints.sql, as below: > CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL); > ^^^^^^^^^^ > There are two not-null definitions, and is the second one redundant? > hi. i think this is ok. please see function transformColumnDefinition and variable saw_nullable. we need to make sure this: CREATE TABLE notnull_tbl3 (a INTEGER NULL NOT NULL); fails. of course, it's also OK do this: CREATE TABLE notnull_tbl3 (a INTEGER NULL NULL);
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2024-09-09T09:08:09Z
jian he <jian.universality@gmail.com> 于2024年9月9日周一 16:31写道： > On Mon, Sep 2, 2024 at 6:33 PM Tender Wang <tndrwang@gmail.com> wrote: > > > > > > > > The attached patch adds List *nnconstraints, which store the not-null > definition, in struct CreateStmt. > > This makes me a little confused about List *constraints in struct > CreateStmt. Actually, the List constraints > > store ckeck constraint, and it will be better if the comments can > reflect that. Re-naming it to List *ckconstraints > > seems more reasonable. But a lot of codes that use stmt->constraints > will be changed. > > > hi. > seems you forgot to attach the patch? > I also noticed this minor issue. > I have no preference for Renaming it to List *ckconstraints. > +1 for better comments. maybe reword to > >>> > List *constraints; /* CHECK constraints (list of Constraint > nodes) */ > >>> > I just gave advice; whether it is accepted or not, it's up to Alvaro. If Alvaro agrees with the advice, he will patch a new one. We can continue to review the new patch. If Alvaro disagrees, he doesn't need to change the current patch. I think this way will be more straightforward for others who will review this feature. > > On Tue, Sep 3, 2024 at 3:17 PM Tender Wang <tndrwang@gmail.com> wrote: > > > > The test case in constraints.sql, as below: > > CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL); > > > ^^^^^^^^^^ > > There are two not-null definitions, and is the second one redundant? > > > > hi. > i think this is ok. please see > function transformColumnDefinition and variable saw_nullable. > Yeah, it is ok. > we need to make sure this: > CREATE TABLE notnull_tbl3 (a INTEGER NULL NOT NULL); > fails. > > > of course, it's also OK do this: > CREATE TABLE notnull_tbl3 (a INTEGER NULL NULL); > -- Thanks, Tender Wang
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-10T14:05:10Z
On 2024-Sep-09, jian he wrote: > bold idea. print out the constraint name: violates not-null constraint \"%s\" > for the following code: > ereport(ERROR, > (errcode(ERRCODE_NOT_NULL_VIOLATION), > errmsg("null value in column \"%s\" of > relation \"%s\" violates not-null constraint", > NameStr(att->attname), > RelationGetRelationName(orig_rel)), > val_desc ? errdetail("Failing row contains > %s.", val_desc) : 0, > errtablecol(orig_rel, attrChk))); I gave this a quick run, but I'm not sure it actually improves things much. Here's one change from the regression tests. What do you think? INSERT INTO reloptions_test VALUES (1, NULL), (NULL, NULL); -ERROR: null value in column "i" of relation "reloptions_test" violates not-null constraint +ERROR: null value in column "i" of relation "reloptions_test" violates not-null constraint "reloptions_test_i_not_null" What do I get from having the constraint name? It's not like I'm going to drop the constraint and retry the insert. Here's the POC-quality patch for this. I changes a lot of regression tests, which I don't patch here. (But that's not the reason for me thinking that this isn't worth it.) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 29e186fa73d..d84137f4f43 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1907,6 +1907,7 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, * have been converted from the original input tuple after tuple routing. * 'resultRelInfo' is the final result relation, after tuple routing. */ +#include "catalog/pg_constraint.h" void ExecConstraints(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate) @@ -1932,6 +1933,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo, char *val_desc; Relation orig_rel = rel; TupleDesc orig_tupdesc = RelationGetDescr(rel); + char *conname; /* * If the tuple has been routed, it's been converted to the @@ -1970,14 +1972,24 @@ ExecConstraints(ResultRelInfo *resultRelInfo, tupdesc, modifiedCols, 64); + { + HeapTuple tuple; + Form_pg_constraint conForm; + + tuple = findNotNullConstraintAttnum(RelationGetRelid(orig_rel), + att->attnum); + conForm = (Form_pg_constraint) GETSTRUCT(tuple); + conname = pstrdup(NameStr(conForm->conname)); + } ereport(ERROR, - (errcode(ERRCODE_NOT_NULL_VIOLATION), - errmsg("null value in column \"%s\" of relation \"%s\" violates not-null constraint", - NameStr(att->attname), - RelationGetRelationName(orig_rel)), - val_desc ? errdetail("Failing row contains %s.", val_desc) : 0, - errtablecol(orig_rel, attrChk))); + errcode(ERRCODE_NOT_NULL_VIOLATION), + errmsg("null value in column \"%s\" of relation \"%s\" violates not-null constraint \"%s\"", + NameStr(att->attname), + RelationGetRelationName(orig_rel), + conname), + val_desc ? errdetail("Failing row contains %s.", val_desc) : 0, + errtablecol(orig_rel, attrChk)); } } } -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-10T14:09:20Z
On 2024-Sep-02, Tender Wang wrote: > The attached patch adds List *nnconstraints, which store the not-null > definition, in struct CreateStmt. This makes me a little confused > about List *constraints in struct CreateStmt. Actually, the List > constraints store ckeck constraint, and it will be better if the > comments can reflect that. Re-naming it to List *ckconstraints seems > more reasonable. But a lot of codes that use stmt->constraints will be > changed. Well, if you look at the comment about CreateStmt, there's this: /* ---------------------- * Create Table Statement * * NOTE: in the raw gram.y output, ColumnDef and Constraint nodes are * intermixed in tableElts, and constraints and nnconstraints are NIL. After * parse analysis, tableElts contains just ColumnDefs, nnconstraints contains * Constraint nodes of CONSTR_NOTNULL type from various sources, and * constraints contains just CONSTR_CHECK Constraint nodes. * ---------------------- */ So if we were to rename 'constraints' to 'ckconstraints', it would no longer reflect the fact that not-null ones can be in the former list initially. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "No hay ausente sin culpa ni presente sin disculpa" (Prov. francés)
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-10T18:18:35Z
Hello, here's a v2 of this patch. I have fixed --I think-- all the issues you and Tender Wang reported (unless I declined a fix in some previous email). It turns out I have not finished cleaning up the regression tests from now-useless additions, because while doing so last time around I found some bugs (especially one around comments in not-null constraints, which weren't being preserved by an ALTER TABLE TYPE command; that required a new strange hack in RememberConstraintForRebuilding), but also the LIKE clause again). Also, in this version there's a problem in the pg_upgrade test, which I hope to fix tomorrow. This code can also be found at https://github.com/alvherre/postgres/tree/notnull-init-18 (Please do ignore 89685e691f75309fec882723272c8b17106e6aa2, that was a merge mistake). On 2024-Sep-09, jian he wrote: > change to > > printfPQExpBuffer(&buf, > "SELECT co.conname, at.attname, > co.connoinherit, co.conislocal,\n" > "co.coninhcount <> 0\n" > "FROM pg_catalog.pg_constraint co JOIN\n" > "pg_catalog.pg_attribute at ON\n" > "(at.attrelid = co.conrelid AND > at.attnum = co.conkey[1])\n" > "WHERE co.contype = 'n' AND\n" > "co.conrelid = '%s'::pg_catalog.regclass AND\n" > "NOT EXISTS (SELECT 1 FROM > pg_catalog.pg_constraint co1 where co1.contype = 'p'\n" > "AND at.attnum = any(co1.conkey) AND > co1.conrelid = '%s'::pg_catalog.regclass)\n" > "ORDER BY at.attnum", > oid, > oid); Ah, obvious now that I see it, thanks. > ============ > create type comptype as (r float8, i float8); > create domain dcomptype1 as comptype not null no inherit; > with cte as ( > SELECT oid, conrelid::regclass, conname FROM pg_catalog.pg_constraint > where contypid in ('dcomptype1'::regtype)) > select pg_get_constraintdef(oid) from cte; > i don't really sure the meaning of "on inherit" in > "create domain dcomptype1 as comptype not null no inherit;" Yeah, I think we need to reject NO INHERIT constraints for domains. I've done so in this new version. > ==================== > in extractNotNullColumn > we can Assert(colnum > 0); True, assuming we reject the case for system columns as you say below. > create table t3(a int , b int , c int ,not null a, not null c, not > null b, not null tableoid); > this should not be allowed? Added explicit rejection here and in a couple of other places. > foreach(lc, RelationGetNotNullConstraints(RelationGetRelid(relation), false)) > forgive me for being hypocritical. > I guess this is not a good coding pattern. > one reason would be: if you do: > = > list *a = RelationGetNotNullConstraints(RelationGetRelid(relation), false); > foreach(lc, a) > = > then you can call pprint(a). I'm undecided about this, but seeing that we don't use this pattern almost anywhere, I've gone ahead and added the extra local variable. > + /* > + * If INCLUDING INDEXES is not given and a primary key exists, we need to > + * add not-null constraints to the columns covered by the PK (except those > + * that already have one.) This is required for backwards compatibility. > this part (" if (bms_is_member(attnum, donecols))" will always be true? > donecols is all not-null attnums, pkcols is pk not-null attnums. > so pkcols info will always be included in donecols. > i placed a "elog(INFO, "%s we are in", __func__);" > above > "attForm = TupleDescAttr(tupleDesc, attnum - 1);" > all regression tests still passed. Yes, this code is completely unnecessary now. Removed. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-11T01:11:47Z
On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Hello, here's a v2 of this patch. I have fixed --I think-- all the > issues you and Tender Wang reported (unless I declined a fix in some > previous email). > + /* + * The constraint must appear as inherited in children, so create a + * modified constraint object to use. + */ + constr = copyObject(constr); + constr->inhcount = 1; in ATAddCheckNNConstraint, we don't need the above copyObject call. because at the beginning of ATAddCheckNNConstraint, we do newcons = AddRelationNewConstraints(rel, NIL, list_make1(copyObject(constr)), recursing || is_readd, /* allow_merge */ !recursing, /* is_local */ is_readd, /* is_internal */ NULL); /* queryString not available * here */ pg_constraint manual <<<<QUOTE<<< conislocal bool This constraint is defined locally for the relation. Note that a constraint can be locally defined and inherited simultaneously. coninhcount int2 The number of direct inheritance ancestors this constraint has. A constraint with a nonzero number of ancestors cannot be dropped nor renamed. <<<<END OF QUOTE drop table idxpart cascade; create table idxpart (a int) partition by range (a); create table idxpart0 (like idxpart); alter table idxpart0 add primary key (a); alter table idxpart attach partition idxpart0 for values from (0) to (1000); alter table idxpart add primary key (a); alter table idxpart0 DROP CONSTRAINT idxpart0_pkey; alter table idxpart0 DROP CONSTRAINT idxpart0_a_not_null; First DROP CONSTRAINT failed as the doc said, but the second success. but the second DROP CONSTRAINT should fail? Even if you drop success, idxpart0_a_not_null still exists. it also conflicts with the pg_constraint I've quoted above. transformTableLikeClause, expandTableLikeClause can be further simplified when the relation don't have not-null as all like: /* * Reproduce not-null constraints by copying them. This doesn't require * any option to have been given. */ if (tupleDesc->constr && tupleDesc->constr->has_not_null) { lst = RelationGetNotNullConstraints(RelationGetRelid(relation), false); cxt->nnconstraints = list_concat(cxt->nnconstraints, lst); } we can do: create table parent (a text, b int); create table child () inherits (parent); alter table child no inherit parent; so comments in AdjustNotNullInheritance * AdjustNotNullInheritance * Adjust not-null constraints' inhcount/islocal for * ALTER TABLE [NO] INHERITS "ALTER TABLE [NO] INHERITS" should be "ALTER TABLE ALTER COLUMN [NO] INHERITS" ? Also, seems AdjustNotNullInheritance never being called/used? -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-11T09:27:00Z
On Wed, Sep 11, 2024 at 9:11 AM jian he <jian.universality@gmail.com> wrote: > > On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > Hello, here's a v2 of this patch. I have fixed --I think-- all the > > issues you and Tender Wang reported (unless I declined a fix in some > > previous email). > > after applying your changes. in ATExecAddConstraint, ATAddCheckNNConstraint. ATAddCheckNNConstraint(wqueue, tab, rel, newConstraint, recurse, false, is_readd, lockmode); if passed to ATAddCheckNNConstraint rel is a partitioned table. ATAddCheckNNConstraint itself can recurse to create not-null pg_constraint for itself and it's partitions (children table). This is fine as long as we only call ATExecAddConstraint once. but ATExecAddConstraint itself will recurse, it will call the partitioned table and each of the partitions. The first time ATExecAddConstraint with a partitioned table with the following calling chain ATAddCheckNNConstraint-> AddRelationNewConstraints -> AdjustNotNullInheritance1 works fine. the second time ATExecAddConstraint with the partitions ATAddCheckNNConstraint-> AddRelationNewConstraints -> AdjustNotNullInheritance1 AdjustNotNullInheritance1 will make the partitions pg_constraint->coninhcount bigger than 1. for example: drop table if exists idxpart, idxpart0, idxpart1 cascade; create table idxpart (a int) partition by range (a); create table idxpart0 (a int primary key); alter table idxpart attach partition idxpart0 for values from (0) to (1000); alter table idxpart add primary key (a); After the above query pg_constraint->coninhcount of idxpart0_a_not_null becomes 2. but it should be 1 ? -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-11T19:32:16Z
On 2024-Sep-11, jian he wrote: > On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > Hello, here's a v2 of this patch. I have fixed --I think-- all the > > issues you and Tender Wang reported (unless I declined a fix in some > > previous email). > > > > + /* > + * The constraint must appear as inherited in children, so create a > + * modified constraint object to use. > + */ > + constr = copyObject(constr); > + constr->inhcount = 1; > > in ATAddCheckNNConstraint, we don't need the above copyObject call. > because at the beginning of ATAddCheckNNConstraint, we do > newcons = AddRelationNewConstraints(rel, NIL, > list_make1(copyObject(constr)), > recursing || is_readd, /* > allow_merge */ > !recursing, /* is_local */ > is_readd, /* is_internal */ > NULL); /* queryString not available > * here */ I'm disinclined to change this. The purpose of creating a copy at this point is to avoid modifying an object that doesn't belong to us. It doesn't really matter much that we have an additional copy anyway; this isn't in any way performance-critical or memory-intensive. > create table idxpart (a int) partition by range (a); > create table idxpart0 (like idxpart); > alter table idxpart0 add primary key (a); > alter table idxpart attach partition idxpart0 for values from (0) to (1000); > alter table idxpart add primary key (a); > > alter table idxpart0 DROP CONSTRAINT idxpart0_pkey; > alter table idxpart0 DROP CONSTRAINT idxpart0_a_not_null; > > First DROP CONSTRAINT failed as the doc said, > but the second success. > but the second DROP CONSTRAINT should fail? > Even if you drop success, idxpart0_a_not_null still exists. > it also conflicts with the pg_constraint I've quoted above. Hmm, this is because we allow a DROP CONSTRAINT to set conislocal from true to false. So the constraint isn't *actually* dropped. If you try the DROP CONSTRAINT a second time, you'll get an error then. Maybe I should change the order of checks here, so that we forbid doing the conislocal change; that would be more consistent with what we document. I'm undecided about this TBH -- maybe I should reword the documentation you cite in a different way. > transformTableLikeClause, expandTableLikeClause > can be further simplified when the relation don't have not-null as all like: > > /* > * Reproduce not-null constraints by copying them. This doesn't require > * any option to have been given. > */ > if (tupleDesc->constr && tupleDesc->constr->has_not_null) > { > lst = RelationGetNotNullConstraints(RelationGetRelid(relation), false); > cxt->nnconstraints = list_concat(cxt->nnconstraints, lst); > } True. > Also, seems AdjustNotNullInheritance never being called/used? Oh, right, I removed the last callsite recently. I'll remove the function, and rename AdjustNotNullInheritance1 to AdjustNotNullInheritance now that that name is free. Thanks for reviewing! I'll handle your other comment tomorrow. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-12T08:18:57Z
> > On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > > > Hello, here's a v2 of this patch. I have fixed --I think-- all the > > > issues you and Tender Wang reported (unless I declined a fix in some > > > previous email). > > > PREPARE constr_meta (name[]) AS with cte as ( select con.oid as conoid, conrelid::regclass, pc.relkind as relkind, con.coninhcount as inhcnt ,con.conname, con.contype, con.connoinherit as noinherit ,con.conislocal as islocal, pa.attname, pa.attnum from pg_constraint con join pg_class pc on pc.oid = con.conrelid join pg_attribute pa on pa.attrelid = pc.oid and pa.attnum = any(conkey) where con.contype in ('n', 'p', 'c') and pc.relname = ANY ($1) ) select pg_get_constraintdef(conoid), * from cte order by contype, inhcnt, islocal, attnum; The above constr_meta is used to query meta info, nothing fancy. ---exampleA drop table pp1,cc1, cc2; create table pp1 (f1 int); create table cc1 (f2 text, f3 int) inherits (pp1); create table cc2(f4 float) inherits(pp1,cc1); alter table pp1 alter column f1 set not null; execute constr_meta('{pp1,cc1, cc2}'); ---exampleB drop table pp1,cc1, cc2; create table pp1 (f1 int not null); create table cc1 (f2 text, f3 int) inherits (pp1); create table cc2(f4 float) inherits(pp1,cc1); execute constr_meta('{pp1,cc1, cc2}'); Should exampleA and exampleB return same pg_constraint->coninhcount for not-null constraint "cc2_f1_not_null" ? We only have this Synopsis ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL --tests from src/test/regress/sql/inherit.sql CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT); ALTER TABLE inh_nn_parent ALTER a SET NOT NULL; current fail at ATExecSetNotNull ERROR: cannot change NO INHERIT status of NOT NULL constraint "inh_nn_parent_a_not_null" on relation "inh_nn_parent" seems we translate ALTER TABLE inh_nn_parent ALTER a SET NOT NULL; to ALTER TABLE inh_nn_parent ALTER a SET NOT NULL INHERIT but we cannot (syntax error) ALTER TABLE inh_nn_parent ALTER a SET NOT NULL NO INHERIT; In this case, why not make it no-op, this column "a" already NOT NULL. so ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL will change not-null information, no need to consider other not-null related information. /* - * Return the address of the modified column. If the column was already NOT - * NULL, InvalidObjectAddress is returned. + * ALTER TABLE ALTER COLUMN SET NOT NULL + * + * Add a not-null constraint to a single table and its children. Returns + * the address of the constraint added to the parent relation, if one gets + * added, or InvalidObjectAddress otherwise. + * + * We must recurse to child tables during execution, rather than using + * ALTER TABLE's normal prep-time recursion. */ static ObjectAddress -ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, - const char *colName, LOCKMODE lockmode) +ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName, + bool recurse, bool recursing, List **readyRels, + LOCKMODE lockmode) you introduced two boolean "recurse", "recursing", don't have enough explanation. That is too much to comprehend. " * We must recurse to child tables during execution, rather than using " * ALTER TABLE's normal prep-time recursion. What does this sentence mean for these two boolean "recurse", "recursing"? Finally, I did some cosmetic changes, also improved error message in MergeConstraintsIntoExisting -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-12T10:41:49Z
On 2024-Sep-12, jian he wrote: > ---exampleA > drop table pp1,cc1, cc2; > create table pp1 (f1 int); > create table cc1 (f2 text, f3 int) inherits (pp1); > create table cc2(f4 float) inherits(pp1,cc1); > alter table pp1 alter column f1 set not null; > execute constr_meta('{pp1,cc1, cc2}'); > > ---exampleB > drop table pp1,cc1, cc2; > create table pp1 (f1 int not null); > create table cc1 (f2 text, f3 int) inherits (pp1); > create table cc2(f4 float) inherits(pp1,cc1); > execute constr_meta('{pp1,cc1, cc2}'); > > Should exampleA and exampleB > return same pg_constraint->coninhcount > for not-null constraint "cc2_f1_not_null" > ? Yes, they should be identical. In this case example A is in the wrong, the constraint in cc2 should have inhcount=2 (which example B has) and it has inhcount=1. This becomes obvious if you do ALTER TABLE NO INHERIT of both parents -- in example A, it fails the second one with ERROR: relation 43823 has non-inherited constraint "cc2_f1_not_null" because the inhcount was set wrong by SET NOT NULL. Will fix. (I think the culprit is the "readyRels" stuff I had added -- I should nuke that and add a CommandCounterIncrement in the right spot ...) > We only have this Synopsis > ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL Yeah, this syntax is intended to add a "normal" not-null constraint, i.e. one that inherits. > --tests from src/test/regress/sql/inherit.sql > CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT); > ALTER TABLE inh_nn_parent ALTER a SET NOT NULL; > current fail at ATExecSetNotNull > ERROR: cannot change NO INHERIT status of NOT NULL constraint > "inh_nn_parent_a_not_null" on relation "inh_nn_parent" This is correct, because here you want a normal not-null constraint but the table already has the weird ones that don't inherit. > seems we translate > ALTER TABLE inh_nn_parent ALTER a SET NOT NULL; > to > ALTER TABLE inh_nn_parent ALTER a SET NOT NULL INHERIT Well, we don't "translate" it as such. It's just what's normal. > but we cannot (syntax error) > ALTER TABLE inh_nn_parent ALTER a SET NOT NULL NO INHERIT; I don't feel a need to support this syntax. You can do with with the ADD CONSTRAINT syntax if you need it. > /* > - * Return the address of the modified column. If the column was already NOT > - * NULL, InvalidObjectAddress is returned. > + * ALTER TABLE ALTER COLUMN SET NOT NULL > + * > + * Add a not-null constraint to a single table and its children. Returns > + * the address of the constraint added to the parent relation, if one gets > + * added, or InvalidObjectAddress otherwise. > + * > + * We must recurse to child tables during execution, rather than using > + * ALTER TABLE's normal prep-time recursion. > */ > static ObjectAddress > -ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, > - const char *colName, LOCKMODE lockmode) > +ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName, > + bool recurse, bool recursing, List **readyRels, > + LOCKMODE lockmode) > > you introduced two boolean "recurse", "recursing", don't have enough > explanation. > That is too much to comprehend. Apologies. I think it's a well-established pattern in tablecmds.c. "bool recurse" is for the caller (ATRewriteCatalogs) to request recursion. "bool recursing" is for the function informing itself that it is calling itself recursively, i.e. "I'm already recursing". This is mostly (?) used to skip things like permission checks. > " * We must recurse to child tables during execution, rather than using > " * ALTER TABLE's normal prep-time recursion. > What does this sentence mean for these two boolean "recurse", "recursing"? Here "recurse during execution" means ALTER TABLE's phase 2, that is, ATRewriteCatalogs (which means some ATExecFoo function needs to implement recursion internally), and "normal prep-time recursion" means the recursion set up by phase 1 (ATPrepCmd), which creates separate AlterTableCmd nodes for each child table. See the comments for AlterTable and the code for ATController. > Finally, I did some cosmetic changes, also improved error message > in MergeConstraintsIntoExisting Thanks, will incorporate. -- Álvaro Herrera PostgreSQL Developer — 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 -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-16T17:47:09Z
Sadly, there were some other time-wasting events that I failed to consider, but here's now v3 which has fixed (AFAICS) all the problems you reported. On 2024-Sep-11, jian he wrote: > after applying your changes. > > in ATExecAddConstraint, ATAddCheckNNConstraint. > ATAddCheckNNConstraint(wqueue, tab, rel, > newConstraint, recurse, false, is_readd, > lockmode); > if passed to ATAddCheckNNConstraint rel is a partitioned table. > ATAddCheckNNConstraint itself can recurse to create not-null pg_constraint > for itself and it's partitions (children table). > This is fine as long as we only call ATExecAddConstraint once. > > but ATExecAddConstraint itself will recurse, it will call > the partitioned table and each of the partitions. Yeah, this is because ATPrepAddPrimaryKey was queueing SetNotNull nodes for each column on each children, which is repetitive and causes the problem you see. That was a leftover from the previous way we handled PKs; we no longer need it to work that way. I have changed it so that it queues one constraint addition per column, on the same table that receives the PK. It now works correctly as far as I can tell. Sadly, there's one more pg_dump issue, which causes the pg_upgrade tests to fail. The problem is that if you have this sequence (taken from constraints.sql): CREATE TABLE notnull_tbl4 (a INTEGER PRIMARY KEY INITIALLY DEFERRED); CREATE TABLE notnull_tbl4_cld2 (PRIMARY KEY (a) DEFERRABLE) INHERITS (notnull_tbl4); this is dumped by pg_dump in this other way: CREATE TABLE public.notnull_tbl4 (a integer NOT NULL); CREATE TABLE public.notnull_tbl4_cld2 () INHERITS (public.notnull_tbl4); ALTER TABLE ONLY public.notnull_tbl4_cld2 ADD CONSTRAINT notnull_tbl4_cld2_pkey PRIMARY KEY (a) DEFERRABLE; ALTER TABLE ONLY public.notnull_tbl4 ADD CONSTRAINT notnull_tbl4_pkey PRIMARY KEY (a) DEFERRABLE INITIALLY DEFERRED; This is almost exactly the same, except that the PK for notnull_tbl4_cld2 is created in a separate command ... and IIUC this causes the not-null constraint to obtain a different name, or a different inheritance characteristic, and then from the restored-by-pg_upgrade database, it's dumped by pg_dump separately. This is what causes the pg_upgrade test to fail. Anyway, this made me realize that there is a more general problem, to wit, that pg_dump is not dumping not-null constraint names correctly -- sometimes they just not dumped, which is Not Good. I'll have to look into that once more. (Also: there are still a few additional test stanzas in regress/ that ought to be removed; also, I haven't re-tested sepgsql, so it's probably broken ATM.) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-19T03:30:00Z
On Tue, Sep 17, 2024 at 1:47 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > still digging inheritance related issues. drop table if exists pp1,cc1, cc2; create table pp1 (f1 int, constraint nn check (f1 > 1)); create table cc1 (f2 text, f3 int ) inherits (pp1); create table cc2(f4 float, constraint nn check (f1 > 1)) inherits(pp1,cc1); execute constr_meta('{pp1,cc1, cc2}'); alter table only cc2 drop constraint nn; ERROR: cannot drop inherited constraint "nn" of relation "cc2" So: drop table if exists pp1,cc1, cc2; create table pp1 (f1 int not null); create table cc1 (f2 text, f3 int not null no inherit) inherits (pp1); create table cc2(f4 float, f1 int not null) inherits(pp1,cc1); execute constr_meta('{pp1,cc1, cc2}'); alter table only cc2 drop constraint cc2_f1_not_null; Last "alter table only cc2" should fail? because it violates catalog-pg-constraint coninhcount description: "The number of direct inheritance ancestors this constraint has. A constraint with a nonzero number of ancestors cannot be dropped nor renamed." also alter table only cc2 drop constraint cc2_f1_not_null; success executed. some pg_constraint attribute info may change. but constraint cc2_f1_not_null still exists. -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-19T06:40:12Z
still based on v3. in src/sgml/html/ddl-partitioning.html <<<QUOTE<< Both CHECK and NOT NULL constraints of a partitioned table are always inherited by all its partitions. CHECK constraints that are marked NO INHERIT are not allowed to be created on partitioned tables. You cannot drop a NOT NULL constraint on a partition's column if the same constraint is present in the parent table. <<<QUOTE<< we can change "CHECK constraints that are marked NO INHERIT are not allowed to be created on partitioned tables." to "CHECK and NOT NULL constraints that are marked NO INHERIT are not allowed to be created on partitioned tables." in sql-altertable.html we have: <<<QUOTE<< ATTACH PARTITION partition_name { FOR VALUES partition_bound_spec | DEFAULT } If any of the CHECK constraints of the table being attached are marked NO INHERIT, the command will fail; such constraints must be recreated without the NO INHERIT clause. <<<QUOTE<< create table idxpart (a int constraint nn not null) partition by range (a); create table idxpart0 (a int constraint nn not null no inherit); alter table idxpart attach partition idxpart0 for values from (0) to (1000); In the above sql query case, we changed a constraint ("nn" on idxpart0) connoinherit attribute after ATTACH PARTITION. (connoinherit from true to false) Do we need extra sentences to explain it? here not-null constraint behavior seems to divert from CHECK constraint. drop table if exists idxpart, idxpart0, idxpart1 cascade; create table idxpart (a int) partition by range (a); create table idxpart0 (a int primary key); alter table idxpart attach partition idxpart0 for values from (0) to (1000); alter table idxpart alter column a set not null; alter table idxpart0 alter column a drop not null; alter table idxpart0 drop constraint idxpart0_a_not_null; "alter table idxpart0 alter column a drop not null;" is logically equivalent to "alter table idxpart0 drop constraint idxpart0_a_not_null;" the first one (alter column) ERROR out, the second success. the second "drop constraint" should also ERROR out? since it violates the sentence in ddl-partitioning.html "You cannot drop a NOT NULL constraint on a partition's column if the same constraint is present in the parent table." -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-19T08:26:00Z
On 2024-Sep-19, jian he wrote: > still based on v3. > in src/sgml/html/ddl-partitioning.html > <<<QUOTE<< > Both CHECK and NOT NULL constraints of a partitioned table are always > inherited by all its partitions. > CHECK constraints that are marked NO INHERIT are not allowed to be > created on partitioned tables. > You cannot drop a NOT NULL constraint on a partition's column if the > same constraint is present in the parent table. > <<<QUOTE<< > we can change > "CHECK constraints that are marked NO INHERIT are not allowed to be > created on partitioned tables." > to > "CHECK and NOT NULL constraints that are marked NO INHERIT are not > allowed to be created on partitioned tables." Right. Your proposed text is correct but sounds a bit repetitive with the phrase just prior, and also the next one about inability to drop a NOT NULL applies equally to CHECK constraints; so I modified the whole paragraph to this: Both <literal>CHECK</literal> and <literal>NOT NULL</literal> constraints of a partitioned table are always inherited by all its partitions; it is not allowed to create <literal>NO INHERIT<literal> constraints of those types. You cannot drop a constraint of those types if the same constraint is present in the parent table. > in sql-altertable.html we have: > <<<QUOTE<< > ATTACH PARTITION partition_name { FOR VALUES partition_bound_spec | DEFAULT } > If any of the CHECK constraints of the table being attached are marked > NO INHERIT, the command will fail; such constraints must be recreated > without the NO INHERIT clause. > <<<QUOTE<< > > create table idxpart (a int constraint nn not null) partition by range (a); > create table idxpart0 (a int constraint nn not null no inherit); > alter table idxpart attach partition idxpart0 for values from (0) to (1000); > > In the above sql query case, > we changed a constraint ("nn" on idxpart0) connoinherit attribute > after ATTACH PARTITION. > (connoinherit from true to false) > Do we need extra sentences to explain it? > here not-null constraint behavior seems to divert from CHECK constraint. Ah, yeah, the docs are misleading: we do allow these constraints to mutate from NO INHERIT to INHERIT. There's no danger in this, because such a table cannot have children: no inheritance children (because inheritance-parent tables cannot be partitions) and no partitions either, because partitioned tables are not allowed to have NOT NULL NO INHERIT constraints. So this can only happen on a standalone table, and thus changing the existing not-null constraint from NO INHERIT to normal does no harm. I think we could make CHECK behave the same way on this point; but in the meantime, I propose this text: If any of the <literal>CHECK</literal> constraints of the table being attached are marked <literal>NO INHERIT</literal>, the command will fail; such constraints must be recreated without the <literal>NO INHERIT</literal> clause. By contrast, a <literal>NOT NULL</literal> constraint that was created as <literal>NO INHERIT</literal> will be changed to a normal inheriting one during attach. > drop table if exists idxpart, idxpart0, idxpart1 cascade; > create table idxpart (a int) partition by range (a); > create table idxpart0 (a int primary key); > alter table idxpart attach partition idxpart0 for values from (0) to (1000); > alter table idxpart alter column a set not null; > alter table idxpart0 alter column a drop not null; > alter table idxpart0 drop constraint idxpart0_a_not_null; > > "alter table idxpart0 alter column a drop not null;" > is logically equivalent to > "alter table idxpart0 drop constraint idxpart0_a_not_null;" > > the first one (alter column) ERROR out, > the second success. > the second "drop constraint" should also ERROR out? > since it violates the sentence in ddl-partitioning.html > "You cannot drop a NOT NULL constraint on a partition's column if the > same constraint is present in the parent table." Yeah, I modified this code already a few days ago, and now it does error out like this ERROR: cannot drop inherited constraint "idxpart0_a_not_null" of relation "idxpart0" Anyway, as I mentioned back then, the DROP CONSTRAINT didn't _actually_ remove the constraint; it only marked the constraint as no longer locally defined (conislocal=false), which had no practical effect other than changing the representation during pg_dump. Even detaching the partition after having "dropped" the constraint would make the not-null constraint appear again as coninhcount=0,conislocal=true rather than drop it. Speaking of pg_dump, I'm still on the nightmarish trip to get it to behave correctly for all cases (esp. for pg_upgrade). It seems I tripped up on my own code from the previous round, having under-commented and misunderstood it :-( -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "The eagle never lost so much time, as when he submitted to learn of the crow." (William Blake) -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-19T09:26:59Z
On Thu, Sep 19, 2024 at 4:26 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > drop table if exists idxpart, idxpart0, idxpart1 cascade; > > create table idxpart (a int) partition by range (a); > > create table idxpart0 (a int primary key); > > alter table idxpart attach partition idxpart0 for values from (0) to (1000); > > alter table idxpart alter column a set not null; > > alter table idxpart0 alter column a drop not null; > > alter table idxpart0 drop constraint idxpart0_a_not_null; > > > > "alter table idxpart0 alter column a drop not null;" > > is logically equivalent to > > "alter table idxpart0 drop constraint idxpart0_a_not_null;" > > > > the first one (alter column) ERROR out, > > the second success. > > the second "drop constraint" should also ERROR out? > > since it violates the sentence in ddl-partitioning.html > > "You cannot drop a NOT NULL constraint on a partition's column if the > > same constraint is present in the parent table." > > Yeah, I modified this code already a few days ago, and now it does error > out like this > > ERROR: cannot drop inherited constraint "idxpart0_a_not_null" of relation "idxpart0" > > Anyway, as I mentioned back then, the DROP CONSTRAINT didn't _actually_ > remove the constraint; it only marked the constraint as no longer > locally defined (conislocal=false), which had no practical effect other > than changing the representation during pg_dump. Even detaching the > partition after having "dropped" the constraint would make the not-null > constraint appear again as coninhcount=0,conislocal=true rather than > drop it. > funny. as the previously sql example, if you execute "alter table idxpart0 drop constraint idxpart0_a_not_null;" again then ERROR: cannot drop inherited constraint "idxpart0_a_not_null" of relation "idxpart0" I am not sure if that's logically OK or if the user can deduce the logic from the manual. like, the first time you use "alter table drop constraint" to drop a constraint, the constraint is not totally dropped, the second time you execute it again the constraint cannot be dropped directly. i think the issue is the changes we did in dropconstraint_internal in dropconstraint_internal, we have: ----------- if (con->contype == CONSTRAINT_NOTNULL && con->conislocal && con->coninhcount > 0) { HeapTuple copytup; copytup = heap_copytuple(constraintTup); con = (Form_pg_constraint) GETSTRUCT(copytup); con->conislocal = false; CatalogTupleUpdate(conrel, ©tup->t_self, copytup); ObjectAddressSet(conobj, ConstraintRelationId, con->oid); CommandCounterIncrement(); table_close(conrel, RowExclusiveLock); return conobj; } /* Don't allow drop of inherited constraints */ if (con->coninhcount > 0 && !recursing) ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("cannot drop inherited constraint \"%s\" of relation \"%s\"", constrName, RelationGetRelationName(rel)))); ----------- comments in dropconstraint_internal "* Reset pg_constraint.attnotnull, if this is a not-null constraint." should be "pg_attribute.attnotnull" also, we don't have tests for not-null constraint similar to check constraint tests on src/test/regress/sql/alter_table.sql (line 2067 to line 2073) -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-20T03:34:15Z
another bug. I will dig later, just want to share it first. minimum producer: drop table if exists pp1,cc1, cc2,cc3; create table pp1 (f1 int ); create table cc1 () inherits (pp1); create table cc2() inherits(pp1,cc1); create table cc3() inherits(pp1,cc1,cc2); alter table pp1 alter f1 set not null; ERROR: tuple already updated by self
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2024-09-20T04:14:12Z
jian he <jian.universality@gmail.com> 于2024年9月20日周五 11:34写道： > another bug. > I will dig later, just want to share it first. > > minimum producer: > drop table if exists pp1,cc1, cc2,cc3; > create table pp1 (f1 int ); > create table cc1 () inherits (pp1); > create table cc2() inherits(pp1,cc1); > create table cc3() inherits(pp1,cc1,cc2); > > alter table pp1 alter f1 set not null; > ERROR: tuple already updated by self > I guess some place needs call CommandCounterIncrement(). -- Thanks, Tender Wang
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2024-09-20T06:31:09Z
By the way, the v3 failed applying on Head(d35e293878) git am v3-0001-Catalog-not-null-constraints.patch Applying: Catalog not-null constraints error: patch failed: doc/src/sgml/ref/create_table.sgml:77 error: doc/src/sgml/ref/create_table.sgml: patch does not apply error: patch failed: src/backend/commands/tablecmds.c:4834 error: src/backend/commands/tablecmds.c: patch does not apply error: patch failed: src/backend/parser/gram.y:4141 error: src/backend/parser/gram.y: patch does not apply error: patch failed: src/backend/parser/parse_utilcmd.c:2385 error: src/backend/parser/parse_utilcmd.c: patch does not apply Patch failed at 0001 Catalog not-null constraints Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年9月17日周二 01:47写道： > Sadly, there were some other time-wasting events that I failed to > consider, but here's now v3 which has fixed (AFAICS) all the problems > you reported. > > On 2024-Sep-11, jian he wrote: > > > after applying your changes. > > > > in ATExecAddConstraint, ATAddCheckNNConstraint. > > ATAddCheckNNConstraint(wqueue, tab, rel, > > newConstraint, recurse, false, is_readd, > > lockmode); > > if passed to ATAddCheckNNConstraint rel is a partitioned table. > > ATAddCheckNNConstraint itself can recurse to create not-null > pg_constraint > > for itself and it's partitions (children table). > > This is fine as long as we only call ATExecAddConstraint once. > > > > but ATExecAddConstraint itself will recurse, it will call > > the partitioned table and each of the partitions. > > Yeah, this is because ATPrepAddPrimaryKey was queueing SetNotNull nodes > for each column on each children, which is repetitive and causes the > problem you see. That was a leftover from the previous way we handled > PKs; we no longer need it to work that way. I have changed it so that > it queues one constraint addition per column, on the same table that > receives the PK. It now works correctly as far as I can tell. > > Sadly, there's one more pg_dump issue, which causes the pg_upgrade tests > to fail. The problem is that if you have this sequence (taken from > constraints.sql): > > CREATE TABLE notnull_tbl4 (a INTEGER PRIMARY KEY INITIALLY DEFERRED); > CREATE TABLE notnull_tbl4_cld2 (PRIMARY KEY (a) DEFERRABLE) INHERITS > (notnull_tbl4); > > this is dumped by pg_dump in this other way: > > CREATE TABLE public.notnull_tbl4 (a integer NOT NULL); > CREATE TABLE public.notnull_tbl4_cld2 () INHERITS (public.notnull_tbl4); > ALTER TABLE ONLY public.notnull_tbl4_cld2 ADD CONSTRAINT > notnull_tbl4_cld2_pkey PRIMARY KEY (a) DEFERRABLE; > ALTER TABLE ONLY public.notnull_tbl4 ADD CONSTRAINT notnull_tbl4_pkey > PRIMARY KEY (a) DEFERRABLE INITIALLY DEFERRED; > > This is almost exactly the same, except that the PK for > notnull_tbl4_cld2 is created in a separate command ... and IIUC this > causes the not-null constraint to obtain a different name, or a > different inheritance characteristic, and then from the > restored-by-pg_upgrade database, it's dumped by pg_dump separately. > This is what causes the pg_upgrade test to fail. > > Anyway, this made me realize that there is a more general problem, to > wit, that pg_dump is not dumping not-null constraint names correctly -- > sometimes they just not dumped, which is Not Good. I'll have to look > into that once more. > > > (Also: there are still a few additional test stanzas in regress/ that > ought to be removed; also, I haven't re-tested sepgsql, so it's probably > broken ATM.) > > -- > Álvaro Herrera PostgreSQL Developer — > https://www.EnterpriseDB.com/ > -- Thanks, Tender Wang
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-20T07:31:13Z
On 2024-Sep-20, Tender Wang wrote: > jian he <jian.universality@gmail.com> 于2024年9月20日周五 11:34写道： > > > another bug. > > I will dig later, just want to share it first. > > > > minimum producer: > > drop table if exists pp1,cc1, cc2,cc3; > > create table pp1 (f1 int ); > > create table cc1 () inherits (pp1); > > create table cc2() inherits(pp1,cc1); > > create table cc3() inherits(pp1,cc1,cc2); > > > > alter table pp1 alter f1 set not null; > > ERROR: tuple already updated by self > > I guess some place needs call CommandCounterIncrement(). Yeah ... this fixes it: diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 579b8075b5..3f66e43b9a 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7877,12 +7877,6 @@ ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName, { List *children; - /* - * Make previous addition visible, in case we process the same - * relation again while chasing down multiple inheritance trees. - */ - CommandCounterIncrement(); - children = find_inheritance_children(RelationGetRelid(rel), lockmode); @@ -7890,6 +7884,8 @@ ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName, { Relation childrel = table_open(childoid, NoLock); + CommandCounterIncrement(); + ATExecSetNotNull(wqueue, childrel, conName, colName, recurse, true, lockmode); table_close(childrel, NoLock); I was trying to save on the number of CCIs that we perform, but it's likely not a wise expenditure of time given that this isn't a very common scenario anyway. (Nobody with thousands of millions of children tables will try to run thousands of commands in a single transaction anyway ... so saving a few increments doesn't make any actual difference. If such people exist, they can show us their use case and we can investigate and fix it then.) -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "This is what I like so much about PostgreSQL. Most of the surprises are of the "oh wow! That's cool" Not the "oh shit!" kind. :)" Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-20T07:32:27Z
On 2024-Sep-20, Tender Wang wrote: > By the way, the v3 failed applying on Head(d35e293878) > git am v3-0001-Catalog-not-null-constraints.patch > Applying: Catalog not-null constraints > error: patch failed: doc/src/sgml/ref/create_table.sgml:77 Yeah, there's a bunch of conflicts in current master. I rebased yesterday but I'm still composing the email for v4. Coming soon. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "En las profundidades de nuestro inconsciente hay una obsesiva necesidad de un universo lógico y coherente. Pero el universo real se halla siempre un paso más allá de la lógica" (Irulan)
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-20T08:27:28Z
about set_attnotnull. we can make set_attnotnull look less recursive. instead of calling find_inheritance_children, let's just one pass, directly call find_all_inheritors overall, I think it would be more intuitive. please check the attached refactored set_attnotnull. regress test passed, i only test regress. I am also beginning to wonder if ATExecSetNotNull inside can also call find_all_inheritors.
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-20T09:31:24Z
> > We only have this Synopsis > > ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL > > Yeah, this syntax is intended to add a "normal" not-null constraint, > i.e. one that inherits. > > > --tests from src/test/regress/sql/inherit.sql > > CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT); > > ALTER TABLE inh_nn_parent ALTER a SET NOT NULL; > > current fail at ATExecSetNotNull > > ERROR: cannot change NO INHERIT status of NOT NULL constraint > > "inh_nn_parent_a_not_null" on relation "inh_nn_parent" > > This is correct, because here you want a normal not-null constraint but > the table already has the weird ones that don't inherit. > i found a case,that in a sense kind of support to make it a no-op. no-op means, if this attribute is already not-null, ALTER column SET NOT NULL; won't have any effect. or maybe there is a bug somewhere. drop table if exists pp1; create table pp1 (f1 int not null no inherit); ALTER TABLE pp1 ALTER f1 SET NOT NULL; ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL; There is no child table, no partition, just a single regular table. so, in this case, with or without ONLY should behave the same? now "ALTER TABLE ONLY" works, "ALTER TABLE" error out. per sql-altertable.html: name The name (optionally schema-qualified) of an existing table to alter. If ONLY is specified before the table name, only that table is altered. If ONLY is not specified, the table and all its descendant tables (if any) are altered. diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml index 93b3f664f2..57c4ecd93a 100644 --- 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> [ NO INHERIT ] | UNIQUE [ NULLS [ NOT ] DISTINCT ] ( <replaceable class="parameter">column_name</replaceable> [, ... ] ) <replaceable class="parameter">index_parameters</replaceable> | PRIMARY KEY ( <replaceable class="parameter">column_name</replaceable> [, ... ] ) <replaceable class="parameter">index_parameters</replaceable> | EXCLUDE [ USING <replaceable class="parameter">index_method</replaceable> ] ( <replaceable class="parameter">exclude_element</replaceable> WITH <replaceable class="parameter">operator</replaceable> [, ... ] ) <replaceable class="parameter">index_parameters</replaceable> [ WHERE ( <replaceable class="parameter">predicate</replaceable> ) ] | we can create table pp1 (f1 int not null no inherit); create table pp1 (f1 int, constraint nn not null f1 no inherit); "NO INHERIT" should be applied for column_constraint and table_constraint? -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-20T12:08:28Z
On 2024-Sep-20, jian he wrote: > about set_attnotnull. > > we can make set_attnotnull look less recursive. > instead of calling find_inheritance_children, > let's just one pass, directly call find_all_inheritors > overall, I think it would be more intuitive. > > please check the attached refactored set_attnotnull. > regress test passed, i only test regress. Hmm, what do we gain from doing this change? It's longer in number of lines of code, and it's not clear to me that it is simpler. > I am also beginning to wonder if ATExecSetNotNull inside can also call > find_all_inheritors. The point of descending levels one by one in ATExecSetNotNull is that we can stop for any child on which a constraint already exists. We don't need to scan any children thereof, which saves work. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Use it up, wear it out, make it do, or do without"
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-20T21:15:19Z
On 2024-Sep-20, Alvaro Herrera wrote: > Yeah, there's a bunch of conflicts in current master. I rebased > yesterday but I'm still composing the email for v4. Coming soon. Okay, so here is v4 with these problems fixed, including correct propagation of constraint names to children tables, which I had inadvertently broken earlier. This one does pass the pg_upgrade tests and as far as I can see pg_dump does all the correct things also. I cleaned up the tests to remove everything that's unneeded, redundant, or testing behavior that no longer exists. I changed the behavior of ALTER TABLE ONLY <parent> ADD PRIMARY KEY, so that it throws error in case a child does not have a NOT NULL constraint on one of the columns, rather than silently creating such a constraint. (This is how `master` currently behaves). I think this is better behavior, because it lets the user decide whether they want to scan the table to create that constraint or not. It's a bit crude at present, because (1) a child could have a NO INHERIT constraint and have further children, which would foil the check (I think changing find_inheritance_children to find_all_inheritors would be sufficient to fix this, but that's only needed in legacy inheritance not partitioning); (2) the error message doesn't have an errcode, and the wording might need work. -- Álvaro Herrera PostgreSQL Developer — 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 -
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2024-09-23T06:40:00Z
Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年9月21日周六 05:15写道： > On 2024-Sep-20, Alvaro Herrera wrote: > > > Yeah, there's a bunch of conflicts in current master. I rebased > > yesterday but I'm still composing the email for v4. Coming soon. > > Okay, so here is v4 with these problems fixed, including correct > propagation of constraint names to children tables, which I had > inadvertently broken earlier. This one does pass the pg_upgrade tests > and as far as I can see pg_dump does all the correct things also. I > cleaned up the tests to remove everything that's unneeded, redundant, or > testing behavior that no longer exists. > > I changed the behavior of ALTER TABLE ONLY <parent> ADD PRIMARY KEY, so > that it throws error in case a child does not have a NOT NULL constraint > on one of the columns, rather than silently creating such a constraint. > (This is how `master` currently behaves). I think this is better > behavior, because it lets the user decide whether they want to scan the > table to create that constraint or not. It's a bit crude at present, > because (1) a child could have a NO INHERIT constraint and have further > children, which would foil the check (I think changing > find_inheritance_children to find_all_inheritors would be sufficient to > fix this, but that's only needed in legacy inheritance not > partitioning); (2) the error message doesn't have an errcode, and the > wording might need work. > The indexing test case in regress failed with v4 patch. alter table only idxpart add primary key (a); -- fail, no not-null constraint -ERROR: column a of table idxpart0 is not marked not null +ERROR: column "a" of table "idxpart0" is not marked NOT NULL It seemed the error message forgot to change. -- Thanks, Tender Wang https://www.openpie.com/
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-24T03:22:00Z
On Sat, Sep 21, 2024 at 5:15 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Okay, so here is v4 with these problems fixed, including correct > propagation of constraint names to children tables, which I had > inadvertently broken earlier. This one does pass the pg_upgrade tests > and as far as I can see pg_dump does all the correct things also. I > cleaned up the tests to remove everything that's unneeded, redundant, or > testing behavior that no longer exists. > in findNotNullConstraintAttnum if (con->contype != CONSTRAINT_NOTNULL) continue; if (!con->convalidated) continue; if con->convalidated is false, then we have a bigger problem? maybe we can change to ERROR to expose/capture potential problems. like: if (con->contype != CONSTRAINT_NOTNULL) continue; if (!con->convalidated) elog(ERROR, "not-null constraint is not validated"); ------<<<<<<<<------------------ HeapTuple findNotNullConstraint(Oid relid, const char *colname) { AttrNumber attnum = get_attnum(relid, colname); return findNotNullConstraintAttnum(relid, attnum); } we can change to HeapTuple findNotNullConstraint(Oid relid, const char *colname) { AttrNumber attnum = get_attnum(relid, colname); if (attnum <= InvalidAttrNumber) return NULL; return findNotNullConstraintAttnum(relid, attnum); } ------<<<<<<<<------------------ sql-createtable.html SECTION: LIKE source_table [ like_option ... ] 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. drop table if exists t, t_1,ssa; create table t(a int, b int, not null a no inherit); create table ssa (like t INCLUDING all); Here create table like won't include no inherit not-null constraint, seems to conflict with the doc? ------<<<<<<<<------------------ drop table if exists t, t_1; create table t(a int primary key, b int, not null a no inherit); create table t_1 () inherits (t); t_1 will inherit the not-null constraint from t, so the syntax "not null a no inherit" information is ignored. other cases: create table t(a int not null, b int, not null a no inherit); create table t(a int not null no inherit, b int, not null a); seems currently, column constraint have not-null constraint, then use it and table constraint (not-null) are ignored. but if column constraint don't have not-null then according to table constraint. -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-24T07:03:00Z
On Fri, Sep 20, 2024 at 8:08 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2024-Sep-20, jian he wrote: > > > about set_attnotnull. > > > > we can make set_attnotnull look less recursive. > > instead of calling find_inheritance_children, > > let's just one pass, directly call find_all_inheritors > > overall, I think it would be more intuitive. > > > > please check the attached refactored set_attnotnull. > > regress test passed, i only test regress. > > Hmm, what do we gain from doing this change? It's longer in number of > lines of code, and it's not clear to me that it is simpler. > static void set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse, LOCKMODE lockmode) { HeapTuple tuple; Form_pg_attribute attForm; bool changed = false; List *all_oids; Relation thisrel; AttrNumber childattno; const char *attrname; CheckAlterTableIsSafe(rel); attrname = get_attname(RelationGetRelid(rel), attnum, false); if (recurse) all_oids = find_all_inheritors(RelationGetRelid(rel), lockmode, NULL); else all_oids = list_make1_int(RelationGetRelid(rel)); foreach_oid(reloid, all_oids) { thisrel = table_open(reloid, NoLock); if (reloid != RelationGetRelid(rel)) CheckAlterTableIsSafe(thisrel); childattno = get_attnum(reloid, attrname); tuple = SearchSysCacheCopyAttNum(reloid, childattno); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for attribute %d of relation %s", attnum, RelationGetRelationName(thisrel)); attForm = (Form_pg_attribute) GETSTRUCT(tuple); if (!attForm->attnotnull) { Relation attr_rel; attr_rel = table_open(AttributeRelationId, RowExclusiveLock); attForm->attnotnull = true; CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple); table_close(attr_rel, RowExclusiveLock); if (wqueue && !NotNullImpliedByRelConstraints(thisrel, attForm)) { AlteredTableInfo *tab; tab = ATGetQueueEntry(wqueue, thisrel); tab->verify_new_notnull = true; } changed = true; } if (changed) CommandCounterIncrement(); changed = false; table_close(thisrel, NoLock); } } What do you think of the above refactor? (I intentionally deleted empty new line) -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-24T09:04:31Z
static Oid StoreRelNotNull(Relation rel, const char *nnname, AttrNumber attnum, bool is_validated, bool is_local, int inhcount, bool is_no_inherit) { Oid constrOid; Assert(attnum > InvalidAttrNumber); constrOid = CreateConstraintEntry(nnname, RelationGetNamespace(rel), CONSTRAINT_NOTNULL, false, false, is_validated .... } is is_validated always true, can we add an Assert on it? in AddRelationNotNullConstraints for (int outerpos = 0; outerpos < list_length(old_notnulls); outerpos++) { } CookedConstraint struct already has "int inhcount;" can we rely on that, rather than using add_inhcount? we can also add an Assert: "Assert(!cooked->is_no_inherit);" I've put these points into a patch, please check the attached. /* * Remember primary key index, if any. We do this only if the index * is valid; but if the table is partitioned, then we do it even if * it's invalid. * * The reason for returning invalid primary keys for foreign tables is * because of pg_dump of NOT NULL constraints, and the fact that PKs * remain marked invalid until the partitions' PKs are attached to it. * If we make rd_pkindex invalid, then the attnotnull flag is reset * after the PK is created, which causes the ALTER INDEX ATTACH * PARTITION to fail with 'column ... is not marked NOT NULL'. With * this, dropconstraint_internal() will believe that the columns must * not have attnotnull reset, so the PKs-on-partitions can be attached * correctly, until finally the PK-on-parent is marked valid. * * Also, this doesn't harm anything, because rd_pkindex is not a * "real" index anyway, but a RELKIND_PARTITIONED_INDEX. */ if (index->indisprimary && (index->indisvalid || relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)) { pkeyIndex = index->indexrelid; pkdeferrable = !index->indimmediate; } The comment (especially paragraph "The reason for returning invalid primary keys") is overwhelming. Can you also add some sql examples into the comments. I guess some sql examples, people can understand it more easily? -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-24T09:43:26Z
On 2024-Sep-24, jian he wrote: > static Oid > StoreRelNotNull(Relation rel, const char *nnname, AttrNumber attnum, > bool is_validated, bool is_local, int inhcount, > bool is_no_inherit) > { > Oid constrOid; > Assert(attnum > InvalidAttrNumber); > constrOid = > CreateConstraintEntry(nnname, > RelationGetNamespace(rel), > CONSTRAINT_NOTNULL, > false, > false, > is_validated > .... > } > is is_validated always true, can we add an Assert on it? Sure. FWIW the reason it's a parameter at all, is that the obvious next patch is to add support for NOT VALID constraints. I don't want to introduce support for NOT VALID immediately with the first patch because I'm sure some wrinkles will appear; but a followup patch will surely follow shortly. > in AddRelationNotNullConstraints > for (int outerpos = 0; outerpos < list_length(old_notnulls); outerpos++) > { > } > CookedConstraint struct already has "int inhcount;" > can we rely on that, rather than using add_inhcount? > we can also add an Assert: "Assert(!cooked->is_no_inherit);" I'm not sure that works, because if your parent has two parents, you don't want to add two -- you still have only one immediate parent. I think the best way to check for correctness is to set up a scenario where you would have that cooked->inhcount=2 (using whatever CREATE TABLEs are necessary) and then see if ALTER TABLE NO INHERIT reach the correct count (0) when all [immediate] parents are detached. But anyway, keep in mind that inhcount keeps the number of _immediate_ parents, not the number of ancestors. > /* > * Remember primary key index, if any. We do this only if the index > * is valid; but if the table is partitioned, then we do it even if > * it's invalid. > * > * The reason for returning invalid primary keys for foreign tables is > * because of pg_dump of NOT NULL constraints, and the fact that PKs > * remain marked invalid until the partitions' PKs are attached to it. > * If we make rd_pkindex invalid, then the attnotnull flag is reset > * after the PK is created, which causes the ALTER INDEX ATTACH > * PARTITION to fail with 'column ... is not marked NOT NULL'. With > * this, dropconstraint_internal() will believe that the columns must > * not have attnotnull reset, so the PKs-on-partitions can be attached > * correctly, until finally the PK-on-parent is marked valid. > * > * Also, this doesn't harm anything, because rd_pkindex is not a > * "real" index anyway, but a RELKIND_PARTITIONED_INDEX. > */ > if (index->indisprimary && > (index->indisvalid || > relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)) > { > pkeyIndex = index->indexrelid; > pkdeferrable = !index->indimmediate; > } > The comment (especially paragraph "The reason for returning invalid > primary keys") is overwhelming. > Can you also add some sql examples into the comments. > I guess some sql examples, people can understand it more easily? Ooh, thanks for catching this -- this comment is a leftover from previous idea that you could have PKs without NOT NULL. I think it mostly needs to be removed, and maybe the whole "if" clause put back to its original form. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "If it is not right, do not do it. If it is not true, do not say it." (Marcus Aurelius, Meditations) -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-24T10:51:54Z
On 2024-Sep-24, jian he wrote: > static void > set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse, > LOCKMODE lockmode) > { > What do you think of the above refactor? > (I intentionally deleted empty new line) Looks nicer ... but you know what? After spending some more time with it, I realized that one caller is dead code (in AttachPartitionEnsureIndexes) and another caller doesn't need to ask for recursion, because it recurses itself (in ATAddCheckNNConstraint). So that leaves us with a grand total of zero callers that need the recursion here ... which means we can simplify it to the case that it only examines a single relation and never recurses. So I've stripped it down to its bare minimum: /* * Helper to set pg_attribute.attnotnull if it isn't set, and to tell phase 3 * to verify it. * * When called to alter an existing table, 'wqueue' must be given so that we * can queue a check that existing tuples pass the constraint. When called * from table creation, 'wqueue' should be passed as NULL. */ static void set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, LOCKMODE lockmode) { Oid reloid = RelationGetRelid(rel); HeapTuple tuple; Form_pg_attribute attForm; CheckAlterTableIsSafe(rel); tuple = SearchSysCacheCopyAttNum(reloid, attnum); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for attribute %d of relation %u", attnum, reloid); attForm = (Form_pg_attribute) GETSTRUCT(tuple); if (!attForm->attnotnull) { Relation attr_rel; attr_rel = table_open(AttributeRelationId, RowExclusiveLock); attForm->attnotnull = true; CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple); if (wqueue && !NotNullImpliedByRelConstraints(rel, attForm)) { AlteredTableInfo *tab; tab = ATGetQueueEntry(wqueue, rel); tab->verify_new_notnull = true; } CommandCounterIncrement(); table_close(attr_rel, RowExclusiveLock); } heap_freetuple(tuple); } -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-24T16:59:46Z
On 2024-Sep-24, jian he wrote: > sql-createtable.html > SECTION: LIKE source_table [ like_option ... ] > 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. > > drop table if exists t, t_1,ssa; > create table t(a int, b int, not null a no inherit); > create table ssa (like t INCLUDING all); > > Here create table like won't include no inherit not-null constraint, > seems to conflict with the doc? Hmm, actually I think this is a bug, because if you have CHECK constraint with NO INHERIT, it will be copied: create table t (a int check (a > 0) no inherit); create table ssa (like t including constraints); 55490 18devel 141626=# \d+ ssa Tabla «public.ssa» Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión │ Almacenamiento> ─────────┼─────────┼──────────────┼─────────┼─────────────┼───────────────> a │ integer │ │ │ │ plain > Restricciones CHECK: "t_a_check" CHECK (a > 0) NO INHERIT Método de acceso: heap It seems that NOT NULL constraint should behave the same as CHECK constraints in this regard, i.e., we should not heed NO INHERIT in this case. I have made these changes and added some tests, and will be posting a v5 shortly. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ <inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell <crab> inflex: you know that "amalgam" means "mixture with mercury", more or less, right? <crab> i.e., "deadly poison" -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-24T19:07:14Z
On 2024-Sep-24, Alvaro Herrera wrote: > I have made these changes and added some tests, and will be posting a v5 > shortly. I ran the coverage report and found a couple of ereports are not covered by any tests. I'm adding those. May add more tomorrow, after looking at the coverage report some more. I should give a try at running Andres' differential coverage report[1] at some point ... [1] https://postgr.es/m/20240414223305.m3i5eju6zylabvln%40awork3.anarazel.de -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-25T08:31:38Z
copy from src/test/regress/sql/index_including.sql -- Unique index and unique constraint CREATE TABLE tbl_include_unique1 (c1 int, c2 int, c3 int, c4 box); INSERT INTO tbl_include_unique1 SELECT x, 2*x, 3*x, box('4,4,4,4') FROM generate_series(1,10) AS x; CREATE UNIQUE INDEX tbl_include_unique1_idx_unique ON tbl_include_unique1 using btree (c1, c2) INCLUDE (c3, c4); ALTER TABLE tbl_include_unique1 add UNIQUE USING INDEX tbl_include_unique1_idx_unique; \d+ tbl_include_unique1 transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) /* Ensure these columns get a NOT NULL constraint */ cxt->nnconstraints = lappend(cxt->nnconstraints, makeNotNullConstraint(makeString(attname))); the above code can only apply when (constraint->contype == CONSTR_UNIQUE ) is false. The above sql example shows that (constraint->contype == CONSTR_UNIQUE ) can be true. drop table if exists idxpart, idxpart0 cascade; create table idxpart (a int) partition by range (a); create table idxpart0 (a int not null); alter table idxpart attach partition idxpart0 for values from (0) to (100); alter table idxpart alter column a set not null; alter table idxpart alter column a drop not null; "alter table idxpart alter column a set not null;" will make idxpart0_a_not_null constraint islocal and inhertited, which is not OK? for partition trees, only the top level/root can be local for not-null constraint? "alter table idxpart alter column a drop not null;" should cascade to idxpart0? <para> However, a column can have at most one explicit not-null constraint. </para> maybe we can add a sentence: "Adding not-null constraints on a column marked as not-null is a no-op." then we can easily explain case like: create table t(a int primary key , b int, constraint nn not null a ); the final not-null constraint name is "t_a_not_null1" /* * Run through the constraints that need to generate an index, and do so. * * For PRIMARY KEY, in addition we set each column's attnotnull flag true. * We do not create a separate not-null constraint, as that would be * redundant: the PRIMARY KEY constraint itself fulfills that role. Other * constraint types don't need any not-null markings. */ the above comments in transformIndexConstraints is wrong and not necessary? "create table t(a int primary key)" we create a primary key and also do create separate a not-null constraint for "t" /* * column is defined in the new table. For PRIMARY KEY, we * can apply the not-null constraint cheaply here. Note that * this isn't effective in ALTER TABLE, unless the column is * being added in the same command. */ in transformIndexConstraint, i am not sure the meaning of the third sentence in above comments i see no error message like ERROR: NOT NULL constraints cannot be marked NOT VALID ERROR: not-null constraints for domains cannot be marked NO INHERIT in regress tests. we can add some in src/test/regress/sql/domain.sql like: create domain d1 as text not null no inherit; create domain d1 as text constraint nn not null no inherit; create domain d1 as text constraint nn not null; ALTER DOMAIN d1 ADD constraint nn not null NOT VALID; drop domain d1; -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-25T12:10:23Z
in ATExecSetNotNull /* * If we find an appropriate constraint, we're almost done, but just * need to change some properties on it: if we're recursing, increment * coninhcount; if not, set conislocal if not already set. */ if (recursing) { conForm->coninhcount++; changed = true; } else if (!conForm->conislocal) { conForm->conislocal = true; changed = true; elog(INFO, "constraint islocal attribute changed"); } if (recursing && !conForm->conislocal) elog(INFO, "should not happenX"); "should not happenX" appeared in regression.diff, but not "constraint islocal attribute changed" Does that mean the IF, ELSE IF logic is not right? in doc/src/sgml/ref/create_table.sgml [ NO INHERIT ] can apply to <replaceable class="parameter">table_constraint</replaceable> and <replaceable class="parameter">column_constraint</replaceable> so we should change create_table.sgml accordingly? -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-25T20:14:53Z
On 2024-Sep-25, jian he wrote: > copy from src/test/regress/sql/index_including.sql > -- Unique index and unique constraint > CREATE TABLE tbl_include_unique1 (c1 int, c2 int, c3 int, c4 box); > INSERT INTO tbl_include_unique1 SELECT x, 2*x, 3*x, box('4,4,4,4') > FROM generate_series(1,10) AS x; > CREATE UNIQUE INDEX tbl_include_unique1_idx_unique ON > tbl_include_unique1 using btree (c1, c2) INCLUDE (c3, c4); > ALTER TABLE tbl_include_unique1 add UNIQUE USING INDEX > tbl_include_unique1_idx_unique; > \d+ tbl_include_unique1 > > transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) > /* Ensure these columns get a NOT NULL constraint */ > cxt->nnconstraints = > lappend(cxt->nnconstraints, > makeNotNullConstraint(makeString(attname))); > the above code can only apply when (constraint->contype == > CONSTR_UNIQUE ) is false. > The above sql example shows that (constraint->contype == CONSTR_UNIQUE > ) can be true. Doh, yeah. Fixed and added a test for this. > drop table if exists idxpart, idxpart0 cascade; > create table idxpart (a int) partition by range (a); > create table idxpart0 (a int not null); > alter table idxpart attach partition idxpart0 for values from (0) to (100); > alter table idxpart alter column a set not null; > alter table idxpart alter column a drop not null; > > "alter table idxpart alter column a set not null;" > will make idxpart0_a_not_null constraint islocal and inhertited, > which is not OK? > for partition trees, only the top level/root can be local for not-null > constraint? > > "alter table idxpart alter column a drop not null;" > should cascade to idxpart0? Hmm, I think this behaves OK. It's valid to have a child with a constraint that the parent doesn't have. And then if the parent acquires one and passes it down to the children, then deleting it from the parent should not leave the child unprotected. This is the whole reason we have the "inhcount/islocal" system, after all. One small glitch here is that detaching a partition (or removing inheritance) does not remove the constraint, even if islocal=false and inhcount reaches 0. Instead, we turn islocal=true, so that the constraint continues to exist. This is a bit weird, but the intent is to preserve properties and give the user an explicit choice; they can still drop the constraint after detaching. Also, columns also work that way: create table parent (a int); create table child () inherits (parent); select attrelid::regclass, attname, attislocal, attinhcount from pg_attribute where attname = 'a'; attrelid │ attname │ attislocal │ attinhcount ──────────┼─────────┼────────────┼───────────── parent │ a │ t │ 0 child │ a │ f │ 1 alter table child no inherit parent; select attrelid::regclass, attname, attislocal, attinhcount from pg_attribute where attname = 'a'; attrelid │ attname │ attislocal │ attinhcount ──────────┼─────────┼────────────┼───────────── parent │ a │ t │ 0 child │ a │ t │ 0 Here the column on child, which didn't have a local definition, becomes a local column during NO INHERIT. > <para> > However, a column can have at most one explicit not-null constraint. > </para> > maybe we can add a sentence: > "Adding not-null constraints on a column marked as not-null is a no-op." > then we can easily explain case like: > create table t(a int primary key , b int, constraint nn not null a ); > the final not-null constraint name is "t_a_not_null1" Yeah, I've been thinking about this in connection with the restriction I just added to forbid two NOT NULLs with differing NO INHERIT flags: we need to preserve a constraint name if it's specified, or raise an error if two different names are specified. This requires a change in AddRelationNotNullConstraints() to propagate a name specified later in the constraint list. This made me realize that transformColumnDefinition() also has a related problem, in that it ignores a subsequent constraint if multiple ones are defined on the same column, such as in create table notnull_tbl2 (a int primary key generated by default as identity constraint foo not null constraint foo not null no inherit); here, the constraint lacks the NO INHERIT flag even though it was specifically requested the second time. > /* > * Run through the constraints that need to generate an index, and do so. > * > * For PRIMARY KEY, in addition we set each column's attnotnull flag true. > * We do not create a separate not-null constraint, as that would be > * redundant: the PRIMARY KEY constraint itself fulfills that role. Other > * constraint types don't need any not-null markings. > */ > the above comments in transformIndexConstraints is wrong > and not necessary? > "create table t(a int primary key)" > we create a primary key and also do create separate a not-null > constraint for "t" I'm going to replace it with "For PRIMARY KEY, we queue not-null constraints for each column." > /* > * column is defined in the new table. For PRIMARY KEY, we > * can apply the not-null constraint cheaply here. Note that > * this isn't effective in ALTER TABLE, unless the column is > * being added in the same command. > */ > in transformIndexConstraint, i am not sure the meaning of the third > sentence in above comments Yeah, this is mostly a preexisting comment (though it was originally talking about tables OF TYPE, which is a completely different thing): create type atype as (a int, b text); create table atable of atype (not null a no inherit); \d+ atable Tabla «public.atable» Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión │ Almacenamiento │ Compresió> ─────────┼─────────┼──────────────┼──────────┼─────────────┼────────────────┼──────────> a │ integer │ │ not null │ │ plain │ > b │ text │ │ │ │ extended │ > Not-null constraints: "atable_a_not_null" NOT NULL "a" NO INHERIT Tabla tipada de tipo: atype Anyway, what this comment means is that if the ALTER TABLE is doing ADD CONSTRAINT on columns that already exist on the table (as opposed to doing it on columns that the same ALTER TABLE command is doing ADD COLUMN for), then "this isn't effective" (ie. it doesn't do anything). In reality, this comment is now wrong, because during ALTER TABLE the NOT NULL constraints are added by ATPrepAddPrimaryKey, which occurs before this code runs, so the column->is_not_null clause is always true and this block is not executed. This code is only used during CREATE TABLE. So the comment needs to be removed, or maybe done this way with an extra assertion: /* + * column is defined in the new table. For CREATE TABLE with + * a PRIMARY KEY, we can apply the not-null constraint cheaply + * here. Note that ALTER TABLE never needs this, because + * those constraints have already been added by + * ATPrepAddPrimaryKey. */ if (constraint->contype == CONSTR_PRIMARY && !column->is_not_null) { + Assert(!cxt->isalter); /* doesn't occur in ALTER TABLE */ column->is_not_null = true; cxt->nnconstraints = lappend(cxt->nnconstraints, makeNotNullConstraint(makeString(key))); } > i see no error message like > ERROR: NOT NULL constraints cannot be marked NOT VALID > ERROR: not-null constraints for domains cannot be marked NO INHERIT > in regress tests. we can add some in src/test/regress/sql/domain.sql > like: > > create domain d1 as text not null no inherit; > create domain d1 as text constraint nn not null no inherit; > create domain d1 as text constraint nn not null; > ALTER DOMAIN d1 ADD constraint nn not null NOT VALID; > drop domain d1; Yeah, I too noticed the lack of tests for not-valid not-null constraints on domains a few days ago. While I was exploring that I noticed that they have some NO INHERIT that seems to be doing nothing (as it should, because what would it actually mean?), so we should remove the gram.y bits that try to handle it. We could add these tests you suggest irrespective of this not-nulls patch in this thread. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-26T07:41:05Z
Please check the attached minor doc changes. make the create_foreign_table.sgml, alter_foreign_table.sgml not-null description consistent with normal tables. change doc/src/sgml/ref/create_table.sgml Parameters section from <term><literal>NOT NULL </literal></term> to <term><literal>NOT NULL [ NO INHERIT ] </literal></term>. in doc/src/sgml/ref/alter_table.sgml Adding a constraint recurses only for <literal>CHECK</literal> constraints that are not marked <literal>NO INHERIT</literal>. This sentence needs to be rephrased to: Adding a constraint recurses for <literal>CHECK</literal> and <literal>NOT NULL </literal> constraints that are not marked <literal>NO INHERIT</literal>. -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-09-26T12:19:46Z
+-- a PK in parent must have a not-null in child that it can mark inherited +create table inh_parent (a int primary key); +create table inh_child (a int primary key); +alter table inh_child inherit inh_parent; -- nope +alter table inh_child alter a set not null; +alter table inh_child inherit inh_parent; -- now it works +ERROR: relation "inh_parent" would be inherited from more than once in src/test/regress/sql/inherit.sql, the comments at the end of the command, seem to conflict with the output? ------------------------------------------------------------------------------- ALTER TABLE ALTER COLUMN SET NOT NULL implicitly means ALTER TABLE ALTER COLUMN SET NOT NULL NO INHERIT. So in ATExecSetNotNull if (conForm->connoinherit && recurse) ereport(ERROR, errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"", NameStr(conForm->conname), RelationGetRelationName(rel))); should be if (conForm->connoinherit) ereport(ERROR, errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"", NameStr(conForm->conname), RelationGetRelationName(rel))); then we can avoid the weird case like below: drop table if exists pp1; create table pp1 (f1 int not null no inherit); ALTER TABLE pp1 ALTER f1 SET NOT NULL; ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL; ------------------------------------------------------------------------------- + else if (rel->rd_rel->relhassubclass && + find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL) + { + ereport(ERROR, + errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("not-null constraint on column \"%s\" must be removed in child tables too", + colName), + errhint("Do not specify the ONLY keyword.")); + } this part in ATExecDropNotNull is not necessary? per alter_table.sql <<<<<<---------->>>>>> -- make sure we can drop a constraint on the parent but it remains on the child CREATE TABLE test_drop_constr_parent (c text CHECK (c IS NOT NULL)); CREATE TABLE test_drop_constr_child () INHERITS (test_drop_constr_parent); ALTER TABLE ONLY test_drop_constr_parent DROP CONSTRAINT "test_drop_constr_parent_c_check"; <<<<<<---------->>>>>> by the same way, we can drop a not-null constraint ONLY on the parent, but it remains on the child. if we not remove the above part then ALTER TABLE ONLY DROP CONSTRAINT will behave differently from ALTER TABLE ONLY ALTER COLUMN DROP NOT NULL. example: drop table pp1,cc1, cc2; create table pp1 (f1 int not null); create table cc1 (f2 text, f3 int) inherits (pp1); create table cc2(f4 float) inherits(pp1,cc1); alter table only pp1 drop constraint pp1_f1_not_null; --works. alter table only pp1 alter column f1 drop not null; --- error, should also work. ------------------------------------------------------------------------------- -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-26T16:53:01Z
On 2024-Sep-26, jian he wrote: > +-- a PK in parent must have a not-null in child that it can mark inherited > +create table inh_parent (a int primary key); > +create table inh_child (a int primary key); > +alter table inh_child inherit inh_parent; -- nope > +alter table inh_child alter a set not null; > +alter table inh_child inherit inh_parent; -- now it works > +ERROR: relation "inh_parent" would be inherited from more than once > in src/test/regress/sql/inherit.sql, the comments at the end of the > command, seem to conflict with the output? Outdated, useless -- removed. > ------------------------------------------------------------------------------- > > ALTER TABLE ALTER COLUMN SET NOT NULL > implicitly means > ALTER TABLE ALTER COLUMN SET NOT NULL NO INHERIT. > > So in ATExecSetNotNull > if (conForm->connoinherit && recurse) > ereport(ERROR, > errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > errmsg("cannot change NO INHERIT status of NOT > NULL constraint \"%s\" on relation \"%s\"", > NameStr(conForm->conname), > RelationGetRelationName(rel))); > should be > if (conForm->connoinherit) > ereport(ERROR, > errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > errmsg("cannot change NO INHERIT status of NOT > NULL constraint \"%s\" on relation \"%s\"", > NameStr(conForm->conname), > RelationGetRelationName(rel))); > > then we can avoid the weird case like below: > > drop table if exists pp1; > create table pp1 (f1 int not null no inherit); > ALTER TABLE pp1 ALTER f1 SET NOT NULL; > ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL; Hmm, I don't understand why you say SET NOT NULL implicitly means SET NOT NULL NO INHERIT. That's definitely not the intention. As I explained earlier, the normal state is that a constraint is inheritable, so if you do SET NOT NULL you want that constraint to be INHERIT. Anyway, I don't see what you see as weird in the commands you list. To me it reacts like this: =# create table pp1 (f1 int not null no inherit); CREATE TABLE =# ALTER TABLE pp1 ALTER f1 SET NOT NULL; ERROR: cannot change NO INHERIT status of NOT NULL constraint "pp1_f1_not_null" on relation "pp1" =# ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL; ALTER TABLE =# \d+ pp1 Tabla «public.pp1» Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión │ Almacenamiento │ Compresión │ Estadísticas │ Descripción ─────────┼─────────┼──────────────┼──────────┼─────────────┼────────────────┼────────────┼──────────────┼───────────── f1 │ integer │ │ not null │ │ plain │ │ │ Not-null constraints: "pp1_f1_not_null" NOT NULL "f1" NO INHERIT Método de acceso: heap which seems to be exactly what we want. > ------------------------------------------------------------------------------- > > + else if (rel->rd_rel->relhassubclass && > + find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL) > + { > + ereport(ERROR, > + errcode(ERRCODE_INVALID_TABLE_DEFINITION), > + errmsg("not-null constraint on column \"%s\" must be removed in > child tables too", > + colName), > + errhint("Do not specify the ONLY keyword.")); > + } > this part in ATExecDropNotNull is not necessary? > > per alter_table.sql > <<<<<<---------->>>>>> > -- make sure we can drop a constraint on the parent but it remains on the child > CREATE TABLE test_drop_constr_parent (c text CHECK (c IS NOT NULL)); > CREATE TABLE test_drop_constr_child () INHERITS (test_drop_constr_parent); > ALTER TABLE ONLY test_drop_constr_parent DROP CONSTRAINT > "test_drop_constr_parent_c_check"; > <<<<<<---------->>>>>> > by the same way, we can drop a not-null constraint ONLY on the parent, > but it remains on the child. > if we not remove the above part then > ALTER TABLE ONLY DROP CONSTRAINT > will behave differently from > ALTER TABLE ONLY ALTER COLUMN DROP NOT NULL. > > example: > drop table pp1,cc1, cc2; > create table pp1 (f1 int not null); > create table cc1 (f2 text, f3 int) inherits (pp1); > create table cc2(f4 float) inherits(pp1,cc1); > > alter table only pp1 drop constraint pp1_f1_not_null; --works. > alter table only pp1 alter column f1 drop not null; --- error, should also work. > ------------------------------------------------------------------------------- Hmm. I'm not sure I like this behavior, but there is precedent in CHECK, and since DROP CONSTRAINT also already works that way, I suppose DROP NOT NULL should do that too. I'll get it changed. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "I must say, I am absolutely impressed with what pgsql's implementation of VALUES allows me to do. It's kind of ridiculous how much "work" goes away in my code. Too bad I can't do this at work (Oracle 8/9)." (Tom Allison) http://archives.postgresql.org/pgsql-general/2007-06/msg00016.php -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-09-27T13:07:09Z
On 2024-Sep-25, jian he wrote: > in ATExecSetNotNull > /* > * If we find an appropriate constraint, we're almost done, but just > * need to change some properties on it: if we're recursing, increment > * coninhcount; if not, set conislocal if not already set. > */ > if (recursing) > { > conForm->coninhcount++; > changed = true; > } > else if (!conForm->conislocal) > { > conForm->conislocal = true; > changed = true; > elog(INFO, "constraint islocal attribute changed"); > } > if (recursing && !conForm->conislocal) > elog(INFO, "should not happenX"); > > > "should not happenX" appeared in regression.diff, but not > "constraint islocal attribute changed" > Does that mean the IF, ELSE IF logic is not right? I don't see a problem here. It means recursing is true, therefore we're down one level already and don't need to set conislocal. Modifying coninhcount is enough. I attach v6 of this patch, including the requisite removal of the ATExecDropNotNull ereport(ERROR) that I mentioned in the other thread[1]. I think I have made fixes for all your comments, though I would like to go back and verify all of them once again, as well as read it in full. [1] https://postgr.es/m/202409261752.nbvlawkxsttf@alvherre.pgsql -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Digital and video cameras have this adjustment and film cameras don't for the same reason dogs and cats lick themselves: because they can." (Ken Rockwell) -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-10-01T08:14:18Z
CREATE TABLE a (aa TEXT); CREATE TEMP TABLE z (b TEXT, UNIQUE(aa, b)) inherits (a); \d+ z Table "pg_temp_0.z" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description --------+------+-----------+----------+---------+----------+-------------+--------------+------------- aa | text | | not null | | extended | | | b | text | | | | extended | | | Indexes: "z_aa_b_key" UNIQUE CONSTRAINT, btree (aa, b) Not-null constraints: "z_aa_not_null" NOT NULL "aa" Inherits: a Access method: heap that means in transformIndexConstraint, the following part only apply to CONSTR_PRIMARY if (strcmp(key, inhname) == 0) { found = true; typid = inhattr->atttypid; cxt->nnconstraints = lappend(cxt->nnconstraints, makeNotNullConstraint(makeString(pstrdup(inhname)))); break; } -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-10-01T08:17:00Z
create table t2 (a int primary key constraint foo not null no inherit); primary key cannot coexist with not-null no inherit? here t2, pg_dump/restore will fail. create table t7 (a int generated by default as identity, constraint foo not null a no inherit, b int); create table t7 (a int generated by default as identity not null no inherit, b int); first fail, second not fail. pg_dump output is: CREATE TABLE public.t7 (a integer NOT NULL NO INHERIT,b integer); ALTER TABLE public.t7 ALTER COLUMN a ADD GENERATED BY DEFAULT AS IDENTITY ( SEQUENCE NAME public.t7_a_seq START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE CACHE 1 ); seems there is a consistency between column_constraint, table_constraint. but in this case, the public.t7 dump is fine. ------------------------------------------------------------------------------- + By contrast, a <literal>NOT NULL</literal> constraint that was created + as <literal>NO INHERIT</literal> will be changed to a normal inheriting + one during attach. Does this sentence don't have corresponding tests? i think you mean something like: drop table if exists idxpart,idxpart0,idxpart1 cascade; create table idxpart (a int not null) partition by list (a); create table idxpart0 (a int constraint foo not null no inherit); alter table idxpart attach partition idxpart0 for values in (0,1,NULL); --------------------------------------------------------------------------------- the pg_dump of ------------- drop table if exists idxpart, idxpart0 cascade; create table idxpart (a int) partition by range (a); create table idxpart0 (a int not null); alter table idxpart attach partition idxpart0 for values from (0) to (100); alter table idxpart alter column a set not null; ------------- is CREATE TABLE public.idxpart (a integer NOT NULL)PARTITION BY RANGE (a); CREATE TABLE public.idxpart0 (a integer NOT NULL); ALTER TABLE ONLY public.idxpart ATTACH PARTITION public.idxpart0 FOR VALUES FROM (0) TO (100); After pu_dump, the attribute conislocal of constraint idxpart0_a_not_null changes from true to false, is this OK for attribute change after pg_dump in this case? -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-10-01T14:54:50Z
ATExecDropInherit /* * If the parent has a primary key, then we decrement counts for all NOT * NULL constraints */ ObjectAddressSet(address, RelationRelationId, RelationGetRelid(parent_rel)); only not-null constraint, with ALTER TABLE NO INHERIT we still decrement counts for not-null constraints. I feel the comment is in the wrong place? please check the attached function MergeConstraintsIntoExisting refactoring 1. make it error check more confined within CONSTRAINT_CHECK and CONSTRAINT_NOTNULL. 2. since get_attname will do system cache search, we can just use Relation->rd_att and TupleDescAttr -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-01T15:20:29Z
On 2024-Oct-01, jian he wrote: > create table t2 (a int primary key constraint foo not null no inherit); > primary key cannot coexist with not-null no inherit? > here t2, pg_dump/restore will fail. Yeah, this needs to throw an error. If you use a table constraint, it does fail as expected: create table notnull_tbl_fail (a int primary key, not null a no inherit); ERROR: conflicting NO INHERIT declaration for not-null constraint on column "a" I missed adding the check in the column constraint case. > create table t7 (a int generated by default as identity, constraint foo not null a no inherit, b int); > create table t7 (a int generated by default as identity not null no inherit, b int); > first fail, second not fail. pg_dump output is: > > CREATE TABLE public.t7 (a integer NOT NULL NO INHERIT,b integer); > ALTER TABLE public.t7 ALTER COLUMN a ADD GENERATED BY DEFAULT AS IDENTITY ( > SEQUENCE NAME public.t7_a_seq > START WITH 1 > INCREMENT BY 1 > NO MINVALUE > NO MAXVALUE > CACHE 1 > ); > seems there is a consistency between column_constraint, table_constraint. > but in this case, the public.t7 dump is fine. Yeah. I don't see any reasonable way to avoid this problem; I mean, we could add something on the Constraint node that's something like "the NO INH flag of this constraint is unspecified and we don't care what it is" (maybe change is_no_inherit from boolean to a tri-state), so that AddRelationNotNullConstraints() allows a NO INHERIT constraint to override a normal one. But this feels too much mechanism for such a fringe feature. I'd rather we live with the wart. I don't think it's very serious anyway. To clarify. What happens in the second case is that we process both the GENERATED and the NOT NULL clauses in a single transformColumnDefinition pass; for GENERATED we see that we need a NOT NULL, and the NOT NULL is right there (albeit with a NO INHERIT clause, but GENERATED doesn't care); so all's well and it works. In the first case, we see these two things separately. On one hand we get GENERATED in transformColumnDefinition, which requires a not-null; it adds one. Separately we have the NOT NULL NO INHERIT, which is processed by transformTableConstraint. When adding this one, it doesn't see that we already have a not-null constraint for the column, so we add it to be processed later. Both constraint requests travel to AddRelationNotNullConstraints, which is the first time we consider them together. By then, there's no way to know that the one in GENERATED would accept being NO INHERIT, we just see that it is not NO INHERIT, so it conflicts with the other one, kaboom. > ------------------------------------------------------------------------------- > + By contrast, a <literal>NOT NULL</literal> constraint that was created > + as <literal>NO INHERIT</literal> will be changed to a normal inheriting > + one during attach. > Does this sentence don't have corresponding tests? > i think you mean something like: > > drop table if exists idxpart,idxpart0,idxpart1 cascade; > create table idxpart (a int not null) partition by list (a); > create table idxpart0 (a int constraint foo not null no inherit); > alter table idxpart attach partition idxpart0 for values in (0,1,NULL); Yeah. We have the equivalent test for attaching an inheritance-child rather than a partition, which is essentially the same thing, in inherit.sql: -- Can turn a NO INHERIT constraint on children into normal, but only if -- there aren't children create table inh_parent (a int not null); create table inh_child (a int not null no inherit); create table inh_grandchild () inherits (inh_child); alter table inh_child inherit inh_parent; -- nope drop table inh_child, inh_grandchild; create table inh_child (a int not null no inherit); alter table inh_child inherit inh_parent; -- now it works I think we could just remove this behavior and nothing of value would be lost. If I recall correctly, handling of NO INHERIT constraints in this way was just added to support the old way of adding PRIMARY KEY, but it feels like a wart that's easily fixed and not worth having, because it's just weird. I mean, what's the motivation for having created the partition (resp. child table) with a NO INHERIT constraint in the first place? > --------------------------------------------------------------------------------- > the pg_dump of > ------------- > drop table if exists idxpart, idxpart0 cascade; > create table idxpart (a int) partition by range (a); > create table idxpart0 (a int not null); > alter table idxpart attach partition idxpart0 for values from (0) to (100); > alter table idxpart alter column a set not null; > ------------- > is > > CREATE TABLE public.idxpart (a integer NOT NULL)PARTITION BY RANGE (a); > CREATE TABLE public.idxpart0 (a integer NOT NULL); > ALTER TABLE ONLY public.idxpart ATTACH PARTITION public.idxpart0 FOR > VALUES FROM (0) TO (100); > > After pu_dump, the attribute conislocal of constraint > idxpart0_a_not_null changes from true to false, > is this OK for attribute change after pg_dump in this case? Good question. I don't think we care about that in practice (the constraint becomes islocal=true if you happen to DETACH; and other than that, the constraint cannot change state in any way). CHECK constraints behave in the same way, and I'm not sure I want to deviate from that. I think there are pg_upgrade woes that would become better if we did though. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-01T20:12:45Z
On 2024-Oct-01, jian he wrote: > ATExecDropInherit > /* > * If the parent has a primary key, then we decrement counts for all NOT > * NULL constraints > */ > ObjectAddressSet(address, RelationRelationId, > RelationGetRelid(parent_rel)); > > only not-null constraint, > with ALTER TABLE NO INHERIT we still decrement counts for not-null constraints. > I feel the comment is in the wrong place? Yeah, I think it's uselessly left over after removing some code that was there. I dropped it. > please check the attached function MergeConstraintsIntoExisting refactoring > 1. make it error check more confined within CONSTRAINT_CHECK and > CONSTRAINT_NOTNULL. I had already rewritten this, as I mentioned in an earlier response; I just made the error check apply regardless of constraint type. Note that in your patch, you left a useless comment in place. > 2. since get_attname will do system cache search, we can just use > Relation->rd_att and TupleDescAttr Hmm, you're right, but I think we can do even better than that: we should use an AttrMap to map the column numbers from parent to child without having the compare column names. This should be much faster. I also re-verified pg_upgrade from earlier releases (I tried 15 and 17). There was a problem whereby we'd bogusly end up with some constraints having islocal=true, because the UPDATE pg_constraint that tries to flip it false doesn't have a constraint name to reference. I made it use 'conkey' in the WHERE for that case; with that, the tests pass. I pushed this branch to my github clone [1] here, and Cirrus is running it here [2]. [1] https://github.com/alvherre/postgres/commits/notnull-init-18/ [2] https://cirrus-ci.com/build/6343292823011328 -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Nunca confiaré en un traidor. Ni siquiera si el traidor lo he creado yo" (Barón Vladimir Harkonnen)
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-10-02T08:41:35Z
On Tue, Oct 1, 2024 at 11:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2024-Oct-01, jian he wrote: > > > create table t2 (a int primary key constraint foo not null no inherit); > > primary key cannot coexist with not-null no inherit? > > here t2, pg_dump/restore will fail. > > Yeah, this needs to throw an error. If you use a table constraint, it > does fail as expected: > > create table notnull_tbl_fail (a int primary key, not null a no inherit); > ERROR: conflicting NO INHERIT declaration for not-null constraint on column "a" > > I missed adding the check in the column constraint case. > after v7, still not bullet-proof. as before, pg_dump/restore will fail for the following: drop table if exists t2, t2_0 create table t2 (a int, b int, c int, constraint foo primary key(a), constraint foo1 not null a no inherit); create table t2_0 (a int constraint foo1 not null no inherit, b int, c int, constraint foo12 primary key(a)); > > + By contrast, a <literal>NOT NULL</literal> constraint that was created > > + as <literal>NO INHERIT</literal> will be changed to a normal inheriting > > + one during attach. > > Does this sentence don't have corresponding tests? > > i think you mean something like: > > > > drop table if exists idxpart,idxpart0,idxpart1 cascade; > > create table idxpart (a int not null) partition by list (a); > > create table idxpart0 (a int constraint foo not null no inherit); > > alter table idxpart attach partition idxpart0 for values in (0,1,NULL); > > I think we could just remove this behavior and nothing of value would be > lost. If I recall correctly, handling of NO INHERIT constraints in this > way was just added to support the old way of adding PRIMARY KEY, but it > feels like a wart that's easily fixed and not worth having, because it's > just weird. I mean, what's the motivation for having created the > partition (resp. child table) with a NO INHERIT constraint in the first > place? > > with your v7 change, you need remove: > > + By contrast, a <literal>NOT NULL</literal> constraint that was created > > + as <literal>NO INHERIT</literal> will be changed to a normal inheriting > > + one during attach. drop table if exists idxpart,idxpart0,idxpart1 cascade; create table idxpart (a int not null) partition by list (a); create table idxpart0 (a int constraint foo not null no inherit); alter table idxpart attach partition idxpart0 for values in (0,1); With V7, we basically cannot change the status of "NO INHERIT". now, we need to drop the not-null constraint foo, recreate a not-null constraint on idxpart0, then attach it to the partitioned table idxpart. imagine a scenario where: At first we didn't know that the NO INHERIT not-null constraint would be attached to a partitioned table. If we want, then we hope attaching it to a partitioned table would be easier. As you can see, v7 will make idxpart0 attach to idxpart quite difficult. --------------------------------------------------------------------------------- drop table if exists inh_parent,inh_child1,inh_child2; create table inh_parent(f1 int); create table inh_child1(f1 int not null); alter table inh_child1 inherit inh_parent; alter table only inh_parent add constraint nn not null f1; alter table only inh_parent alter column f1 set not null; minor inconsistency, i guess. "alter table only inh_parent add constraint nn not null f1;" will fail. But "alter table only inh_parent alter column f1 set not null;" will not fail, but add a "NOT NULL f1 NO INHERIT" constraint. I thought they should behave the same. for partitioned table now both ALTER TABLE ONLY ADD CONSTRAINT NOT NULL, ALTER TABLE ONLY ALTER COLUMN SET NOT NULL will error out. I am fine with partitioned table behavior.
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-02T12:19:35Z
On 2024-Oct-02, jian he wrote: > On Tue, Oct 1, 2024 at 11:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > On 2024-Oct-01, jian he wrote: > > > > > create table t2 (a int primary key constraint foo not null no inherit); > > > primary key cannot coexist with not-null no inherit? > > > here t2, pg_dump/restore will fail. > > > > Yeah, this needs to throw an error. If you use a table constraint, it > > does fail as expected: > > > > create table notnull_tbl_fail (a int primary key, not null a no inherit); > > ERROR: conflicting NO INHERIT declaration for not-null constraint on column "a" > > > > I missed adding the check in the column constraint case. > > > after v7, still not bullet-proof. as before, pg_dump/restore will fail > for the following: > > drop table if exists t2, t2_0 > create table t2 (a int, b int, c int, constraint foo primary key(a), > constraint foo1 not null a no inherit); > create table t2_0 (a int constraint foo1 not null no inherit, b int, c > int, constraint foo12 primary key(a)); Rats. Fixing :-) > drop table if exists idxpart,idxpart0,idxpart1 cascade; > create table idxpart (a int not null) partition by list (a); > create table idxpart0 (a int constraint foo not null no inherit); > alter table idxpart attach partition idxpart0 for values in (0,1); > > With V7, we basically cannot change the status of "NO INHERIT". > now, we need to drop the not-null constraint foo, > recreate a not-null constraint on idxpart0, > then attach it to the partitioned table idxpart. Yeah, that sucks. We'll need a new command ALTER TABLE .. ALTER CONSTRAINT .. INHERIT (exact syntax TBD) which allows you to turn a NO INHERIT constraint into a normal one, to avoid this problem. I suggest we don't hold up this patch for that. > --------------------------------------------------------------------------------- > drop table if exists inh_parent,inh_child1,inh_child2; > create table inh_parent(f1 int); > create table inh_child1(f1 int not null); > alter table inh_child1 inherit inh_parent; > alter table only inh_parent add constraint nn not null f1; > alter table only inh_parent alter column f1 set not null; > > minor inconsistency, i guess. > "alter table only inh_parent add constraint nn not null f1;" > will fail. > But > "alter table only inh_parent alter column f1 set not null;" > will not fail, but add a "NOT NULL f1 NO INHERIT" constraint. > I thought they should behave the same. > > > for partitioned table > now both ALTER TABLE ONLY ADD CONSTRAINT NOT NULL, > ALTER TABLE ONLY ALTER COLUMN SET NOT NULL > will error out. > I am fine with partitioned table behavior. Yeah, this naughty relationship between ONLY and NO INHERIT is bothersome and maybe we need to re-examine it. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ #error "Operator lives in the wrong universe" ("Use of cookies in real-time system development", M. Gleixner, M. Mc Guire) -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-02T14:25:13Z
On 2024-Oct-02, Alvaro Herrera wrote: > On 2024-Oct-02, jian he wrote: > > > On Tue, Oct 1, 2024 at 11:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > after v7, still not bullet-proof. as before, pg_dump/restore will fail > > for the following: > > > > drop table if exists t2, t2_0 > > create table t2 (a int, b int, c int, constraint foo primary key(a), > > constraint foo1 not null a no inherit); > > create table t2_0 (a int constraint foo1 not null no inherit, b int, c > > int, constraint foo12 primary key(a)); > > Rats. Fixing :-) Hmm, I thought this was going to be a five-minute job: I figured I could add a check in DefineIndex() that reads all columns and ensure they're no-inherit. First complication: when creating a partition, we do DefineIndex to create the indexes that the parent table has, before we do AddRelationNotNullConstraints(), so the not-null constraint lookup fails. Easy enough to fix: just move the AddRelationNotNullConstraints call a few lines up. However, things are still not OK because ALTER TABLE ALTER COLUMN TYPE does want to recreate the PK before the not-nulls (per ATPostAlterTypeParse), because AT_PASS_OLD_INDEX comes before AT_PASS_OLD_CONSTR ... and obviously we cannot change that. Another possibility is to add something like AT_PASS_OLD_NOTNULL but that sounds far too ad-hoc. Maybe I need the restriction to appear somewhere else rather than on DefineIndex. Still looking ... -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Los dioses no protegen a los insensatos. Éstos reciben protección de otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-10-03T07:16:43Z
I thought SearchSysCacheCopyAttNum is expensive. Relation->rd_att is enough for checking attnotnull. What do you think of the following refactoring of set_attnotnull? static void set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, LOCKMODE lockmode) { Oid reloid = RelationGetRelid(rel); HeapTuple tuple; Form_pg_attribute attForm; Form_pg_attribute attr; TupleDesc tupleDesc; CheckAlterTableIsSafe(rel); tupleDesc = RelationGetDescr(rel); attr = TupleDescAttr(tupleDesc, attnum - 1); if (attr->attisdropped) return; if (!attr->attnotnull) { Relation attr_rel; attr_rel = table_open(AttributeRelationId, RowExclusiveLock); tuple = SearchSysCacheCopyAttNum(reloid, attnum); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for attribute %d of relation %u", attnum, reloid); attForm = (Form_pg_attribute) GETSTRUCT(tuple); attForm->attnotnull = true; CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple); if (wqueue && !NotNullImpliedByRelConstraints(rel, attForm)) { AlteredTableInfo *tab; tab = ATGetQueueEntry(wqueue, rel); tab->verify_new_notnull = true; } CommandCounterIncrement(); heap_freetuple(tuple); table_close(attr_rel, RowExclusiveLock); } } -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-04T13:08:15Z
On 2024-Oct-03, jian he wrote: > I thought SearchSysCacheCopyAttNum is expensive. > Relation->rd_att is enough for checking attnotnull. > > What do you think of the following refactoring of set_attnotnull? Eh, sure, why not. I mean, I expect that this is going to be barely noticeable performance-wise, but I don't see a reason not to do it this way. The new code in transformIndexConstraint() I added to verify NO INHERIT for columns in the PK[1] is likely to have a more noticeable impact: we have to scan the whole cxt->nnconstraints list for each column of the PK, and strcmp() the column names in order to find matches. I expect this this to be slow (and affect everybody) but I don't see any other reasonable way to do it. A possibility is to add a Constraint member to ColumnDef, and pre-process so that we attach the correct constraint definition to each column definition before invoking transformIndexConstraints in transformCreateStmt; we already do the match there, so it would be a good place for that. Alternatively, turn is_not_null into a tristate (yes, no, "yes but is no inherit"). [1] https://github.com/alvherre/postgres/commit/22e5820e241c744fb36cbc643a4d8d94162c562e -- Álvaro Herrera Breisgau, Deutschland — 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, http://www.paulgraham.com/opensource.html -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-04T13:11:44Z
Here's v8 of this patch. Tests are ok: https://cirrus-ci.com/build/5744512465633280 My next step is to write the complete commit message to explain it in detail and put it to sleep on November's commitfest. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-10-08T07:48:00Z
On Fri, Oct 4, 2024 at 9:11 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Here's v8 of this patch. in AdjustNotNullInheritance if (count > 0) { conform->coninhcount += count; changed = true; } if (is_local) { conform->conislocal = true; changed = true; } change to if (count > 0) { conform->coninhcount += count; changed = true; } if (is_local && !conform->conislocal) { conform->conislocal = true; changed = true; } then we can save some cycles. -------------------<<>>>>------------ MergeConstraintsIntoExisting /* * If the CHECK child constraint is "no inherit" then cannot * merge. */ if (child_con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with non-inherited constraint on child table \"%s\"", NameStr(child_con->conname), RelationGetRelationName(child_rel)))); the comments apply to not-null constraint aslo, so the comments need to be refactored. -------------------<<>>>>------------ in ATExecSetNotNull if (recursing) { conForm->coninhcount++; changed = true; } grep "coninhcount++", I found out pattern: constrForm->coninhcount++; if (constrForm->coninhcount < 0) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); here, maybe we can change to if (recursing) { // conForm->coninhcount++; if (pg_add_s16_overflow(conForm->coninhcount,1, &conForm->coninhcount)) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); changed = true; } -------------------<<>>>>------------ base on your reply at [1] By contrast, a <literal>NOT NULL</literal> constraint that was created as <literal>NO INHERIT</literal> will be changed to a normal inheriting one during attach. these text should removed from section: <<ATTACH PARTITION partition_name { FOR VALUES partition_bound_spec | DEFAULT }>> since currently v8, partition_name not-null no inherit constraint cannot merge with the parent. [1] https://www.postgresql.org/message-id/202410021219.bvjmxzdspif2%40alvherre.pgsql -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-10-10T01:12:16Z
I did some refactoring on transformColumnDefinition since transformColumnDefinition only deals with a single ColumnDef. and serial/primary/identity cannot allow not-null no inherit. We can preliminary iterate through ColumnDef->constraints to check that ColumnDef can allow not-null no inherit or not. if not allowed, then error out at CONSTR_NOTNULL. please check attached. in MergeConstraintsIntoExisting we can while (HeapTupleIsValid(child_tuple = systable_getnext(child_scan))) { Form_pg_constraint child_con = (Form_pg_constraint) GETSTRUCT(child_tuple); HeapTuple child_copy; if (child_con->contype != parent_con->contype) continue; if (child_con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with non-inherited constraint on child table \"%s\"", NameStr(child_con->conname), RelationGetRelationName(child_rel)))); if (parent_con->convalidated && !child_con->convalidated) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with NOT VALID constraint on child table \"%s\"", NameStr(child_con->conname), RelationGetRelationName(child_rel)))); } error out earlier, save some cache search cycle. MergeConstraintsIntoExisting comment says " * XXX See MergeWithExistingConstraint too if you change this code." we actually did change the MergeConstraintsIntoExisting, not change MergeWithExistingConstraint but it seems MergeWithExistingConstraint does not deal with CONSTRAINT_NOTNULL. So I guess the comments are fine. previously, we mentioned adding some domain tests at [1]. but it seems v8, we don't have domain related regression tests. [1] https://www.postgresql.org/message-id/202409252014.74iepgsyuyws%40alvherre.pgsql -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-10-10T06:30:32Z
tricky case: drop table if exists part, part0 cascade; create table part (a int not null) partition by range (a); create table part0 (a int primary key); alter table part attach partition part0 for values from (0) to (1000); alter table ONLY part add primary key(a); alter table ONLY part drop constraint part_a_not_null; -- alter table ONLY part alter column a drop not null; Now we are in a state where a partitioned table have a primary key but doesn't have a not-null constraint for it. select indisunique, indisprimary, indimmediate,indisvalid from pg_index where indexrelid = 'part_pkey'::regclass; shows this primary key index is invalid. but select conname,contype,convalidated from pg_constraint where conname = 'part_pkey'; shows this primary key constraint is valid.
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-10-14T16:06:05Z
On 2024-Oct-10, jian he wrote: > tricky case: > drop table if exists part, part0 cascade; > create table part (a int not null) partition by range (a); > create table part0 (a int primary key); > alter table part attach partition part0 for values from (0) to (1000); > alter table ONLY part add primary key(a); > alter table ONLY part drop constraint part_a_not_null; > -- alter table ONLY part alter column a drop not null; > > > Now we are in a state where a partitioned > table have a primary key but doesn't have a not-null constraint for it. Oh, of course! This means that the comment in RelationGetIndexList (reverted in 6f8bb7c1e961) was incorrect: it was not only about pg_dump's strategy for dumping PKs in partitioned tables, but it was also about protecting those not-nulls under PKs for the same tables. By removing it (which I did when I forward-ported the reversal of the reversal) I removed that protection, and failed to notice because we don't have this test case. Anyway, I put it back, fixing the comment; and I also had to change RelationGetPrimaryKeyIndex API to have an additional "bool deferrable_ok" parameter, so that it knows it can return a deferrable PK. This is needed by the code just added in dropconstraint_internal(). I also added your test case. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ Officer Krupke, what are we to do? Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-11-07T05:59:34Z
sql-altertable.html <varlistentry id="sql-altertable-desc-set-drop-not-null"> <term><literal>SET</literal>/<literal>DROP NOT NULL</literal></term> <listitem> <para> These forms change whether a column is marked to allow null values or to reject null values. </para> <para> If this table is a partition, one cannot perform <literal>DROP NOT NULL</literal> on a column if it is marked <literal>NOT NULL</literal> in the parent table. To drop the <literal>NOT NULL</literal> constraint from all the partitions, perform <literal>DROP NOT NULL</literal> on the parent table. </para> Now this will be slightly inaccurate. drop table if exists part, part0 cascade; create table part (a int not null) partition by range (a); create table part0 (a int not null); alter table part attach partition part0 for values from (0) to (1000); alter table ONLY part0 add primary key(a); alter table part alter column a drop not null; as the example shows that part0 not-null constraint is still there. that means: perform <literal>DROP NOT NULL</literal> on the parent table will not drop the <literal>NOT NULL</literal> constraint from all partitions. so we need rephrase the following sentence: To drop the <literal>NOT NULL</literal> constraint from all the partitions, perform <literal>DROP NOT NULL</literal> on the parent table. to address this kind of corner case? -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-11-07T07:10:33Z
RemoveInheritance if (copy_con->coninhcount <= 0) /* shouldn't happen */ elog(ERROR, "relation %u has non-inherited constraint \"%s\"", RelationGetRelid(child_rel), NameStr(copy_con->conname)); dropconstraint_internal if (childcon->coninhcount <= 0) /* shouldn't happen */ elog(ERROR, "relation %u has non-inherited constraint \"%s\"", childrelid, NameStr(childcon->conname)); RemoveInheritance error triggered (see below), dropconstraint_internal may also. that means the error message should use RelationGetRelationName rather than plain "relation %u"? drop table if exists inh_parent,inh_child1,inh_child2; create table inh_parent(f1 int not null no inherit); create table inh_child1(f1 int not null no inherit); alter table inh_child1 inherit inh_parent; alter table inh_child1 NO INHERIT inh_parent; ERROR: relation 26387 has non-inherited constraint "inh_child1_f1_not_null" sql-altertable.html INHERIT parent_table This form adds the target table as a new child of the specified parent table. Subsequently, queries against the parent will include records of the target table. To be added as a child, the target table must already contain all the same columns as the parent (it could have additional columns, too). The columns must have matching data types, and if they have NOT NULL constraints in the parent then they must also have NOT NULL constraints in the child. " The columns must have matching data types, and if they have NOT NULL constraints in the parent then they must also have NOT NULL constraints in the child. " For the above sentence, we need to add some text to explain NOT NULL constraints, NO INHERIT property for the child table and parent table. ------------------------------------------------ drop table if exists inh_parent,inh_child1,inh_child2; create table inh_parent(f1 int not null no inherit); create table inh_child1(f1 int); alter table inh_child1 inherit inh_parent; alter table inh_child1 NO INHERIT inh_parent; ERROR: 1 unmatched constraints while removing inheritance from "inh_child1" to "inh_parent" now, we cannot "uninherit" inh_child1 from inh_parent? not sure this is expected behavior. -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-07T16:48:24Z
On 2024-Nov-07, jian he wrote: > RemoveInheritance > if (copy_con->coninhcount <= 0) /* shouldn't happen */ > elog(ERROR, "relation %u has non-inherited constraint \"%s\"", > RelationGetRelid(child_rel), NameStr(copy_con->conname)); > dropconstraint_internal > if (childcon->coninhcount <= 0) /* shouldn't happen */ > elog(ERROR, "relation %u has non-inherited constraint \"%s\"", > childrelid, NameStr(childcon->conname)); > > RemoveInheritance error triggered (see below), dropconstraint_internal may also. > that means the error message should use RelationGetRelationName > rather than plain "relation %u"? > > drop table if exists inh_parent,inh_child1,inh_child2; > create table inh_parent(f1 int not null no inherit); > create table inh_child1(f1 int not null no inherit); > alter table inh_child1 inherit inh_parent; > alter table inh_child1 NO INHERIT inh_parent; > ERROR: relation 26387 has non-inherited constraint "inh_child1_f1_not_null" Hmm, no, this is just a code bug: in RemoveInheritance when scanning 'parent' for constraints, we must skip the ones that are NO INHERIT, but weren't. With the bug fixed, the sequence above results in a no-longer-child inh_child1 that still has inh_child1_f1_not_null, and no error is thrown. > sql-altertable.html > INHERIT parent_table > This form adds the target table as a new child of the specified parent table. > Subsequently, queries against the parent will include records of the target > table. To be added as a child, the target table must already contain all the > same columns as the parent (it could have additional columns, too). The columns > must have matching data types, and if they have NOT NULL constraints in the > parent then they must also have NOT NULL constraints in the child. > > " > The columns must have matching data types, and if they have NOT NULL > constraints in the > parent then they must also have NOT NULL constraints in the child. > " > For the above sentence, we need to add some text to explain > NOT NULL constraints, NO INHERIT property > for the child table and parent table. True. I rewrote as follows, moving the whole explanation of constraints together to the same paragraph, rather than talking about some constraints in one paragraph and other constraints in another. The previous approach was better when NOT NULL markings were a property of the column, but now that they are constraints in their own right, this seems better. <term><literal>INHERIT <replaceable class="parameter">parent_table</replaceable></literal></term> <listitem> <para> This form adds the target table as a new child of the specified parent table. Subsequently, queries against the parent will include records of the target table. To be added as a child, the target table must already contain all the same columns as the parent (it could have additional columns, too). The columns must have matching data types. </para> <para> In addition, all <literal>CHECK</literal> and <literal>NOT NULL</literal> constraints on the parent must also exist on the child, except those marked non-inheritable (that is, created with <literal>ALTER TABLE ... ADD CONSTRAINT ... NO INHERIT</literal>), which are ignored. All child-table constraints matched must not be marked non-inheritable. Currently <literal>UNIQUE</literal>, <literal>PRIMARY KEY</literal>, and <literal>FOREIGN KEY</literal> constraints are not considered, but this might change in the future. </para> </listitem> > ------------------------------------------------ > drop table if exists inh_parent,inh_child1,inh_child2; > create table inh_parent(f1 int not null no inherit); > create table inh_child1(f1 int); > alter table inh_child1 inherit inh_parent; > alter table inh_child1 NO INHERIT inh_parent; > ERROR: 1 unmatched constraints while removing inheritance from "inh_child1" to "inh_parent" > > now, we cannot "uninherit" inh_child1 from inh_parent? > not sure this is expected behavior. Yeah, this is the same bug as above. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "In fact, the basic problem with Perl 5's subroutines is that they're not crufty enough, so the cruft leaks out into user-defined code instead, by the Conservation of Cruft Principle." (Larry Wall, Apocalypse 6) -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-07T19:16:32Z
On 2024-Nov-07, jian he wrote: > drop table if exists part, part0 cascade; > create table part (a int not null) partition by range (a); > create table part0 (a int not null); > alter table part attach partition part0 for values from (0) to (1000); > alter table ONLY part0 add primary key(a); > alter table part alter column a drop not null; > > as the example shows that part0 not-null constraint is still there. > that means: > > perform <literal>DROP NOT NULL</literal> on the parent table > will not drop the <literal>NOT NULL</literal> constraint from all partitions. > > so we need rephrase the following sentence: > > To drop the <literal>NOT NULL</literal> constraint from all the > partitions, perform <literal>DROP NOT NULL</literal> on the parent > table. > > to address this kind of corner case? I've been mulling over this and I'm not very sure I want to change the docs over this point. I think it's fine to leave it as is; otherwise it becomes too verbose for a very esoteric corner case that has (what I think is) an obvious explanation: the primary key in the child table requires that the not-null constraint remains, so it does. Do you disagree? Here's v11, which I intended to commit today, but didn't get around to. CI is happy with it, so I'll probably do it tomorrow first thing. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "I'm impressed how quickly you are fixing this obscure issue. I came from MS SQL and it would be hard for me to put into words how much of a better job you all are doing on [PostgreSQL]." Steve Midgley, http://archives.postgresql.org/pgsql-sql/2008-08/msg00000.php
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-11-08T06:44:40Z
> > Here's v11, which I intended to commit today, but didn't get around to. > CI is happy with it, so I'll probably do it tomorrow first thing. > CREATE TABLE notnull_tbl2 (a INTEGER CONSTRAINT blah NOT NULL, b INTEGER CONSTRAINT blah NOT NULL); RelationGetNotNullConstraints, StoreRelNotNull will first create the constraint "blah", then iterate through the second "blah" error out, which is not great for error out cleaning, i believe. so i change AddRelationNotNullConstraints first loop "for (int outerpos = 0; outerpos < list_length(constraints); outerpos++)" we can first validate it through the loop, collect information then do a loop to StoreRelNotNull. while debugging, in RelationGetNotNullConstraints if (cooked) { CookedConstraint *cooked; cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint)); cooked->contype = CONSTR_NOTNULL; cooked->name = pstrdup(NameStr(conForm->conname)); cooked->attnum = colnum; ..... } We missed the assignment of cooked->conoid? MergeConstraintsIntoExisting /* * If the CHECK child constraint is "no inherit" then cannot * merge. */ if (child_con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with non-inherited constraint on child table \"%s\"", NameStr(child_con->conname), RelationGetRelationName(child_rel)))); the above comment can also be hit by not-null constraint, so the comment is wrong? -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-08T09:25:57Z
On 2024-Nov-08, jian he wrote: > > Here's v11, which I intended to commit today, but didn't get around to. > > CI is happy with it, so I'll probably do it tomorrow first thing. > > > > CREATE TABLE notnull_tbl2 (a INTEGER CONSTRAINT blah NOT NULL, b > INTEGER CONSTRAINT blah NOT NULL); > > RelationGetNotNullConstraints, StoreRelNotNull > will first create the constraint "blah", then iterate through the > second "blah" error out, > which is not great for error out cleaning, i believe. I applaud your enthusiasm, but I don't like this change. We have plenty of cases where we abort a command partway through after having created a bunch of catalog rows (we even have comments about such behavior being acceptable); if we wanted to get rid of them all, the code would become far too complicated because it'd have to save state until the last minute, just in case something else threw errors. Your proposed coding seems complicated enough, in fact, given how fringe an error condition it's protecting against. It's not like the user will try to run the command thousands of times "to see if it works next time". One dead catalog row every now and then won't hurt anything. > while debugging, in RelationGetNotNullConstraints > if (cooked) > { > CookedConstraint *cooked; > cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint)); > cooked->contype = CONSTR_NOTNULL; > cooked->name = pstrdup(NameStr(conForm->conname)); > cooked->attnum = colnum; > ..... > } > We missed the assignment of cooked->conoid? Eh, I can't see the OID would ever be useful for anything, but let's put it there just in case some future caller wants it for some reason. > MergeConstraintsIntoExisting > /* > * If the CHECK child constraint is "no inherit" then cannot > * merge. > */ > if (child_con->connoinherit) > ereport(ERROR, > (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > errmsg("constraint \"%s\" conflicts with > non-inherited constraint on child table \"%s\"", > NameStr(child_con->conname), > RelationGetRelationName(child_rel)))); > > the above comment can also be hit by not-null constraint, so the > comment is wrong? Strange ... my copy is fixed already, and in fact I don't see the patch touching this function at all. [ pokes around ] Ah, I changed it two weeks ago: https://github.com/alvherre/postgres/commit/efeed9416b8c7397d61446958d6835e23ec3f0b6 Thanks for looking once more! -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "La grandeza es una experiencia transitoria. Nunca es consistente. Depende en gran parte de la imaginación humana creadora de mitos" (Irulan) -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-11-08T09:32:35Z
> Here's v11, which I intended to commit today, but didn't get around to. > CI is happy with it, so I'll probably do it tomorrow first thing. > v11 still has column_constraint versus table_constraint inconsistency. create table t7 (a int generated by default as identity, constraint foo not null a no inherit, b int); create table t7 (a int generated by default as identity not null no inherit, b int); create table t8 (a serial, constraint foo1 not null a no inherit); create table t8 (a serial not null no inherit, b int); i solved this issue at [1], that patch has one whitespace issue though. what do you think? [1] https://postgr.es/m/CACJufxHgBsJrHyGJ0EQzi9XV+ZSozNDcUJ5sg-f5Wk+dGCYZMg@mail.gmail.com
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-08T13:08:10Z
On 2024-Nov-08, jian he wrote: > > Here's v11, which I intended to commit today, but didn't get around to. > > CI is happy with it, so I'll probably do it tomorrow first thing. > > > v11 still has column_constraint versus table_constraint inconsistency. > > create table t7 (a int generated by default as identity, constraint > foo not null a no inherit, b int); > create table t7 (a int generated by default as identity not null no > inherit, b int); > create table t8 (a serial, constraint foo1 not null a no inherit); > create table t8 (a serial not null no inherit, b int); > > i solved this issue at [1], Ah yeah, that stuff. Your commit message said it was a refactoring so I hadn't paid too much attention to it, but it's in fact not a refactoring at all. I included it with a large comment explaining why we do it that way and that we may want to remove it in the future. I also included these four sentences above in the tests, and pushed it after checking that the CI results are clean. Yesterday I verified that pg_upgrade works with the regression database from 12 onwards. I know the buildfarm uses a different way to do the pg_upgrade test, so there's no way to know if it'll work ahead of time. But we'll see what else the buildfarm has to say now that I pushed it ... -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
-
Re: not null constraints, again
Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-08T16:26:11Z
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > But we'll see what else the buildfarm has to say now that I pushed it ... A lot of the buildfarm is saying adder | 2024-11-08 13:04:39 | ../pgsql/src/backend/catalog/pg_constraint.c:708:37: warning: comparison is always true due to limited range of data type [-Wtype-limits] which evidently is about this: Assert(colnum > 0 && colnum <= MaxAttrNumber); The memcpy right before that doesn't seem like project style either. Most other places that are doing similar things just cast the ARR_DATA_PTR to the right pointer type and dereference it. regards, tom lane
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-09T12:29:12Z
On 2024-Nov-08, Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > But we'll see what else the buildfarm has to say now that I pushed it ... > > A lot of the buildfarm is saying > > adder | 2024-11-08 13:04:39 | ../pgsql/src/backend/catalog/pg_constraint.c:708:37: warning: comparison is always true due to limited range of data type [-Wtype-limits] > > which evidently is about this: > > Assert(colnum > 0 && colnum <= MaxAttrNumber); Hah. > The memcpy right before that doesn't seem like project style either. > Most other places that are doing similar things just cast the > ARR_DATA_PTR to the right pointer type and dereference it. Hmm, yeah, that's easily removed, diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index e953000c01d..043bf7c24dd 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -704,11 +704,7 @@ extractNotNullColumn(HeapTuple constrTup) ARR_DIMS(arr)[0] != 1) elog(ERROR, "conkey is not a 1-D smallint array"); - memcpy(&colnum, ARR_DATA_PTR(arr), sizeof(AttrNumber)); - Assert(colnum > 0 && colnum <= MaxAttrNumber); - - if ((Pointer) arr != DatumGetPointer(adatum)) - pfree(arr); /* free de-toasted copy, if any */ + colnum = ((AttrNumber *) ARR_DATA_PTR(arr))[0]; return colnum; } I notice I cargo-culted a "free de-toasted copy", but I think it's impossible to end up with a toasted datum here, because the column is guaranteed to have only one element, so not a candidate for toasting. But also, if we don't free it (in case somebody does an UPDATE to the catalog with a large array), nothing happens, because memory is going to be released soon anyway, by the error that results by conkey not being one element long. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Si quieres ser creativo, aprende el arte de perder el tiempo"
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-12T10:43:38Z
On 2024-Nov-09, Alvaro Herrera wrote: > I notice I cargo-culted a "free de-toasted copy", but I think it's > impossible to end up with a toasted datum here, because the column is > guaranteed to have only one element, so not a candidate for toasting. > But also, if we don't free it (in case somebody does an UPDATE to the > catalog with a large array), nothing happens, because memory is going to > be released soon anyway, by the error that results by conkey not being > one element long. I found out that my claim that it's impossible to have a detoasted datum was false: because the value is so small, we end up with a short varlena, which does use a separate palloc(). I decided to remove the pfree() anyway, because that makes it easier to return the value we want without having to first assign it away from the chunk we'd pfree. The DDL code mostly doesn't worry too much about memory leaks anyway, and this one is very small. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ “Cuando no hay humildad las personas se degradan” (A. Christie)
-
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-12-04T02:52:29Z
hi. heap_create_with_catalog argument (cooked_constraints): passed as NIL in function {create_toast_table, make_new_heap} passed as list_concat(cookedDefaults,old_constraints) in DefineRelation in DefineRelation we have function call: MergeAttributes heap_create_with_catalog StoreConstraints StoreConstraints second argument: cooked_constraints, some is comes from DefineRelation->MergeAttributes old_constraints: { stmt->tableElts = MergeAttributes(stmt->tableElts, inheritOids, stmt->relation->relpersistence, stmt->partbound != NULL, &old_constraints, &old_notnulls); } My understanding from DefineRelation->MergeAttributes is that old_constraints will only have CHECK constraints. that means heap_create_with_catalog->StoreConstraints StoreConstraints didn't actually handle CONSTR_NOTNULL. heap_create_with_catalog comments also says: * cooked_constraints: list of precooked check constraints and defaults coverage https://coverage.postgresql.org/src/backend/catalog/heap.c.gcov.html also shows StoreConstraints, CONSTR_NOTNULL never being called, which is added by this thread. my question is can we remove StoreConstraints, CONSTR_NOTNULL handling. we have 3 functions {StoreConstraints, AddRelationNotNullConstraints, AddRelationNewConstraints} that will call StoreRelNotNull to store the not-null constraint. That means if we want to bullet proof that something is conflicting with not-null, we need to add code to check all these 3 places. removing StoreConstraints handling not-null seems helpful. also comments in MergeAttributes: * Output arguments: * 'supconstr' receives a list of constraints belonging to the parents, * updated as necessary to be valid for the child. * 'supnotnulls' receives a list of CookedConstraints that corresponds to * constraints coming from inheritance parents. can we be explicit that "supconstr" is only about CHECK constraint, "supnotnulls" is only about NOT-NULL constraint. -
Re: not null constraints, again
jian he <jian.universality@gmail.com> — 2024-12-12T02:47:47Z
On Wed, Dec 4, 2024 at 10:52 AM jian he <jian.universality@gmail.com> wrote: > > hi. > > heap_create_with_catalog argument (cooked_constraints): > passed as NIL in function {create_toast_table, make_new_heap} > passed as list_concat(cookedDefaults,old_constraints) in DefineRelation > > in DefineRelation we have function call: > MergeAttributes > heap_create_with_catalog > StoreConstraints > > StoreConstraints second argument: cooked_constraints, some is comes from > DefineRelation->MergeAttributes old_constraints: > { > stmt->tableElts = MergeAttributes(stmt->tableElts, inheritOids, > stmt->relation->relpersistence, stmt->partbound != NULL, &old_constraints, > &old_notnulls); > } > > My understanding from DefineRelation->MergeAttributes is that old_constraints > will only have CHECK constraints. > that means heap_create_with_catalog->StoreConstraints > StoreConstraints didn't actually handle CONSTR_NOTNULL. > > heap_create_with_catalog comments also says: > * cooked_constraints: list of precooked check constraints and defaults > > coverage https://coverage.postgresql.org/src/backend/catalog/heap.c.gcov.html > also shows StoreConstraints, CONSTR_NOTNULL never being called, > which is added by this thread. > > > my question is can we remove StoreConstraints, CONSTR_NOTNULL handling. > we have 3 functions {StoreConstraints, AddRelationNotNullConstraints, > AddRelationNewConstraints} that will call StoreRelNotNull to store the not-null > constraint. That means if we want to bullet proof that something is conflicting > with not-null, we need to add code to check all these 3 places. > removing StoreConstraints handling not-null seems helpful. > > > also comments in MergeAttributes: > * Output arguments: > * 'supconstr' receives a list of constraints belonging to the parents, > * updated as necessary to be valid for the child. > * 'supnotnulls' receives a list of CookedConstraints that corresponds to > * constraints coming from inheritance parents. > > can we be explicit that "supconstr" is only about CHECK constraint, > "supnotnulls" is > only about NOT-NULL constraint. patch attached. also change comments of heap_create_with_catalog, StoreConstraints, MergeAttributes. so we can clear idea what's kind of constraints we are dealing with in these functions. -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-01-07T15:55:37Z
On 2024-Dec-12, jian he wrote: > patch attached. > > also change comments of heap_create_with_catalog, > StoreConstraints, MergeAttributes. > so we can clear idea what's kind of constraints we are dealing with > in these functions. Great catch! The patch looks good, I have pushed it. Thank you. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)
-
Re: not null constraints, again
Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-14T17:13:30Z
The attached script simply creates two partitioned tables that are connected by a foreign key constraint, then pg_dumps that setup and tries to do a parallel restore. This works up until 14e87ffa5c543b5f30ead7413084c25f7735039f is the first bad commit commit 14e87ffa5c543b5f30ead7413084c25f7735039f Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Date: Fri Nov 8 13:28:48 2024 +0100 Add pg_constraint rows for not-null constraints Since that commit, it fails every time (for me, anyway, on a couple of different machines) with a deadlock error, typically between ALTER ADD PRIMARY KEY and one of the table COPY commands: 2025-04-14 12:54:49.892 EDT [1278062] ERROR: deadlock detected 2025-04-14 12:54:49.892 EDT [1278062] DETAIL: Process 1278062 waits for AccessExclusiveLock on relation 47164 of database 47159; blocked by process 1278059. Process 1278059 waits for AccessShareLock on relation 47160 of database 47159; blocked by process 1278062. Process 1278062: ALTER TABLE ONLY public.parent1 ADD CONSTRAINT parent1_pkey PRIMARY KEY (id); Process 1278059: COPY public.c11 (id, b) FROM stdin; I stumbled across this result after wondering why the repro I'd devised at [1] didn't fail in v17. The patch I propose there seems to prevent this, but I wonder if we shouldn't look closer into why it's failing in the first place. I would not have expected that adding pg_constraint rows implies stronger locks than what ALTER ADD PRIMARY KEY was using before, and I suspect that doing so will cause more problems than just breaking parallel restore. regards, tom lane [1] https://www.postgresql.org/message-id/flat/2045026.1743801143@sss.pgh.pa.us -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-04-14T21:33:03Z
On 2025-Apr-14, Tom Lane wrote: > The patch I propose there seems to prevent this, but I wonder if we > shouldn't look closer into why it's failing in the first place. > I would not have expected that adding pg_constraint rows implies > stronger locks than what ALTER ADD PRIMARY KEY was using before, > and I suspect that doing so will cause more problems than just > breaking parallel restore. I wasn't aware of this side effect. I'll investigate this in more depth. I suspect it might be a bug in the way we run through ALTER TABLE for the primary key. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Los cuentos de hadas no dan al niño su primera idea sobre los monstruos. Lo que le dan es su primera idea de la posible derrota del monstruo." (G. K. Chesterton) -
Re: not null constraints, again
Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-14T21:39:01Z
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > On 2025-Apr-14, Tom Lane wrote: >> I would not have expected that adding pg_constraint rows implies >> stronger locks than what ALTER ADD PRIMARY KEY was using before, >> and I suspect that doing so will cause more problems than just >> breaking parallel restore. > I wasn't aware of this side effect. I'll investigate this in more > depth. I suspect it might be a bug in the way we run through ALTER > TABLE for the primary key. After further thought it occurs to me that it might not be a case of "we get stronger locks", but a case of "we accidentally get a weaker lock earlier and then try to upgrade it", thus creating a possibility of deadlock where before we'd just have blocked till the other statement cleared. Still worthy of being fixed if that's true, though. regards, tom lane
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2025-04-15T03:20:53Z
Tom Lane <tgl@sss.pgh.pa.us> 于2025年4月15日周二 05:39写道： > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > On 2025-Apr-14, Tom Lane wrote: > >> I would not have expected that adding pg_constraint rows implies > >> stronger locks than what ALTER ADD PRIMARY KEY was using before, > >> and I suspect that doing so will cause more problems than just > >> breaking parallel restore. > > > I wasn't aware of this side effect. I'll investigate this in more > > depth. I suspect it might be a bug in the way we run through ALTER > > TABLE for the primary key. > > After further thought it occurs to me that it might not be a case > of "we get stronger locks", but a case of "we accidentally get a > weaker lock earlier and then try to upgrade it", thus creating a > possibility of deadlock where before we'd just have blocked till > the other statement cleared. Still worthy of being fixed if that's > true, though. > I added sleep(1) in the DeadLockReport() before error report to display the status when a deadlock happened. bool continue_sleep = true; do { sleep(1); } while (continue_sleep); ereport(ERROR, (errcode(ERRCODE_T_R_DEADLOCK_DETECTED), errmsg("deadlock detected"), errdetail_internal("%s", clientbuf.data), errdetail_log("%s", logbuf.data), errhint("See server log for query details."))); ubuntu@VM-0-17-ubuntu:/workspace/postgres$ ps -ef|grep postgres ubuntu 2911109 1 0 10:34 ? 00:00:00 /workspace/pgsql/bin/postgres -D ../data ubuntu 2911110 2911109 0 10:34 ? 00:00:00 postgres: io worker 0 ubuntu 2911111 2911109 0 10:34 ? 00:00:00 postgres: io worker 1 ubuntu 2911112 2911109 0 10:34 ? 00:00:00 postgres: io worker 2 ubuntu 2911113 2911109 0 10:34 ? 00:00:00 postgres: checkpointer ubuntu 2911114 2911109 0 10:34 ? 00:00:00 postgres: background writer ubuntu 2911116 2911109 0 10:34 ? 00:00:00 postgres: walwriter ubuntu 2911117 2911109 0 10:34 ? 00:00:00 postgres: autovacuum launcher ubuntu 2911118 2911109 0 10:34 ? 00:00:00 postgres: logical replication launcher ubuntu 2911180 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] COPY waiting ubuntu 2911184 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle ubuntu 2911187 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle ubuntu 2911188 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] ALTER TABLE ubuntu 2911189 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] SELECT waiting ubuntu 2911190 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle ubuntu 2911191 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] TRUNCATE TABLE waiting ubuntu 2911192 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle ubuntu 2911193 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] SELECT waiting ubuntu 2911194 2911109 0 10:34 ? 00:00:00 postgres: ubuntu target [local] idle gdb -p 2911188 // ALTER TABLE ONLY public.parent2 ADD CONSTRAINT parent2_pkey PRIMARY KEY (id); (gdb) bt #0 0x00007f2f6910878a in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7ffc0e560e60, rem=rem@entry=0x7ffc0e560e60) at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78 #1 0x00007f2f6910d677 in __GI___nanosleep (req=req@entry=0x7ffc0e560e60, rem=rem@entry=0x7ffc0e560e60) at ../sysdeps/unix/sysv/linux/nanosleep.c:25 #2 0x00007f2f6910d5ae in __sleep (seconds=0) at ../sysdeps/posix/sleep.c:55 #3 0x0000561cd9386100 in DeadLockReport () at deadlock.c:1136 #4 0x0000561cd9389df8 in LockAcquireExtended (locktag=0x7ffc0e5610b0, lockmode=8, sessionLock=false, dontWait=false, reportMemoryError=true, locallockp=0x7ffc0e5610a8, logLockFailure=false) at lock.c:1232 #5 0x0000561cd93864bc in LockRelationOid (relid=16473, lockmode=8) at lmgr.c:115 #6 0x0000561cd8f21b20 in find_inheritance_children_extended (parentrelId=16463, omit_detached=true, lockmode=8, detached_exist=0x0, detached_xmin=0x0) at pg_inherits.c:213 #7 0x0000561cd8f217c1 in find_inheritance_children (parentrelId=16463, lockmode=8) at pg_inherits.c:60 #8 0x0000561cd904dd73 in ATPrepAddPrimaryKey (wqueue=0x7ffc0e561348, rel=0x7f2f5d7d6240, cmd=0x561cf1d2ee38, recurse=false, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:9463 #9 0x0000561cd9043906 in ATPrepCmd (wqueue=0x7ffc0e561348, rel=0x7f2f5d7d6240, cmd=0x561cf1d2ee38, recurse=false, recursing=false, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:5079 #10 0x0000561cd90432aa in ATController (parsetree=0x561cf1d062e0, rel=0x7f2f5d7d6240, cmds=0x561cf1d06290, recurse=false, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:4871 #11 0x0000561cd9042f3b in AlterTable (stmt=0x561cf1d062e0, lockmode=8, context=0x7ffc0e561540) at tablecmds.c:4533 #12 0x0000561cd93bb7a8 in ProcessUtilitySlow (pstate=0x561cf1d2f9e0, pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:1321 #13 0x0000561cd93bb04e in standard_ProcessUtility (pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:1070 #14 0x0000561cd93b9f4c in ProcessUtility (pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:523 #15 0x0000561cd93b87a9 in PortalRunUtility (portal=0x561cf1d85d90, pstmt=0x561cf1d06390, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:1185 #16 0x0000561cd93b8a52 in PortalRunMulti (portal=0x561cf1d85d90, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d06750, altdest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:1349 #17 0x0000561cd93b7e73 in PortalRun (portal=0x561cf1d85d90, count=9223372036854775807, isTopLevel=true, dest=0x561cf1d06750, altdest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:820 #18 0x0000561cd93b037e in exec_simple_query (query_string=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n") at postgres.c:1274 #19 0x0000561cd93b5b46 in PostgresMain (dbname=0x561cf1d3f580 "target", username=0x561cf1d3f568 "ubuntu") at postgres.c:4771 #20 0x0000561cd93abab7 in BackendMain (startup_data=0x7ffc0e561e50, startup_data_len=24) at backend_startup.c:124 #21 0x0000561cd92abf09 in postmaster_child_launch (child_type=B_BACKEND, child_slot=2, startup_data=0x7ffc0e561e50, startup_data_len=24, client_sock=0x7ffc0e561eb0) at launch_backend.c:290 #22 0x0000561cd92b2946 in BackendStartup (client_sock=0x7ffc0e561eb0) at postmaster.c:3580 #23 0x0000561cd92afeb1 in ServerLoop () at postmaster.c:1702 #24 0x0000561cd92af7a2 in PostmasterMain (argc=3, argv=0x561cf1cffb00) at postmaster.c:1400 #25 0x0000561cd914cf06 in main (argc=3, argv=0x561cf1cffb00) at main.c:227 gdb -p 2911180 // COPY public.c22 (id, ref, b) FROM stdin; (gdb) bt #0 0x00007f2f69148dea in epoll_wait (epfd=5, events=0x561cf1d003a8, maxevents=1, timeout=-1) at ../sysdeps/unix/sysv/linux/epoll_wait.c:30 #1 0x0000561cd938106f in WaitEventSetWaitBlock (set=0x561cf1d00340, cur_timeout=-1, occurred_events=0x7ffc0e561000, nevents=1) at waiteventset.c:1190 #2 0x0000561cd9380f74 in WaitEventSetWait (set=0x561cf1d00340, timeout=-1, occurred_events=0x7ffc0e561000, nevents=1, wait_event_info=50331648) at waiteventset.c:1138 #3 0x0000561cd936f6a5 in WaitLatch (latch=0x7f2f665629e4, wakeEvents=33, timeout=0, wait_event_info=50331648) at latch.c:194 #4 0x0000561cd939fdbc in ProcSleep (locallock=0x561cf1d63560) at proc.c:1454 #5 0x0000561cd938b2c6 in WaitOnLock (locallock=0x561cf1d63560, owner=0x561cf1d44040) at lock.c:1968 #6 0x0000561cd9389dbd in LockAcquireExtended (locktag=0x7ffc0e5613f0, lockmode=1, sessionLock=false, dontWait=false, reportMemoryError=true, locallockp=0x7ffc0e5613e8, logLockFailure=false) at lock.c:1217 #7 0x0000561cd93864bc in LockRelationOid (relid=16463, lockmode=1) at lmgr.c:115 #8 0x0000561cd8daf922 in relation_open (relationId=16463, lockmode=1) at relation.c:55 #9 0x0000561cd95a29f3 in generate_partition_qual (rel=0x7f2f5d7d6640) at partcache.c:362 #10 0x0000561cd95a28b8 in RelationGetPartitionQual (rel=0x7f2f5d7d6640) at partcache.c:283 #11 0x0000561cd90bd59f in ExecPartitionCheck (resultRelInfo=0x561cf1d2ead8, slot=0x561cf1d33af8, estate=0x561cf1dde450, emitError=true) at execMain.c:1952 #12 0x0000561cd8fd1c9f in CopyFrom (cstate=0x561cf1de23a8) at copyfrom.c:1368 #13 0x0000561cd8fccd30 in DoCopy (pstate=0x561cf1d2f9e0, stmt=0x561cf1d06140, stmt_location=0, stmt_len=39, processed=0x7ffc0e561830) at copy.c:306 #14 0x0000561cd93ba623 in standard_ProcessUtility (pstmt=0x561cf1d06210, queryString=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at utility.c:738 #15 0x0000561cd93b9f4c in ProcessUtility (pstmt=0x561cf1d06210, queryString=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n", readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at utility.c:523 #16 0x0000561cd93b87a9 in PortalRunUtility (portal=0x561cf1d85d90, pstmt=0x561cf1d06210, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:1185 #17 0x0000561cd93b8a52 in PortalRunMulti (portal=0x561cf1d85d90, isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d065d0, altdest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:1349 #18 0x0000561cd93b7e73 in PortalRun (portal=0x561cf1d85d90, count=9223372036854775807, isTopLevel=true, dest=0x561cf1d065d0, altdest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:820 #19 0x0000561cd93b037e in exec_simple_query (query_string=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n") at postgres.c:1274 #20 0x0000561cd93b5b46 in PostgresMain (dbname=0x561cf1d3f580 "target", username=0x561cf1d3f568 "ubuntu") at postgres.c:4771 #21 0x0000561cd93abab7 in BackendMain (startup_data=0x7ffc0e561e50, startup_data_len=24) at backend_startup.c:124 #22 0x0000561cd92abf09 in postmaster_child_launch (child_type=B_BACKEND, child_slot=1, startup_data=0x7ffc0e561e50, startup_data_len=24, client_sock=0x7ffc0e561eb0) at launch_backend.c:290 #23 0x0000561cd92b2946 in BackendStartup (client_sock=0x7ffc0e561eb0) at postmaster.c:3580 #24 0x0000561cd92afeb1 in ServerLoop () at postmaster.c:1702 #25 0x0000561cd92af7a2 in PostmasterMain (argc=3, argv=0x561cf1cffb00) at postmaster.c:1400 #26 0x0000561cd914cf06 in main (argc=3, argv=0x561cf1cffb00) at main.c:227 The alter table session do check its children not-null constraint using lockmod=8, and the copy session do get partition_qual to lock parent using lockmode =1. I wonder if we have to use the same lockmode for checking children's not-null constraint. -- Thanks, Tender Wang -
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2025-04-15T05:08:24Z
Tender Wang <tndrwang@gmail.com> 于2025年4月15日周二 11:20写道： > > > Tom Lane <tgl@sss.pgh.pa.us> 于2025年4月15日周二 05:39写道： > >> Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> > On 2025-Apr-14, Tom Lane wrote: >> >> I would not have expected that adding pg_constraint rows implies >> >> stronger locks than what ALTER ADD PRIMARY KEY was using before, >> >> and I suspect that doing so will cause more problems than just >> >> breaking parallel restore. >> >> > I wasn't aware of this side effect. I'll investigate this in more >> > depth. I suspect it might be a bug in the way we run through ALTER >> > TABLE for the primary key. >> >> After further thought it occurs to me that it might not be a case >> of "we get stronger locks", but a case of "we accidentally get a >> weaker lock earlier and then try to upgrade it", thus creating a >> possibility of deadlock where before we'd just have blocked till >> the other statement cleared. Still worthy of being fixed if that's >> true, though. >> > > I added sleep(1) in the DeadLockReport() before error report to display > the status when a deadlock happened. > bool continue_sleep = true; > do > { > sleep(1); > } while (continue_sleep); > ereport(ERROR, > (errcode(ERRCODE_T_R_DEADLOCK_DETECTED), > errmsg("deadlock detected"), > errdetail_internal("%s", clientbuf.data), > errdetail_log("%s", logbuf.data), > errhint("See server log for query details."))); > > > > ubuntu@VM-0-17-ubuntu:/workspace/postgres$ ps -ef|grep postgres > ubuntu 2911109 1 0 10:34 ? 00:00:00 > /workspace/pgsql/bin/postgres -D ../data > ubuntu 2911110 2911109 0 10:34 ? 00:00:00 postgres: io worker 0 > ubuntu 2911111 2911109 0 10:34 ? 00:00:00 postgres: io worker 1 > ubuntu 2911112 2911109 0 10:34 ? 00:00:00 postgres: io worker 2 > ubuntu 2911113 2911109 0 10:34 ? 00:00:00 postgres: checkpointer > ubuntu 2911114 2911109 0 10:34 ? 00:00:00 postgres: background > writer > ubuntu 2911116 2911109 0 10:34 ? 00:00:00 postgres: walwriter > ubuntu 2911117 2911109 0 10:34 ? 00:00:00 postgres: autovacuum > launcher > ubuntu 2911118 2911109 0 10:34 ? 00:00:00 postgres: logical > replication launcher > ubuntu 2911180 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] COPY waiting > ubuntu 2911184 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] idle > ubuntu 2911187 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] idle > ubuntu 2911188 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] ALTER TABLE > ubuntu 2911189 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] SELECT waiting > ubuntu 2911190 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] idle > ubuntu 2911191 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] TRUNCATE TABLE waiting > ubuntu 2911192 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] idle > ubuntu 2911193 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] SELECT waiting > ubuntu 2911194 2911109 0 10:34 ? 00:00:00 postgres: ubuntu > target [local] idle > > gdb -p 2911188 // ALTER TABLE ONLY public.parent2 ADD CONSTRAINT > parent2_pkey PRIMARY KEY (id); > (gdb) bt > #0 0x00007f2f6910878a in __GI___clock_nanosleep (clock_id=clock_id@entry=0, > flags=flags@entry=0, req=req@entry=0x7ffc0e560e60, rem=rem@entry=0x7ffc0e560e60) > at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78 > #1 0x00007f2f6910d677 in __GI___nanosleep (req=req@entry=0x7ffc0e560e60, > rem=rem@entry=0x7ffc0e560e60) at ../sysdeps/unix/sysv/linux/nanosleep.c:25 > #2 0x00007f2f6910d5ae in __sleep (seconds=0) at > ../sysdeps/posix/sleep.c:55 > #3 0x0000561cd9386100 in DeadLockReport () at deadlock.c:1136 > #4 0x0000561cd9389df8 in LockAcquireExtended (locktag=0x7ffc0e5610b0, > lockmode=8, sessionLock=false, dontWait=false, reportMemoryError=true, > locallockp=0x7ffc0e5610a8, logLockFailure=false) at lock.c:1232 > #5 0x0000561cd93864bc in LockRelationOid (relid=16473, lockmode=8) at > lmgr.c:115 > #6 0x0000561cd8f21b20 in find_inheritance_children_extended > (parentrelId=16463, omit_detached=true, lockmode=8, detached_exist=0x0, > detached_xmin=0x0) at pg_inherits.c:213 > #7 0x0000561cd8f217c1 in find_inheritance_children (parentrelId=16463, > lockmode=8) at pg_inherits.c:60 > #8 0x0000561cd904dd73 in ATPrepAddPrimaryKey (wqueue=0x7ffc0e561348, > rel=0x7f2f5d7d6240, cmd=0x561cf1d2ee38, recurse=false, lockmode=8, > context=0x7ffc0e561540) at tablecmds.c:9463 > #9 0x0000561cd9043906 in ATPrepCmd (wqueue=0x7ffc0e561348, > rel=0x7f2f5d7d6240, cmd=0x561cf1d2ee38, recurse=false, recursing=false, > lockmode=8, context=0x7ffc0e561540) at tablecmds.c:5079 > #10 0x0000561cd90432aa in ATController (parsetree=0x561cf1d062e0, > rel=0x7f2f5d7d6240, cmds=0x561cf1d06290, recurse=false, lockmode=8, > context=0x7ffc0e561540) at tablecmds.c:4871 > #11 0x0000561cd9042f3b in AlterTable (stmt=0x561cf1d062e0, lockmode=8, > context=0x7ffc0e561540) at tablecmds.c:4533 > #12 0x0000561cd93bb7a8 in ProcessUtilitySlow (pstate=0x561cf1d2f9e0, > pstmt=0x561cf1d06390, queryString=0x561cf1d05570 "ALTER TABLE ONLY > public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", > context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, > dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:1321 > #13 0x0000561cd93bb04e in standard_ProcessUtility (pstmt=0x561cf1d06390, > queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD > CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", readOnlyTree=false, > context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, > dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:1070 > #14 0x0000561cd93b9f4c in ProcessUtility (pstmt=0x561cf1d06390, > queryString=0x561cf1d05570 "ALTER TABLE ONLY public.parent2\n ADD > CONSTRAINT parent2_pkey PRIMARY KEY (id);\n\n\n", readOnlyTree=false, > context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, > dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at utility.c:523 > #15 0x0000561cd93b87a9 in PortalRunUtility (portal=0x561cf1d85d90, > pstmt=0x561cf1d06390, isTopLevel=true, setHoldSnapshot=false, > dest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:1185 > #16 0x0000561cd93b8a52 in PortalRunMulti (portal=0x561cf1d85d90, > isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d06750, > altdest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:1349 > #17 0x0000561cd93b7e73 in PortalRun (portal=0x561cf1d85d90, > count=9223372036854775807, isTopLevel=true, dest=0x561cf1d06750, > altdest=0x561cf1d06750, qc=0x7ffc0e561ba0) at pquery.c:820 > #18 0x0000561cd93b037e in exec_simple_query (query_string=0x561cf1d05570 > "ALTER TABLE ONLY public.parent2\n ADD CONSTRAINT parent2_pkey PRIMARY > KEY (id);\n\n\n") at postgres.c:1274 > #19 0x0000561cd93b5b46 in PostgresMain (dbname=0x561cf1d3f580 "target", > username=0x561cf1d3f568 "ubuntu") at postgres.c:4771 > #20 0x0000561cd93abab7 in BackendMain (startup_data=0x7ffc0e561e50, > startup_data_len=24) at backend_startup.c:124 > #21 0x0000561cd92abf09 in postmaster_child_launch (child_type=B_BACKEND, > child_slot=2, startup_data=0x7ffc0e561e50, startup_data_len=24, > client_sock=0x7ffc0e561eb0) at launch_backend.c:290 > #22 0x0000561cd92b2946 in BackendStartup (client_sock=0x7ffc0e561eb0) at > postmaster.c:3580 > #23 0x0000561cd92afeb1 in ServerLoop () at postmaster.c:1702 > #24 0x0000561cd92af7a2 in PostmasterMain (argc=3, argv=0x561cf1cffb00) at > postmaster.c:1400 > #25 0x0000561cd914cf06 in main (argc=3, argv=0x561cf1cffb00) at main.c:227 > > gdb -p 2911180 // COPY public.c22 (id, ref, b) FROM stdin; > (gdb) bt > #0 0x00007f2f69148dea in epoll_wait (epfd=5, events=0x561cf1d003a8, > maxevents=1, timeout=-1) at ../sysdeps/unix/sysv/linux/epoll_wait.c:30 > #1 0x0000561cd938106f in WaitEventSetWaitBlock (set=0x561cf1d00340, > cur_timeout=-1, occurred_events=0x7ffc0e561000, nevents=1) at > waiteventset.c:1190 > #2 0x0000561cd9380f74 in WaitEventSetWait (set=0x561cf1d00340, > timeout=-1, occurred_events=0x7ffc0e561000, nevents=1, > wait_event_info=50331648) at waiteventset.c:1138 > #3 0x0000561cd936f6a5 in WaitLatch (latch=0x7f2f665629e4, wakeEvents=33, > timeout=0, wait_event_info=50331648) at latch.c:194 > #4 0x0000561cd939fdbc in ProcSleep (locallock=0x561cf1d63560) at > proc.c:1454 > #5 0x0000561cd938b2c6 in WaitOnLock (locallock=0x561cf1d63560, > owner=0x561cf1d44040) at lock.c:1968 > #6 0x0000561cd9389dbd in LockAcquireExtended (locktag=0x7ffc0e5613f0, > lockmode=1, sessionLock=false, dontWait=false, reportMemoryError=true, > locallockp=0x7ffc0e5613e8, logLockFailure=false) at lock.c:1217 > #7 0x0000561cd93864bc in LockRelationOid (relid=16463, lockmode=1) at > lmgr.c:115 > #8 0x0000561cd8daf922 in relation_open (relationId=16463, lockmode=1) at > relation.c:55 > #9 0x0000561cd95a29f3 in generate_partition_qual (rel=0x7f2f5d7d6640) at > partcache.c:362 > #10 0x0000561cd95a28b8 in RelationGetPartitionQual (rel=0x7f2f5d7d6640) at > partcache.c:283 > #11 0x0000561cd90bd59f in ExecPartitionCheck > (resultRelInfo=0x561cf1d2ead8, slot=0x561cf1d33af8, estate=0x561cf1dde450, > emitError=true) at execMain.c:1952 > #12 0x0000561cd8fd1c9f in CopyFrom (cstate=0x561cf1de23a8) at > copyfrom.c:1368 > #13 0x0000561cd8fccd30 in DoCopy (pstate=0x561cf1d2f9e0, > stmt=0x561cf1d06140, stmt_location=0, stmt_len=39, > processed=0x7ffc0e561830) at copy.c:306 > #14 0x0000561cd93ba623 in standard_ProcessUtility (pstmt=0x561cf1d06210, > queryString=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n", > readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, > queryEnv=0x0, dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at utility.c:738 > #15 0x0000561cd93b9f4c in ProcessUtility (pstmt=0x561cf1d06210, > queryString=0x561cf1d05570 "COPY public.c22 (id, ref, b) FROM stdin;\n", > readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, > queryEnv=0x0, > dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at utility.c:523 > #16 0x0000561cd93b87a9 in PortalRunUtility (portal=0x561cf1d85d90, > pstmt=0x561cf1d06210, isTopLevel=true, setHoldSnapshot=false, > dest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:1185 > #17 0x0000561cd93b8a52 in PortalRunMulti (portal=0x561cf1d85d90, > isTopLevel=true, setHoldSnapshot=false, dest=0x561cf1d065d0, > altdest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:1349 > #18 0x0000561cd93b7e73 in PortalRun (portal=0x561cf1d85d90, > count=9223372036854775807, isTopLevel=true, dest=0x561cf1d065d0, > altdest=0x561cf1d065d0, qc=0x7ffc0e561ba0) at pquery.c:820 > #19 0x0000561cd93b037e in exec_simple_query (query_string=0x561cf1d05570 > "COPY public.c22 (id, ref, b) FROM stdin;\n") at postgres.c:1274 > #20 0x0000561cd93b5b46 in PostgresMain (dbname=0x561cf1d3f580 "target", > username=0x561cf1d3f568 "ubuntu") at postgres.c:4771 > #21 0x0000561cd93abab7 in BackendMain (startup_data=0x7ffc0e561e50, > startup_data_len=24) at backend_startup.c:124 > #22 0x0000561cd92abf09 in postmaster_child_launch (child_type=B_BACKEND, > child_slot=1, startup_data=0x7ffc0e561e50, startup_data_len=24, > client_sock=0x7ffc0e561eb0) at launch_backend.c:290 > #23 0x0000561cd92b2946 in BackendStartup (client_sock=0x7ffc0e561eb0) at > postmaster.c:3580 > #24 0x0000561cd92afeb1 in ServerLoop () at postmaster.c:1702 > #25 0x0000561cd92af7a2 in PostmasterMain (argc=3, argv=0x561cf1cffb00) at > postmaster.c:1400 > #26 0x0000561cd914cf06 in main (argc=3, argv=0x561cf1cffb00) at main.c:227 > > The alter table session do check its children not-null constraint using > lockmod=8, and the copy session do get partition_qual to lock parent using > lockmode =1. > I wonder if we have to use the same lockmode for checking children's > not-null constraint. > I thought further about the lockmode calling find_inheritance_children in ATPrepAddPrimaryKey. What we do here? We first get oids of children, then check the if the column of children has marked not-null, if not, report an error. No operation here on children. I check other places that call find_inheritance_children, if we have operation on children, we usually pass Lockmode to find_inheritance_children, otherwise pass NoLock. I try NoLock, then restore-deadlock.sh will have no error. -- Thanks, Tender Wang -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-04-15T19:11:49Z
On 2025-Apr-15, Tender Wang wrote: > I thought further about the lockmode calling find_inheritance_children > in ATPrepAddPrimaryKey. > What we do here? We first get oids of children, then check the if the > column of children has marked not-null, if not, report an error. > No operation here on children. I check other places that call > find_inheritance_children, if we have operation on children, we usually pass > Lockmode to find_inheritance_children, otherwise pass NoLock. Hmm, I'm wary of doing this, although you're perhaps right that there's no harm. If we do need to add a not-null constraint on the children, surely we'll acquire a stronger lock further down the execution chain. In principle this sounds a good idea though. (I'm not sure about doing SearchSysCacheAttName() on a relation that might be dropped concurrently; does dropping the child acquire lock on its parent? I suppose so, in which case this is okay; but still icky. What about DETACH CONCURRENTLY?) However, I've also been looking at this and realized that this code can have different structure which may allows us to skip the find_inheritance_children() altogether. The reason is that we already scan the parent's list of columns searching for not-null constraints on each of them; we only need to run this verification on children for columns where there is none in the parent, and then only in the case where recursion is turned off. So I propose the attached patch, which also has some comments to hopefully explain what is going on and why. I ran Tom's test script a few hundred times in a loop and I see no deadlock anymore. Note that I also considered the idea of just not doing the check at all; that is, if a child table doesn't have a not-null constraint, then let ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... ) create the not-null constraint. This works fine (it breaks one regression test query though, would be easily fixed). But I don't like this very much, because it means the user could be surprised by the lengthy unexpected runtime of creating the primary key, only to realize that the server is checking the child table for nulls. This is especially bad if the user says ONLY. I think it's better if they have the chance to create the not-null constraint on their own volition. -- Álvaro Herrera PostgreSQL Developer — 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 -
Re: not null constraints, again
Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-15T19:51:14Z
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > However, I've also been looking at this and realized that this code can > have different structure which may allows us to skip the > find_inheritance_children() altogether. The reason is that we already > scan the parent's list of columns searching for not-null constraints on > each of them; we only need to run this verification on children for > columns where there is none in the parent, and then only in the case > where recursion is turned off. +1. Fundamentally the problem here is that pg_restore needs ALTER TABLE ONLY foo ADD PRIMARY KEY to not recurse to child tables at all. It is expecting this command to acquire a lock on foo and nothing else; and it has already taken care of making foo's PK column(s) NOT NULL, so there is no reason we should have to examine the children. Looking at the patch itself, it doesn't seem like the got_children flag is accomplishing anything; I guess that was leftover from an earlier version? You could declare "List *children" inside the block where it's used, too. Basically, this patch is just moving the check-the-children logic from one place to another. Also I find the comments still a bit confusing, but maybe that's on me. regards, tom lane
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-04-15T21:10:26Z
On 2025-Apr-15, Tom Lane wrote: > +1. Fundamentally the problem here is that pg_restore needs > > ALTER TABLE ONLY foo ADD PRIMARY KEY > > to not recurse to child tables at all. It is expecting this command > to acquire a lock on foo and nothing else; and it has already taken > care of making foo's PK column(s) NOT NULL, so there is no reason we > should have to examine the children. Right. > Looking at the patch itself, it doesn't seem like the got_children > flag is accomplishing anything; I guess that was leftover from an > earlier version? You could declare "List *children" inside the > block where it's used, too. Basically, this patch is just moving > the check-the-children logic from one place to another. Ah yes, I forgot to set got_children when reading the children list. This happens within the loop for columns, so the idea is to obtain that list just once instead of once per column. I don't think there's any ill effect from doing it multiple times, but it's wasted work and that's what led me to adding got_children. I'll add the assignment. > Also I find the comments still a bit confusing, but maybe that's > on me. I'll review tomorrow morning, maybe I can find some improvements for them. -- Á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)
-
Re: not null constraints, again
Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-15T21:12:42Z
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > On 2025-Apr-15, Tom Lane wrote: >> Looking at the patch itself, it doesn't seem like the got_children >> flag is accomplishing anything; > Ah yes, I forgot to set got_children when reading the children list. > This happens within the loop for columns, so the idea is to obtain that > list just once instead of once per column. Ah, got it. Makes sense as long as you actually avoid the work ;-) regards, tom lane
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2025-04-16T06:28:27Z
Alvaro Herrera <alvherre@alvh.no-ip.org> 于2025年4月16日周三 03:12写道： > On 2025-Apr-15, Tender Wang wrote: > > > I thought further about the lockmode calling find_inheritance_children > > in ATPrepAddPrimaryKey. > > What we do here? We first get oids of children, then check the if the > > column of children has marked not-null, if not, report an error. > > No operation here on children. I check other places that call > > find_inheritance_children, if we have operation on children, we usually > pass > > Lockmode to find_inheritance_children, otherwise pass NoLock. > > Hmm, I'm wary of doing this, although you're perhaps right that there's > no harm. If we do need to add a not-null constraint on the children, > surely we'll acquire a stronger lock further down the execution chain. > In principle this sounds a good idea though. (I'm not sure about doing > SearchSysCacheAttName() on a relation that might be dropped > concurrently; does dropping the child acquire lock on its parent? I > suppose so, in which case this is okay; but still icky. What about > DETACH CONCURRENTLY?) > Yes, I'm also wary of doing this. Although NoLock may fix this issue, I feel it will trigger other problems, such as the scenario you listed above. > However, I've also been looking at this and realized that this code can > have different structure which may allows us to skip the > find_inheritance_children() altogether. The reason is that we already > scan the parent's list of columns searching for not-null constraints on > each of them; we only need to run this verification on children for > columns where there is none in the parent, and then only in the case > where recursion is turned off. > So I propose the attached patch, which also has some comments to > hopefully explain what is going on and why. I ran Tom's test script a > few hundred times in a loop and I see no deadlock anymore. > No objection from me. The comments may need a little polishing. -- Thanks, Tender Wang
-
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-04-16T11:24:13Z
Here's another version where I do skip searching for children twice, and rewrote the comments. I also noticed that in child tables we were only looking for pg_attribute.attnotnull, and not whether the constraints had been validated or made inheritable. This seemed a wasted opportunity, so I refactored the code to instead examine the pg_constraint row and apply the same checks as for the constraint on the parent (namely, that it's valid and not NO INHERIT). We already check for these things downstream (alter table phase 2, during AdjustNotNullInheritance), but only after potentially wasting more work, so it makes sense to do it here (alter table phase 1) given that it's very easy. I added some tests for these things also, as those cases weren't covered. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "¿Cómo puedes confiar en algo que pagas y que no ves, y no confiar en algo que te dan y te lo muestran?" (Germán Poo)
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2025-04-16T12:11:45Z
Alvaro Herrera <alvherre@alvh.no-ip.org> 于2025年4月16日周三 19:24写道： > Here's another version where I do skip searching for children twice, and > rewrote the comments. > > I also noticed that in child tables we were only looking for > pg_attribute.attnotnull, and not whether the constraints had been > validated or made inheritable. This seemed a wasted opportunity, so I > refactored the code to instead examine the pg_constraint row and apply > the same checks as for the constraint on the parent (namely, that it's > valid and not NO INHERIT). We already check for these things downstream > (alter table phase 2, during AdjustNotNullInheritance), but only after > potentially wasting more work, so it makes sense to do it here (alter > table phase 1) given that it's very easy. I added some tests for these > things also, as those cases weren't covered. > if (conForm->contype != CONSTRAINT_NOTNULL) elog(ERROR, "constraint %u is not a not-null constraint", conForm->oid); I feel that using conForm->conname is more friendly than oid for users. Others look good for me. -- Thanks, Tender Wang -
Re: not null constraints, again
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-04-16T12:24:18Z
On 2025-Apr-16, Tender Wang wrote: > if (conForm->contype != CONSTRAINT_NOTNULL) > elog(ERROR, "constraint %u is not a not-null constraint", conForm->oid); > > I feel that using conForm->conname is more friendly than oid for users. Yeah, this doesn't really matter because this function would not be called with any other kind of constraint anyway. This test could just as well be an Assert() ... I was pretty torn about that choice TBH (I still am). > Others look good for me. Thanks for looking! -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "No renuncies a nada. No te aferres a nada."
-
Re: not null constraints, again
Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-16T19:55:09Z
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > Here's another version where I do skip searching for children twice, and > rewrote the comments. v2 LGTM, with two small nits: 1. Grammar feels shaky here: + * normal case where we're asked to recurse, this routine ensures that the + * not-null constraints either exist already, or queues a requirement for them + * to be created by phase 2. The "either" seems to apply to "ensures" versus "queues", but it's in the wrong place for that. Maybe something like + * normal case where we're asked to recurse, this routine checks if the + * not-null constraints exist already, and if not queues a requirement for + * them to be created by phase 2. 2. Stupider compilers are likely to whine about the "children" variable possibly being used uninitialized. Suggest initializing it to NIL. regards, tom lane
-
Re: not null constraints, again
Tender Wang <tndrwang@gmail.com> — 2025-04-17T17:01:14Z
Hi, I found an inconsistent behavior in v17.4 and v18 after not-null constraints were committed. create table t1(a int not null); ALTER TABLE t1 ADD CONSTRAINT d PRIMARY KEY(a), ALTER a DROP NOT NULL; in v17.4, ALTER TABLE successes, but in v18, it reports below error: ERROR: primary key column "a" is not marked NOT NULL But if I separate the 'ALTER TABLE' command, there are no errors. postgres=# create table t1(a int not null); CREATE TABLE postgres=# ALTER TABLE t1 ADD CONSTRAINT d PRIMARY KEY(a), ALTER a DROP NOT NULL; ERROR: primary key column "a" is not marked NOT NULL postgres=# ALTER TABLE t1 ALTER a DROP NOT NULL; ALTER TABLE postgres=# ALTER TABLE t1 ADD CONSTRAINT d PRIMARY KEY(a); ALTER TABLE in v17.4, we first drop not null, but when run add primary key constraint, set_notnull command will be added in transformIndexConstraint(). But in v18, the adding of set_notnull command logic in transformIndexConstraint() has been removed. And when call ATPrepAddPrimaryKey(), the column has not-null constraint, so will not add not-null again. Is the above error expected in v18? If so, we had better add more words to the document. If it's not, we should fix it. BTW, the document doesn't state the order in which the commands are executed when users specify more than one manipulation in a single ALTER TABLE command. -- Thanks, Tender Wang
-
Re: not null constraints, again
Álvaro Herrera <alvherre@kurilemu.de> — 2025-04-17T18:17:10Z
Hello, On Thu, Apr 17, 2025, at 7:01 PM, Tender Wang wrote: > create table t1(a int not null); > ALTER TABLE t1 ADD CONSTRAINT d PRIMARY KEY(a), ALTER a DROP NOT NULL; > > in v17.4, ALTER TABLE successes, but in v18, it reports below error: > ERROR: primary key column "a" is not marked NOT NULL Yeah, I suppose this behavior is more or less expected. ALTER TABLE subcommands are reordered for execution on several passes (per AlterTablePass, which you're already familiar with). DROP commands are always executed first, so I suppose that what happens is that we first drop the not-null (and not queue addition of one because it already exists), then when time comes to add the PK we find (index_check_primary_key) that the column isn't not null. I guess if you don't want to get this error, just don't run this command. It's quite useless anyway. Note that if the column isn't not-null to start with, then this doesn't fail, yet you still end up with the column marked not-null. -- Álvaro Herrera