public inbox for [email protected]help / color / mirror / Atom feed
Re: CREATE SCHEMA ... CREATE DOMAIN support 6+ messages / 3 participants [nested] [flat]
* Re: CREATE SCHEMA ... CREATE DOMAIN support @ 2024-11-27 18:39 Tom Lane <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Tom Lane @ 2024-11-27 18:39 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: jian he <[email protected]>; PostgreSQL Hackers <[email protected]> Kirill Reshke <[email protected]> writes: > On Wed, 27 Nov 2024 at 08:42, jian he <[email protected]> wrote: >> CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE >> create domain ss1 as ss >> create domain ss as text; >> ERROR: type "ss" does not exist >> >> the error message seems not that OK, >> if we can point out the error position, that would be great. > To implement this, we need to include `ParseLoc location` to the > `CreateDomainStmt` struct, which is doubtful, because I don't see any > other type of create *something* that does this. No, that error is thrown from typenameType(), which has a perfectly good location in the TypeName. What it's lacking is a ParseState containing the source query string. Breakpoint 1, typenameType (pstate=pstate@entry=0x0, typeName=0x25d6b58, typmod_p=typmod_p@entry=0x7ffe7dcd641c) at parse_type.c:268 268 tup = LookupTypeName(pstate, typeName, typmod_p, false); (gdb) p pstate $2 = (ParseState *) 0x0 (gdb) p typeName->location $3 = 21 We've fixed a few utility statements so that they can receive a passed-down ParseState, but not DefineDomain. regards, tom lane ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: CREATE SCHEMA ... CREATE DOMAIN support @ 2024-11-28 05:27 Kirill Reshke <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Kirill Reshke @ 2024-11-28 05:27 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: jian he <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, 27 Nov 2024 at 23:39, Tom Lane <[email protected]> wrote: > > Kirill Reshke <[email protected]> writes: > > On Wed, 27 Nov 2024 at 08:42, jian he <[email protected]> wrote: > >> CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE > >> create domain ss1 as ss > >> create domain ss as text; > >> ERROR: type "ss" does not exist > >> > >> the error message seems not that OK, > >> if we can point out the error position, that would be great. > > > To implement this, we need to include `ParseLoc location` to the > > `CreateDomainStmt` struct, which is doubtful, because I don't see any > > other type of create *something* that does this. > > No, that error is thrown from typenameType(), which has a perfectly > good location in the TypeName. What it's lacking is a ParseState > containing the source query string. > > Breakpoint 1, typenameType (pstate=pstate@entry=0x0, typeName=0x25d6b58, > typmod_p=typmod_p@entry=0x7ffe7dcd641c) at parse_type.c:268 > 268 tup = LookupTypeName(pstate, typeName, typmod_p, false); > (gdb) p pstate > $2 = (ParseState *) 0x0 > (gdb) p typeName->location > $3 = 21 > > We've fixed a few utility statements so that they can receive > a passed-down ParseState, but not DefineDomain. > > regards, tom lane Indeed, my analysis is wrong. Turns out passing parsestate to DefineDomain is itself enhancement. Before this patch: ``` db1=# create domain ss1 as ss; ERROR: type "ss" does not exist ``` after: ``` db1=# create domain ss1 as ss; ERROR: type "ss" does not exist LINE 1: create domain ss1 as ss; ^ ``` PFA as an independent patch then. Or should we combine these two into one? -- Best regards, Kirill Reshke Attachments: [application/octet-stream] v1-0001-Pass-ParseState-as-first-param-to-DefineRelation.patch (2.7K, 2-v1-0001-Pass-ParseState-as-first-param-to-DefineRelation.patch) download | inline diff: From e9a0908697ff89e73197b2a366b4c1c41a416975 Mon Sep 17 00:00:00 2001 From: reshke kirill <[email protected]> Date: Thu, 28 Nov 2024 05:19:58 +0000 Subject: [PATCH v1] Pass ParseState as first param to DefineRelation ParseState was lacking in typenameType call inside DefineDomain. This patch fixes that. Now error-message for create domain also show error location. --- src/backend/commands/typecmds.c | 8 ++------ src/backend/tcop/utility.c | 2 +- src/include/commands/typecmds.h | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index 859e2191f08..42407df89a5 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -694,7 +694,7 @@ RemoveTypeById(Oid typeOid) * Registers a new domain. */ ObjectAddress -DefineDomain(CreateDomainStmt *stmt) +DefineDomain(ParseState *pstate, CreateDomainStmt *stmt) { char *domainName; char *domainArrayName; @@ -761,7 +761,7 @@ DefineDomain(CreateDomainStmt *stmt) /* * Look up the base type. */ - typeTup = typenameType(NULL, stmt->typeName, &basetypeMod); + typeTup = typenameType(pstate, stmt->typeName, &basetypeMod); baseType = (Form_pg_type) GETSTRUCT(typeTup); basetypeoid = baseType->oid; @@ -885,12 +885,8 @@ DefineDomain(CreateDomainStmt *stmt) if (constr->raw_expr) { - ParseState *pstate; Node *defaultExpr; - /* Create a dummy ParseState for transformExpr */ - pstate = make_parsestate(NULL); - /* * Cook the constr->raw_expr into an expression. Note: * name is strictly for error message diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index f28bf371059..33dea5a781c 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1712,7 +1712,7 @@ ProcessUtilitySlow(ParseState *pstate, break; case T_CreateDomainStmt: - address = DefineDomain((CreateDomainStmt *) parsetree); + address = DefineDomain(pstate, (CreateDomainStmt *) parsetree); break; case T_CreateConversionStmt: diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h index e1b02927c4b..cb30d1a2583 100644 --- a/src/include/commands/typecmds.h +++ b/src/include/commands/typecmds.h @@ -23,7 +23,7 @@ extern ObjectAddress DefineType(ParseState *pstate, List *names, List *parameters); extern void RemoveTypeById(Oid typeOid); -extern ObjectAddress DefineDomain(CreateDomainStmt *stmt); +extern ObjectAddress DefineDomain(ParseState *pstate, CreateDomainStmt *stmt); extern ObjectAddress DefineEnum(CreateEnumStmt *stmt); extern ObjectAddress DefineRange(ParseState *pstate, CreateRangeStmt *stmt); extern ObjectAddress AlterEnum(AlterEnumStmt *stmt); -- 2.34.1 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: CREATE SCHEMA ... CREATE DOMAIN support @ 2024-11-28 05:52 Tom Lane <[email protected]> parent: Kirill Reshke <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Tom Lane @ 2024-11-28 05:52 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: jian he <[email protected]>; PostgreSQL Hackers <[email protected]> Kirill Reshke <[email protected]> writes: > On Wed, 27 Nov 2024 at 23:39, Tom Lane <[email protected]> wrote: >> We've fixed a few utility statements so that they can receive >> a passed-down ParseState, but not DefineDomain. > PFA as an independent patch then. Or should we combine these two into one? No, I don't think this should be part of the patch discussed in this thread. It feels rather random to me to be fixing only DefineDomain; I'm sure there's more in the same vein. I'd like to see a patch with a scope along the lines of "fix everything reachable within CREATE SCHEMA" or perhaps "fix all calls of typenameType". (A quick grep shows that an outright majority of the callers of that are passing null ParseState. I didn't look to see if any of them have a good excuse beyond "we didn't do the plumbing work".) regards, tom lane ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: CREATE SCHEMA ... CREATE DOMAIN support @ 2024-11-28 09:58 Kirill Reshke <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Kirill Reshke @ 2024-11-28 09:58 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: jian he <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, 28 Nov 2024 at 10:52, Tom Lane <[email protected]> wrote: > No, I don't think this should be part of the patch discussed in this > thread. Ok, I created a separate thread for this. How about this one? Do you think the suggested idea is good? Is it worthwhile to do this, in your opinion? -- Best regards, Kirill Reshke ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: CREATE SCHEMA ... CREATE DOMAIN support @ 2024-11-29 13:47 jian he <[email protected]> parent: Kirill Reshke <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: jian he @ 2024-11-29 13:47 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> new patch, add tab complete for it. Attachments: [text/x-patch] v5-0001-support-CREATE-SCHEMA-CREATE-DOMAIN.patch (10.7K, 2-v5-0001-support-CREATE-SCHEMA-CREATE-DOMAIN.patch) download | inline diff: From 91d05e547ca722d4537ff7420b8248a3fcce3b58 Mon Sep 17 00:00:00 2001 From: jian he <[email protected]> Date: Fri, 29 Nov 2024 21:44:54 +0800 Subject: [PATCH v5 1/1] support CREATE SCHEMA CREATE DOMAIN SQL standard allow domain to be specified with CREATE SCHEMA statement. This patch adds support in PostgreSQL for that. For example: CREATE SCHEMA schema_name AUTHORIZATION CURRENT_ROLE create view test as select 'hello'::ss as test CREATE table t(a ss) create domain ss as text not null; The domain will be created within the to be created schema. The domain name can be schema-qualified or database-qualified, however it's not allowed to let domain create within a different schema. Author: Kirill Reshke <[email protected]> Author: Jian He <[email protected]> Reviewed-by: Alvaro Herrera <[email protected]> Reviewed-by: Tom Lane <[email protected]> --- doc/src/sgml/ref/create_schema.sgml | 2 +- src/backend/parser/gram.y | 1 + src/backend/parser/parse_utilcmd.c | 27 +++++++++++ src/bin/psql/tab-complete.in.c | 12 ++--- src/test/regress/expected/create_schema.out | 51 +++++++++++++++++++++ src/test/regress/sql/create_schema.sql | 33 +++++++++++++ 6 files changed, 119 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/ref/create_schema.sgml b/doc/src/sgml/ref/create_schema.sgml index ed69298ccc..06f6314a5b 100644 --- a/doc/src/sgml/ref/create_schema.sgml +++ b/doc/src/sgml/ref/create_schema.sgml @@ -100,7 +100,7 @@ CREATE SCHEMA IF NOT EXISTS AUTHORIZATION <replaceable class="parameter">role_sp <listitem> <para> An SQL statement defining an object to be created within the - schema. Currently, only <command>CREATE + schema. Currently, only <command>CREATE DOMAIN</command>, <command>CREATE TABLE</command>, <command>CREATE VIEW</command>, <command>CREATE INDEX</command>, <command>CREATE SEQUENCE</command>, <command>CREATE TRIGGER</command> and <command>GRANT</command> are accepted as clauses diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 67eb96396a..ad8d9270ac 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -1584,6 +1584,7 @@ schema_stmt: | CreateTrigStmt | GrantStmt | ViewStmt + | CreateDomainStmt ; diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 0f324ee4e3..45328eea16 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -105,6 +105,7 @@ typedef struct List *indexes; /* CREATE INDEX items */ List *triggers; /* CREATE TRIGGER items */ List *grants; /* GRANT items */ + List *domains; /* DOMAIN items */ } CreateSchemaStmtContext; @@ -4039,6 +4040,7 @@ transformCreateSchemaStmtElements(List *schemaElts, const char *schemaName) cxt.indexes = NIL; cxt.triggers = NIL; cxt.grants = NIL; + cxt.domains = NIL; /* * Run through each schema element in the schema element list. Separate @@ -4107,6 +4109,30 @@ transformCreateSchemaStmtElements(List *schemaElts, const char *schemaName) cxt.grants = lappend(cxt.grants, element); break; + case T_CreateDomainStmt: + { + CreateDomainStmt *elp = (CreateDomainStmt *) element; + char *domain_schema = NULL; + + /* + * DOMAIN's schema must the same as the to be created + * schema if length of domainname > 3 will fail at + * DeconstructQualifiedName, + */ + if (list_length(elp->domainname) == 2) + { + domain_schema = strVal(list_nth(elp->domainname, 0)); + setSchemaName(cxt.schemaname, &domain_schema); + } + else if (list_length(elp->domainname) == 3) + { + domain_schema = strVal(list_nth(elp->domainname, 1)); + setSchemaName(cxt.schemaname, &domain_schema); + } + cxt.domains = lappend(cxt.domains, element); + } + break; + default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(element)); @@ -4114,6 +4140,7 @@ transformCreateSchemaStmtElements(List *schemaElts, const char *schemaName) } result = NIL; + result = list_concat(result, cxt.domains); result = list_concat(result, cxt.sequences); result = list_concat(result, cxt.tables); result = list_concat(result, cxt.views); diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c index bbd08770c3..542429712a 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -2135,7 +2135,7 @@ match_previous_words(int pattern_id, { /* only some object types can be created as part of CREATE SCHEMA */ if (HeadMatches("CREATE", "SCHEMA")) - COMPLETE_WITH("TABLE", "VIEW", "INDEX", "SEQUENCE", "TRIGGER", + COMPLETE_WITH("TABLE", "VIEW", "INDEX", "SEQUENCE", "TRIGGER", "DOMAIN", /* for INDEX and TABLE/SEQUENCE, respectively */ "UNIQUE", "UNLOGGED"); else @@ -3293,15 +3293,15 @@ match_previous_words(int pattern_id, else if (Matches("CREATE", "DATABASE", MatchAny, "STRATEGY")) COMPLETE_WITH("WAL_LOG", "FILE_COPY"); - /* CREATE DOMAIN */ - else if (Matches("CREATE", "DOMAIN", MatchAny)) + /* CREATE DOMAIN --- is allowed inside CREATE SCHEMA, so use TailMatches */ + else if (TailMatches("CREATE", "DOMAIN", MatchAny)) COMPLETE_WITH("AS"); - else if (Matches("CREATE", "DOMAIN", MatchAny, "AS")) + else if (TailMatches("CREATE", "DOMAIN", MatchAny, "AS")) COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_datatypes); - else if (Matches("CREATE", "DOMAIN", MatchAny, "AS", MatchAny)) + else if (TailMatches("CREATE", "DOMAIN", MatchAny, "AS", MatchAny)) COMPLETE_WITH("COLLATE", "DEFAULT", "CONSTRAINT", "NOT NULL", "NULL", "CHECK ("); - else if (Matches("CREATE", "DOMAIN", MatchAny, "COLLATE")) + else if (TailMatches("CREATE", "DOMAIN", MatchAny, "COLLATE")) COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_collations); /* CREATE EXTENSION */ diff --git a/src/test/regress/expected/create_schema.out b/src/test/regress/expected/create_schema.out index 93302a07ef..d2b97911cc 100644 --- a/src/test/regress/expected/create_schema.out +++ b/src/test/regress/expected/create_schema.out @@ -58,6 +58,57 @@ CREATE SCHEMA regress_schema_1 AUTHORIZATION CURRENT_ROLE EXECUTE FUNCTION schema_trig.no_func(); ERROR: CREATE specifies a schema (schema_not_existing) different from the one being created (regress_schema_1) RESET ROLE; +-- Cases where the schema creation with domain. +--fail. cannot create domain to other schema +CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE + create table t(a ss) + create domain public.ss as text not null default 'hello' constraint nn check (value <> 'hello'); +ERROR: CREATE specifies a schema (public) different from the one being created (regress_schema_2) +--fail. cannot create domain to other schema +CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE + create table t(a ss) + create domain postgres.public.ss as text not null default 'hello' constraint nn check (value <> 'hello'); +ERROR: CREATE specifies a schema (public) different from the one being created (regress_schema_2) +--fail. forward references, need reorder. +CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE + create domain ss1 as ss + create domain ss as text; +ERROR: type "ss" does not exist +--ok, qualified schema name for domain should be same as the created schema. +CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE + create table t(a regress_schema_2.ss) + create domain regress_schema_2.ss as text not null; +\dD regress_schema_2.* + List of domains + Schema | Name | Type | Collation | Nullable | Default | Check +------------------+------+------+-----------+----------+---------+------- + regress_schema_2 | ss | text | | not null | | +(1 row) + +--ok, no qualified schema name for domain. +CREATE SCHEMA regress_schema_3 AUTHORIZATION CURRENT_ROLE + create view test as select 'hello'::ss as test + create table t(a ss1) + create domain ss as text not null + create domain ss1 as ss; +\dD regress_schema_3.* + List of domains + Schema | Name | Type | Collation | Nullable | Default | Check +------------------+------+---------------------+-----------+----------+---------+------- + regress_schema_3 | ss | text | | not null | | + regress_schema_3 | ss1 | regress_schema_3.ss | | | | +(2 rows) + +DROP SCHEMA regress_schema_2 CASCADE; +NOTICE: drop cascades to 2 other objects +DETAIL: drop cascades to type regress_schema_2.ss +drop cascades to table regress_schema_2.t +DROP SCHEMA regress_schema_3 CASCADE; +NOTICE: drop cascades to 4 other objects +DETAIL: drop cascades to type regress_schema_3.ss +drop cascades to type regress_schema_3.ss1 +drop cascades to table regress_schema_3.t +drop cascades to view regress_schema_3.test -- Cases where the schema creation succeeds. -- The schema created matches the role name. CREATE SCHEMA AUTHORIZATION regress_create_schema_role diff --git a/src/test/regress/sql/create_schema.sql b/src/test/regress/sql/create_schema.sql index 1b7064247a..421aaa424e 100644 --- a/src/test/regress/sql/create_schema.sql +++ b/src/test/regress/sql/create_schema.sql @@ -47,6 +47,39 @@ CREATE SCHEMA regress_schema_1 AUTHORIZATION CURRENT_ROLE EXECUTE FUNCTION schema_trig.no_func(); RESET ROLE; +-- Cases where the schema creation with domain. + +--fail. cannot create domain to other schema +CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE + create table t(a ss) + create domain public.ss as text not null default 'hello' constraint nn check (value <> 'hello'); +--fail. cannot create domain to other schema +CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE + create table t(a ss) + create domain postgres.public.ss as text not null default 'hello' constraint nn check (value <> 'hello'); + +--fail. forward references, need reorder. +CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE + create domain ss1 as ss + create domain ss as text; + +--ok, qualified schema name for domain should be same as the created schema. +CREATE SCHEMA regress_schema_2 AUTHORIZATION CURRENT_ROLE + create table t(a regress_schema_2.ss) + create domain regress_schema_2.ss as text not null; +\dD regress_schema_2.* + +--ok, no qualified schema name for domain. +CREATE SCHEMA regress_schema_3 AUTHORIZATION CURRENT_ROLE + create view test as select 'hello'::ss as test + create table t(a ss1) + create domain ss as text not null + create domain ss1 as ss; +\dD regress_schema_3.* + +DROP SCHEMA regress_schema_2 CASCADE; +DROP SCHEMA regress_schema_3 CASCADE; + -- Cases where the schema creation succeeds. -- The schema created matches the role name. CREATE SCHEMA AUTHORIZATION regress_create_schema_role -- 2.34.1 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: CREATE SCHEMA ... CREATE DOMAIN support @ 2024-11-29 14:00 Kirill Reshke <[email protected]> parent: jian he <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Kirill Reshke @ 2024-11-29 14:00 UTC (permalink / raw) To: jian he <[email protected]>; +Cc: Tom Lane <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, 29 Nov 2024 at 18:47, jian he <[email protected]> wrote: > > new patch, add tab complete for it. Thank you. You may also be interested in reviewing [0]. [0] https://www.postgresql.org/message-id/CALdSSPhqfvKbDwqJaY%3DyEePi_aq61GmMpW88i6ZH7CMG_2Z4Cg%40mail.g... -- Best regards, Kirill Reshke ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-11-29 14:00 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-11-27 18:39 Re: CREATE SCHEMA ... CREATE DOMAIN support Tom Lane <[email protected]> 2024-11-28 05:27 ` Kirill Reshke <[email protected]> 2024-11-28 05:52 ` Tom Lane <[email protected]> 2024-11-28 09:58 ` Kirill Reshke <[email protected]> 2024-11-29 13:47 ` jian he <[email protected]> 2024-11-29 14:00 ` Kirill Reshke <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox