On Fri, Dec 6, 2024 at 10:48 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > On 2024-Dec-06, jian he wrote: > > > basically processCASbits > > from > > processCASbits($3, @3, "NOT NULL") > > processCASbits($5, @5, "CHECK") > > to > > processCASbits($3, @3, "DOMAIN with NOT NULL") > > processCASbits($5, @5, "DOMAIN with CHECK") > > This doesn't actually work from a translation point of view, because the > actual error message is split in two parts. I think it might be better > to pass a non-NULL variable to processCASbits, then in the caller check > whether that flag is true; if so, raise the error in a single ereport(). >
i let the error fail at AlterDomainAddConstraint. what do you think?
From 106ec073dfae8d5854a948ffc3e89c4fae79e129 Mon Sep 17 00:00:00 2001 From: jian he <jian.universal...@gmail.com> Date: Fri, 6 Dec 2024 23:58:34 +0800 Subject: [PATCH v2 1/1] refactor AlterDomainAddConstraint In gram.y only CHECK and NOT-NULL constraint are allowed for the ALTER DOMAIN ... ADD CONSTRAINT statement. so we can safely remove the code handles errors for other constraint type. as of constraint other property ([ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]) we alerady handled in processCASbits (gram.y). however AlterDomainAddConstraint only pass single (Node *newConstraint). that means we cannot trigger an error for constraints specified as "NOT DEFERRABLE" or "INITIALLY IMMEDIATE" because "NOT DEFERRABLE" or "INITIALLY IMMEDIATE" is actualy a seperated Constraint node. To error out case like: alter domain d_int add constraint cc1 check(value > 1) not deferrable initially immediate; we need to make DomainConstraint in gram.y more like TableConstraint. that means this patch, "not deferrable" and "initially immediate" words is accepted, maybe i should change create_domain.sgml Accordingly. i also made "alter domain d_int add constraint cc check(value > 1) no inherit;" fail. the error message is not so good, for example in master, for `alter domain d_int add constraint nn not null no inherit;` you get: ERROR: NOT NULL constraints cannot be marked NO INHERIT but NOT NULL constraints can be marked NO INHERIT. for domain constraint part, i slightly changed the third parameter of processCASbits while calling. so now the error message becomes: ERROR: DOAMIN with NOT NULL constraints cannot be marked NO INHERIT discussion: https://postgr.es/m/cacjufxhitd5lglbssapshhtdwxt0vivkthinkyw-skbx93t...@mail.gmail.com --- src/backend/commands/tablecmds.c | 2 +- src/backend/commands/typecmds.c | 68 +++++++++++++--------------- src/backend/parser/gram.y | 12 ++--- src/backend/tcop/utility.c | 3 +- src/include/commands/typecmds.h | 2 +- src/test/regress/expected/domain.out | 58 ++++++++++++++++++++++++ src/test/regress/sql/domain.sql | 22 +++++++++ 7 files changed, 121 insertions(+), 46 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 6ccae4cb4a..2dd33c0435 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5390,7 +5390,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, address = AlterDomainAddConstraint(((AlterDomainStmt *) cmd->def)->typeName, ((AlterDomainStmt *) cmd->def)->def, - NULL); + NULL, context->queryString); break; case AT_ReAddComment: /* Re-add existing comment */ address = CommentObject((CommentStmt *) cmd->def); diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index da591c0922..96da0fd11d 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -2903,7 +2903,7 @@ AlterDomainDropConstraint(List *names, const char *constrName, */ ObjectAddress AlterDomainAddConstraint(List *names, Node *newConstraint, - ObjectAddress *constrAddr) + ObjectAddress *constrAddr, const char *querystring) { TypeName *typename; Oid domainoid; @@ -2913,10 +2913,13 @@ AlterDomainAddConstraint(List *names, Node *newConstraint, Constraint *constr; char *ccbin; ObjectAddress address = InvalidObjectAddress; + ParseState *pstate; + pstate = make_parsestate(NULL); + pstate->p_sourcetext = querystring; /* Make a TypeName so we can use standard type lookup machinery */ typename = makeTypeNameFromNameList(names); - domainoid = typenameTypeId(NULL, typename); + domainoid = typenameTypeId(pstate, typename); /* Look up the domain in the type table */ typrel = table_open(TypeRelationId, RowExclusiveLock); @@ -2938,43 +2941,34 @@ AlterDomainAddConstraint(List *names, Node *newConstraint, switch (constr->contype) { case CONSTR_CHECK: + if (constr->is_no_inherit) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("DOMAIN with CHECK constraints cannot be marked NO INHERIT"), + parser_errposition(pstate, constr->location)); + if (constr->deferrable || constr->initdeferred) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("DOMAIN with CHECK constraints cannot be marked DEFERRABLE"), + parser_errposition(pstate, constr->location)); + break; case CONSTR_NOTNULL: - /* processed below */ + if (constr->skip_validation) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("DOMAIN with NOT NULL constraints cannot be marked NOT VALID"), + parser_errposition(pstate, constr->location)); + if (constr->is_no_inherit) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("DOMAIN with NOT NULL constraints cannot be marked NO INHERIT"), + parser_errposition(pstate, constr->location)); + if (constr->deferrable || constr->initdeferred) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("DOMAIN with NOT NULL constraints cannot be marked DEFERRABLE"), + parser_errposition(pstate, constr->location)); break; - - case CONSTR_UNIQUE: - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("unique constraints not possible for domains"))); - break; - - case CONSTR_PRIMARY: - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("primary key constraints not possible for domains"))); - break; - - case CONSTR_EXCLUSION: - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("exclusion constraints not possible for domains"))); - break; - - case CONSTR_FOREIGN: - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("foreign key constraints not possible for domains"))); - break; - - case CONSTR_ATTR_DEFERRABLE: - case CONSTR_ATTR_NOT_DEFERRABLE: - case CONSTR_ATTR_DEFERRED: - case CONSTR_ATTR_IMMEDIATE: - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("specifying constraint deferrability not supported for domains"))); - break; - default: elog(ERROR, "unrecognized constraint subtype: %d", (int) constr->contype); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 67eb96396a..ba90daa265 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -4321,8 +4321,8 @@ DomainConstraintElem: n->location = @1; n->raw_expr = $3; n->cooked_expr = NULL; - processCASbits($5, @5, "CHECK", - NULL, NULL, &n->skip_validation, + processCASbits($5, @5, "DOMAIN with CHECK", + &n->deferrable, &n->initdeferred, &n->skip_validation, &n->is_no_inherit, yyscanner); n->initially_valid = !n->skip_validation; $$ = (Node *) n; @@ -4334,10 +4334,10 @@ DomainConstraintElem: n->contype = CONSTR_NOTNULL; n->location = @1; n->keys = list_make1(makeString("value")); - /* no NOT VALID, NO INHERIT support */ - processCASbits($3, @3, "NOT NULL", - NULL, NULL, NULL, - NULL, yyscanner); + /* no NOT VALID, NO INHERIT support, error check at AlterDomainAddConstraint */ + processCASbits($3, @3, "DOMAIN with NOT NULL", + &n->deferrable, &n->initdeferred, &n->skip_validation, + &n->is_no_inherit, yyscanner); n->initially_valid = true; $$ = (Node *) n; } diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index f28bf37105..ebe24ff8b0 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1367,7 +1367,8 @@ ProcessUtilitySlow(ParseState *pstate, address = AlterDomainAddConstraint(stmt->typeName, stmt->def, - &secondaryObject); + &secondaryObject, + queryString); break; case 'X': /* DROP CONSTRAINT */ address = diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h index e1b02927c4..76b2528b36 100644 --- a/src/include/commands/typecmds.h +++ b/src/include/commands/typecmds.h @@ -35,7 +35,7 @@ extern Oid AssignTypeMultirangeArrayOid(void); extern ObjectAddress AlterDomainDefault(List *names, Node *defaultRaw); extern ObjectAddress AlterDomainNotNull(List *names, bool notNull); extern ObjectAddress AlterDomainAddConstraint(List *names, Node *newConstraint, - ObjectAddress *constrAddr); + ObjectAddress *constrAddr, const char* querstring); extern ObjectAddress AlterDomainValidateConstraint(List *names, const char *constrName); extern ObjectAddress AlterDomainDropConstraint(List *names, const char *constrName, DropBehavior behavior, bool missing_ok); diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out index 42b6559f9c..87ab3f596f 100644 --- a/src/test/regress/expected/domain.out +++ b/src/test/regress/expected/domain.out @@ -15,6 +15,64 @@ NOTICE: drop cascades to type dependenttypetest -- this should fail because already gone drop domain domaindroptest cascade; ERROR: type "domaindroptest" does not exist +--alter domain error case. +create domain d_int as int4; +alter domain d_int add constraint nn not null no inherit; +ERROR: DOMAIN with NOT NULL constraints cannot be marked NO INHERIT +LINE 1: alter domain d_int add constraint nn not null no inherit; + ^ +alter domain d_int add constraint nn not null not valid; +ERROR: DOMAIN with NOT NULL constraints cannot be marked NOT VALID +LINE 1: alter domain d_int add constraint nn not null not valid; + ^ +alter domain d_int add constraint nn not null deferrable; +ERROR: DOMAIN with NOT NULL constraints cannot be marked DEFERRABLE +LINE 1: alter domain d_int add constraint nn not null deferrable; + ^ +alter domain d_int add constraint nn not null initially deferred; +ERROR: DOMAIN with NOT NULL constraints cannot be marked DEFERRABLE +LINE 1: alter domain d_int add constraint nn not null initially defe... + ^ +alter domain d_int add constraint nn not null deferrable initially deferred; +ERROR: DOMAIN with NOT NULL constraints cannot be marked DEFERRABLE +LINE 1: alter domain d_int add constraint nn not null deferrable ini... + ^ +alter domain d_int add constraint nn not null deferrable not deferrable; +ERROR: conflicting constraint properties +LINE 1: ...omain d_int add constraint nn not null deferrable not deferr... + ^ +alter domain d_int add constraint cc check(value > 1) no inherit; +ERROR: DOMAIN with CHECK constraints cannot be marked NO INHERIT +LINE 1: alter domain d_int add constraint cc check(value > 1) no inh... + ^ +alter domain d_int add constraint cc check(value > 1) deferrable; +ERROR: DOMAIN with CHECK constraints cannot be marked DEFERRABLE +LINE 1: alter domain d_int add constraint cc check(value > 1) deferr... + ^ +alter domain d_int add constraint cc check(value > 1) initially deferred; +ERROR: DOMAIN with CHECK constraints cannot be marked DEFERRABLE +LINE 1: alter domain d_int add constraint cc check(value > 1) initia... + ^ +alter domain d_int add constraint cc check(value > 1) deferrable initially deferred not valid; +ERROR: DOMAIN with CHECK constraints cannot be marked DEFERRABLE +LINE 1: alter domain d_int add constraint cc check(value > 1) deferr... + ^ +alter domain d_int add constraint cc check(value > 1) deferrable NOT deferrable not valid; +ERROR: conflicting constraint properties +LINE 1: ...int add constraint cc check(value > 1) deferrable NOT deferr... + ^ +--alter domain valid case. +alter domain d_int add constraint cc not null not deferrable initially IMMEDIATE; --ok +alter domain d_int add constraint cc1 check(value > 1) not deferrable initially immediate; --ok +alter domain d_int add constraint cc2 check(value > 11) not deferrable initially immediate not valid; --ok +\dD d_int + List of domains + Schema | Name | Type | Collation | Nullable | Default | Check +--------+-------+---------+-----------+----------+---------+------------------------------------------------ + public | d_int | integer | | not null | | CHECK (VALUE > 1) CHECK (VALUE > 11) NOT VALID +(1 row) + +DROP DOMAIN d_int; -- Test domain input. -- Note: the point of checking both INSERT and COPY FROM is that INSERT -- exercises CoerceToDomain while COPY exercises domain_in. diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql index ee07b03174..ae33abea38 100644 --- a/src/test/regress/sql/domain.sql +++ b/src/test/regress/sql/domain.sql @@ -17,6 +17,28 @@ drop domain domaindroptest cascade; drop domain domaindroptest cascade; +--alter domain error case. +create domain d_int as int4; +alter domain d_int add constraint nn not null no inherit; +alter domain d_int add constraint nn not null not valid; +alter domain d_int add constraint nn not null deferrable; +alter domain d_int add constraint nn not null initially deferred; +alter domain d_int add constraint nn not null deferrable initially deferred; +alter domain d_int add constraint nn not null deferrable not deferrable; + +alter domain d_int add constraint cc check(value > 1) no inherit; +alter domain d_int add constraint cc check(value > 1) deferrable; +alter domain d_int add constraint cc check(value > 1) initially deferred; +alter domain d_int add constraint cc check(value > 1) deferrable initially deferred not valid; +alter domain d_int add constraint cc check(value > 1) deferrable NOT deferrable not valid; + +--alter domain valid case. +alter domain d_int add constraint cc not null not deferrable initially IMMEDIATE; --ok +alter domain d_int add constraint cc1 check(value > 1) not deferrable initially immediate; --ok +alter domain d_int add constraint cc2 check(value > 11) not deferrable initially immediate not valid; --ok +\dD d_int +DROP DOMAIN d_int; + -- Test domain input. -- Note: the point of checking both INSERT and COPY FROM is that INSERT -- 2.34.1
