Thread: Re: not null constraints, again
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
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);
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
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)