From 106ec073dfae8d5854a948ffc3e89c4fae79e129 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@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-skBX93TcpA@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

