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);
