Add LINE: hint when schemaname.typename is a non-existent schema
The attached patch adds a LINE: ... hint when schemaname.typename
results in a schema which does not exist. I came across this when a
missing comma in a SELECT list resulted in an error without a location
in a query a few thousand lines long.
Before:
(postgres@[local]:5432 14:41:25) [postgres]> select test.id 'all' as
example from test;
ERROR: 3F000: schema "test" does not exist
LOCATION: get_namespace_oid, namespace.c:2826
After:
(postgres@[local]:5433 14:42:32) [postgres]> select test.id 'all' as
example from test;
ERROR: 3F000: schema "test" does not exist
LINE 1: select test.id 'all' as example from test;
^
LOCATION: LookupTypeName, parse_type.c:171
-Ryan Kelly
Attachments:
missing_type_schema_hint.patchapplication/octet-stream; name=missing_type_schema_hint.patchDownload
diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index ca5fbed..0e9632c 100644
--- a/src/backend/parser/parse_type.c
+++ b/src/backend/parser/parse_type.c
@@ -157,13 +157,18 @@ LookupTypeName(ParseState *pstate, const TypeName *typeName,
/* Look in specific schema only */
Oid namespaceId;
- namespaceId = LookupExplicitNamespace(schemaname, missing_ok);
+ namespaceId = LookupExplicitNamespace(schemaname, true);
if (OidIsValid(namespaceId))
typoid = GetSysCacheOid2(TYPENAMENSP,
PointerGetDatum(typname),
ObjectIdGetDatum(namespaceId));
- else
+ else if (missing_ok)
typoid = InvalidOid;
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_SCHEMA),
+ errmsg("schema \"%s\" does not exist", schemaname),
+ parser_errposition(pstate, typeName->location)));
}
else
{
On Mon, Feb 2, 2015 at 2:44 PM, Ryan Kelly <rpkelly22@gmail.com> wrote:
The attached patch adds a LINE: ... hint when schemaname.typename
results in a schema which does not exist. I came across this when a
missing comma in a SELECT list resulted in an error without a location
in a query a few thousand lines long.Before:
(postgres@[local]:5432 14:41:25) [postgres]> select test.id 'all' as
example from test;
ERROR: 3F000: schema "test" does not exist
LOCATION: get_namespace_oid, namespace.c:2826After:
(postgres@[local]:5433 14:42:32) [postgres]> select test.id 'all' as
example from test;
ERROR: 3F000: schema "test" does not exist
LINE 1: select test.id 'all' as example from test;
^
LOCATION: LookupTypeName, parse_type.c:171-Ryan Kelly
Please add your patch to
https://commitfest.postgresql.org/action/commitfest_view/open so do we
don't forget about it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Ryan Kelly <rpkelly22@gmail.com> writes:
The attached patch adds a LINE: ... hint when schemaname.typename
results in a schema which does not exist. I came across this when a
missing comma in a SELECT list resulted in an error without a location
in a query a few thousand lines long.
I think this is a good problem to attack, but the proposed patch seems
a bit narrow and brute-force. Surely this isn't the only place where
we're missing an errposition pointer on a "no such schema" error?
It's probably not reasonable to add a pstate argument to
LookupExplicitNamespace itself, given that namespace.c isn't part
of the parser. But you could imagine adding an interface function
in the parser that calls LookupExplicitNamespace and then throws a
position-aware error on failure, and then making sure that all schema
lookups in the parser go through that.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Feb 3, 2015 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ryan Kelly <rpkelly22@gmail.com> writes:
The attached patch adds a LINE: ... hint when schemaname.typename
results in a schema which does not exist. I came across this when a
missing comma in a SELECT list resulted in an error without a location
in a query a few thousand lines long.I think this is a good problem to attack, but the proposed patch seems
a bit narrow and brute-force. Surely this isn't the only place where
we're missing an errposition pointer on a "no such schema" error?
I managed to find four code paths that failed to report an error position if
the schema did not exist:
- Schema-qualified types (e.g. select schemaname.typename 'foo')
- Schema-qualified operators (e.g. select 1 operator(schemaname.+) 1)
- Schema-qualified table names in CREATE TABLE (e.g. create table
schemaname.tablename (colname integer))
- Schema-qualified function calls (e.g. select schemaname.funcname())
It's probably not reasonable to add a pstate argument to
LookupExplicitNamespace itself, given that namespace.c isn't part
of the parser. But you could imagine adding an interface function
in the parser that calls LookupExplicitNamespace and then throws a
position-aware error on failure, and then making sure that all schema
lookups in the parser go through that.
While searching for other instances of this problem, I found that using
a schema that does not exist in conjunction with a collation reported the
position. That code uses setup_parser_errposition_callback and the
documentation for that function seems to indicate that the usage of it in my
newly attached patch are consistent. I do not know, however, if this is the
cleanest approach or if I should actually create a function like
validate_explicit_namespace to handle this (and I'm also not sure where
such a function should live, if I do create it).
-Ryan Kelly
Attachments:
missing_type_schema_hint_v2.patchapplication/octet-stream; name=missing_type_schema_hint_v2.patchDownload
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index a200804..e0a0980 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -93,6 +93,7 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
Oid vatype;
FuncDetailCode fdresult;
char aggkind = 0;
+ ParseCallbackState pcbstate;
/*
* If there's an aggregate filter, transform it using transformWhereClause
@@ -235,12 +236,18 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
* type. We'll fix up the variadic case below. We may also have to deal
* with default arguments.
*/
+
+ setup_parser_errposition_callback(&pcbstate, pstate, location);
+
fdresult = func_get_detail(funcname, fargs, argnames, nargs,
actual_arg_types,
!func_variadic, true,
&funcid, &rettype, &retset,
&nvargs, &vatype,
&declared_arg_types, &argdefaults);
+
+ cancel_parser_errposition_callback(&pcbstate);
+
if (fdresult == FUNCDETAIL_COERCION)
{
/*
diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c
index 10de97b..0e2b228 100644
--- a/src/backend/parser/parse_oper.c
+++ b/src/backend/parser/parse_oper.c
@@ -379,11 +379,17 @@ oper(ParseState *pstate, List *opname, Oid ltypeId, Oid rtypeId,
bool key_ok;
FuncDetailCode fdresult = FUNCDETAIL_NOTFOUND;
HeapTuple tup = NULL;
+ ParseCallbackState pcbstate;
/*
- * Try to find the mapping in the lookaside cache.
+ * Try to find the mapping in the lookaside cache. since this call will
+ * fail if the operator is schema-qualified and the schema does not exist,
+ * we will not check again below (e.g., around OpernameGetCandidates).
*/
+ setup_parser_errposition_callback(&pcbstate, pstate, location);
key_ok = make_oper_cache_key(&key, opname, ltypeId, rtypeId);
+ cancel_parser_errposition_callback(&pcbstate);
+
if (key_ok)
{
operOid = find_oper_cache_entry(&key);
@@ -525,11 +531,17 @@ right_oper(ParseState *pstate, List *op, Oid arg, bool noError, int location)
bool key_ok;
FuncDetailCode fdresult = FUNCDETAIL_NOTFOUND;
HeapTuple tup = NULL;
+ ParseCallbackState pcbstate;
/*
- * Try to find the mapping in the lookaside cache.
+ * Try to find the mapping in the lookaside cache. since this call will
+ * fail if the operator is schema-qualified and the schema does not exist,
+ * we will not check again below (e.g., around OpernameGetCandidates).
*/
+ setup_parser_errposition_callback(&pcbstate, pstate, location);
key_ok = make_oper_cache_key(&key, op, arg, InvalidOid);
+ cancel_parser_errposition_callback(&pcbstate);
+
if (key_ok)
{
operOid = find_oper_cache_entry(&key);
@@ -603,11 +615,17 @@ left_oper(ParseState *pstate, List *op, Oid arg, bool noError, int location)
bool key_ok;
FuncDetailCode fdresult = FUNCDETAIL_NOTFOUND;
HeapTuple tup = NULL;
+ ParseCallbackState pcbstate;
/*
- * Try to find the mapping in the lookaside cache.
+ * Try to find the mapping in the lookaside cache. since this call will
+ * fail if the operator is schema-qualified and the schema does not exist,
+ * we will not check again below (e.g., around OpernameGetCandidates).
*/
+ setup_parser_errposition_callback(&pcbstate, pstate, location);
key_ok = make_oper_cache_key(&key, op, InvalidOid, arg);
+ cancel_parser_errposition_callback(&pcbstate);
+
if (key_ok)
{
operOid = find_oper_cache_entry(&key);
diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index ca5fbed..1ba6ca7 100644
--- a/src/backend/parser/parse_type.c
+++ b/src/backend/parser/parse_type.c
@@ -156,6 +156,9 @@ LookupTypeName(ParseState *pstate, const TypeName *typeName,
{
/* Look in specific schema only */
Oid namespaceId;
+ ParseCallbackState pcbstate;
+
+ setup_parser_errposition_callback(&pcbstate, pstate, typeName->location);
namespaceId = LookupExplicitNamespace(schemaname, missing_ok);
if (OidIsValid(namespaceId))
@@ -164,6 +167,8 @@ LookupTypeName(ParseState *pstate, const TypeName *typeName,
ObjectIdGetDatum(namespaceId));
else
typoid = InvalidOid;
+
+ cancel_parser_errposition_callback(&pcbstate);
}
else
{
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 3ccdbb7..6098be4 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -149,6 +149,7 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
ListCell *elements;
Oid namespaceid;
Oid existing_relid;
+ ParseCallbackState pcbstate;
/*
* We must not scribble on the passed-in CreateStmt, so copy it. (This is
@@ -156,40 +157,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
*/
stmt = (CreateStmt *) copyObject(stmt);
- /*
- * Look up the creation namespace. This also checks permissions on the
- * target namespace, locks it against concurrent drops, checks for a
- * preexisting relation in that namespace with the same name, and updates
- * stmt->relation->relpersistence if the select namespace is temporary.
- */
- namespaceid =
- RangeVarGetAndCheckCreationNamespace(stmt->relation, NoLock,
- &existing_relid);
-
- /*
- * If the relation already exists and the user specified "IF NOT EXISTS",
- * bail out with a NOTICE.
- */
- if (stmt->if_not_exists && OidIsValid(existing_relid))
- {
- ereport(NOTICE,
- (errcode(ERRCODE_DUPLICATE_TABLE),
- errmsg("relation \"%s\" already exists, skipping",
- stmt->relation->relname)));
- return NIL;
- }
-
- /*
- * If the target relation name isn't schema-qualified, make it so. This
- * prevents some corner cases in which added-on rewritten commands might
- * think they should apply to other relations that have the same name and
- * are earlier in the search path. But a local temp table is effectively
- * specified to be in pg_temp, so no need for anything extra in that case.
- */
- if (stmt->relation->schemaname == NULL
- && stmt->relation->relpersistence != RELPERSISTENCE_TEMP)
- stmt->relation->schemaname = get_namespace_name(namespaceid);
-
/* Set up pstate and CreateStmtContext */
pstate = make_parsestate(NULL);
pstate->p_sourcetext = queryString;
@@ -225,6 +192,44 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
transformOfType(&cxt, stmt->ofTypename);
/*
+ * Look up the creation namespace. This also checks permissions on the
+ * target namespace, locks it against concurrent drops, checks for a
+ * preexisting relation in that namespace with the same name, and updates
+ * stmt->relation->relpersistence if the select namespace is temporary.
+ */
+ setup_parser_errposition_callback(&pcbstate, pstate, stmt->relation->location);
+
+ namespaceid =
+ RangeVarGetAndCheckCreationNamespace(stmt->relation, NoLock,
+ &existing_relid);
+
+ cancel_parser_errposition_callback(&pcbstate);
+
+ /*
+ * If the relation already exists and the user specified "IF NOT EXISTS",
+ * bail out with a NOTICE.
+ */
+ if (stmt->if_not_exists && OidIsValid(existing_relid))
+ {
+ ereport(NOTICE,
+ (errcode(ERRCODE_DUPLICATE_TABLE),
+ errmsg("relation \"%s\" already exists, skipping",
+ stmt->relation->relname)));
+ return NIL;
+ }
+
+ /*
+ * If the target relation name isn't schema-qualified, make it so. This
+ * prevents some corner cases in which added-on rewritten commands might
+ * think they should apply to other relations that have the same name and
+ * are earlier in the search path. But a local temp table is effectively
+ * specified to be in pg_temp, so no need for anything extra in that case.
+ */
+ if (stmt->relation->schemaname == NULL
+ && stmt->relation->relpersistence != RELPERSISTENCE_TEMP)
+ stmt->relation->schemaname = get_namespace_name(namespaceid);
+
+ /*
* Run through each primary element in the table creation clause. Separate
* column defs from constraints, and do preliminary analysis.
*/
Ryan Kelly <rpkelly22@gmail.com> writes:
On Tue, Feb 3, 2015 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It's probably not reasonable to add a pstate argument to
LookupExplicitNamespace itself, given that namespace.c isn't part
of the parser. But you could imagine adding an interface function
in the parser that calls LookupExplicitNamespace and then throws a
position-aware error on failure, and then making sure that all schema
lookups in the parser go through that.
While searching for other instances of this problem, I found that using
a schema that does not exist in conjunction with a collation reported the
position. That code uses setup_parser_errposition_callback and the
documentation for that function seems to indicate that the usage of it in my
newly attached patch are consistent. I do not know, however, if this is the
cleanest approach or if I should actually create a function like
validate_explicit_namespace to handle this (and I'm also not sure where
such a function should live, if I do create it).
Yeah, setup_parser_errposition_callback would work too. I'm not sure
offhand which I like better. One thing to keep in mind is that the
callback approach results in adding an error cursor position to *any*
error thrown while the callback is active, not only "schema not found".
There are pluses and minuses to that. I've seen error cursors attached
to very bizarre internal problems that (more or less by chance) showed up
while the parser was looking up a table name, but weren't really connected
to the table name at all. OTOH, most of the time you'd just as soon not
be too picky about what conditions you provide a cursor for.
The main place I'd question what you did is the callback placement around
make_oper_cache_key --- that might work, but it seems very bizarre, and
perhaps more than usually prone to the "cursor given for unrelated
problem" issue. Perhaps better to push it down inside that function
so it surrounds just the namespace lookup call.
Also the diffs in parse_utilcmd.c are very confusing and seem to change
more code than is necessary --- why did that happen?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, failed
Spec compliant: not tested
Documentation: not tested
Tom suggested few changes already which I too think author needs to address.
So marking it "Waiting on Author".
However, I see following, example does not work well.
postgres=# create or replace function
f1(a abc.test.id%type) returns int as
$$ select 1; $$
language sql;
ERROR: schema "abc" does not exist
Is that expected?
I guess we need it at all places in parse_*.c where we will look for namespace.
Please fix.
Also, like Tom's suggestion on make_oper_cache_key, can we push down this
inside func_get_detail() as well, just to limit it for namespace lookup?
However, patch is not getting applied cleanly on latest sources. Need rebase.
On Tom comments on parse_utilcmd.c:
I guess the block is moved after the pstate and CreateStmtContext are setup
properly. I guess, we can move just after pstate setup, so that it will
result into minimal changes?
Can we have small test-case? Or will it be too much for this feature?
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
In the interest of moving forward, I have updated this patch because
Ryan has been inactive for over a month now.
Tom Lane wrote:
Yeah, setup_parser_errposition_callback would work too. I'm not sure
offhand which I like better. One thing to keep in mind is that the
callback approach results in adding an error cursor position to *any*
error thrown while the callback is active, not only "schema not found".
There are pluses and minuses to that. I've seen error cursors attached
to very bizarre internal problems that (more or less by chance) showed up
while the parser was looking up a table name, but weren't really connected
to the table name at all. OTOH, most of the time you'd just as soon not
be too picky about what conditions you provide a cursor for.
I think we can live with cursor positions in some weird corner cases.
If we later find out that we don't like it for some reason, we can
reduce the scope that this applies to.
The main place I'd question what you did is the callback placement around
make_oper_cache_key --- that might work, but it seems very bizarre, and
perhaps more than usually prone to the "cursor given for unrelated
problem" issue. Perhaps better to push it down inside that function
so it surrounds just the namespace lookup call.
Agreed; the attached patch does it that way. (I notice that we have the
pstate as first arg in many places; I put at the end for
make_oper_cache_key, together with location. Is there some convention
to have it as first arg?)
Also the diffs in parse_utilcmd.c are very confusing and seem to change
more code than is necessary --- why did that happen?
The reason appears to be that Ryan wanted to have the pstate set, but
that was only created after looking other things up, so he moved a
largish block down; this was pretty bogus AFAICT. The attached patch
fixes this by first creating the pstate, then doing the namespace
lookup, then doing the rest of the setup.
It's a bit disappointing to see so little changes in regression expected
output ...
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
missing_type_schema_hint_v3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 53bbaec..1385776 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -93,6 +93,7 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
Oid vatype;
FuncDetailCode fdresult;
char aggkind = 0;
+ ParseCallbackState pcbstate;
/*
* If there's an aggregate filter, transform it using transformWhereClause
@@ -235,12 +236,18 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
* type. We'll fix up the variadic case below. We may also have to deal
* with default arguments.
*/
+
+ setup_parser_errposition_callback(&pcbstate, pstate, location);
+
fdresult = func_get_detail(funcname, fargs, argnames, nargs,
actual_arg_types,
!func_variadic, true,
&funcid, &rettype, &retset,
&nvargs, &vatype,
&declared_arg_types, &argdefaults);
+
+ cancel_parser_errposition_callback(&pcbstate);
+
if (fdresult == FUNCDETAIL_COERCION)
{
/*
diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c
index 10de97b..f0786bd 100644
--- a/src/backend/parser/parse_oper.c
+++ b/src/backend/parser/parse_oper.c
@@ -76,7 +76,8 @@ static void op_error(ParseState *pstate, List *op, char oprkind,
Oid arg1, Oid arg2,
FuncDetailCode fdresult, int location);
static bool make_oper_cache_key(OprCacheKey *key, List *opname,
- Oid ltypeId, Oid rtypeId);
+ Oid ltypeId, Oid rtypeId,
+ ParseState *pstate, int location);
static Oid find_oper_cache_entry(OprCacheKey *key);
static void make_oper_cache_entry(OprCacheKey *key, Oid opr_oid);
static void InvalidateOprCacheCallBack(Datum arg, int cacheid, uint32 hashvalue);
@@ -383,7 +384,8 @@ oper(ParseState *pstate, List *opname, Oid ltypeId, Oid rtypeId,
/*
* Try to find the mapping in the lookaside cache.
*/
- key_ok = make_oper_cache_key(&key, opname, ltypeId, rtypeId);
+ key_ok = make_oper_cache_key(&key, opname, ltypeId, rtypeId, pstate, location);
+
if (key_ok)
{
operOid = find_oper_cache_entry(&key);
@@ -529,7 +531,8 @@ right_oper(ParseState *pstate, List *op, Oid arg, bool noError, int location)
/*
* Try to find the mapping in the lookaside cache.
*/
- key_ok = make_oper_cache_key(&key, op, arg, InvalidOid);
+ key_ok = make_oper_cache_key(&key, op, arg, InvalidOid, pstate, location);
+
if (key_ok)
{
operOid = find_oper_cache_entry(&key);
@@ -607,7 +610,8 @@ left_oper(ParseState *pstate, List *op, Oid arg, bool noError, int location)
/*
* Try to find the mapping in the lookaside cache.
*/
- key_ok = make_oper_cache_key(&key, op, InvalidOid, arg);
+ key_ok = make_oper_cache_key(&key, op, InvalidOid, arg, pstate, location);
+
if (key_ok)
{
operOid = find_oper_cache_entry(&key);
@@ -1006,9 +1010,13 @@ static HTAB *OprCacheHash = NULL;
*
* Returns TRUE if successful, FALSE if the search_path overflowed
* (hence no caching is possible).
+ *
+ * pstate/location are used only to report the error position; pass NULL/-1
+ * if not available.
*/
static bool
-make_oper_cache_key(OprCacheKey *key, List *opname, Oid ltypeId, Oid rtypeId)
+make_oper_cache_key(OprCacheKey *key, List *opname, Oid ltypeId, Oid rtypeId,
+ ParseState *pstate, int location)
{
char *schemaname;
char *opername;
@@ -1026,8 +1034,12 @@ make_oper_cache_key(OprCacheKey *key, List *opname, Oid ltypeId, Oid rtypeId)
if (schemaname)
{
+ ParseCallbackState pcbstate;
+
/* search only in exact schema given */
+ setup_parser_errposition_callback(&pcbstate, pstate, location);
key->search_path[0] = LookupExplicitNamespace(schemaname, false);
+ cancel_parser_errposition_callback(&pcbstate);
}
else
{
diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index ca5fbed..1ba6ca7 100644
--- a/src/backend/parser/parse_type.c
+++ b/src/backend/parser/parse_type.c
@@ -156,6 +156,9 @@ LookupTypeName(ParseState *pstate, const TypeName *typeName,
{
/* Look in specific schema only */
Oid namespaceId;
+ ParseCallbackState pcbstate;
+
+ setup_parser_errposition_callback(&pcbstate, pstate, typeName->location);
namespaceId = LookupExplicitNamespace(schemaname, missing_ok);
if (OidIsValid(namespaceId))
@@ -164,6 +167,8 @@ LookupTypeName(ParseState *pstate, const TypeName *typeName,
ObjectIdGetDatum(namespaceId));
else
typoid = InvalidOid;
+
+ cancel_parser_errposition_callback(&pcbstate);
}
else
{
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 1e6da9c..1bbed95 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -149,6 +149,7 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
ListCell *elements;
Oid namespaceid;
Oid existing_relid;
+ ParseCallbackState pcbstate;
/*
* We must not scribble on the passed-in CreateStmt, so copy it. (This is
@@ -156,15 +157,22 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
*/
stmt = (CreateStmt *) copyObject(stmt);
+ /* Set up pstate */
+ pstate = make_parsestate(NULL);
+ pstate->p_sourcetext = queryString;
+
/*
* Look up the creation namespace. This also checks permissions on the
* target namespace, locks it against concurrent drops, checks for a
* preexisting relation in that namespace with the same name, and updates
* stmt->relation->relpersistence if the selected namespace is temporary.
*/
+ setup_parser_errposition_callback(&pcbstate, pstate,
+ stmt->relation->location);
namespaceid =
RangeVarGetAndCheckCreationNamespace(stmt->relation, NoLock,
&existing_relid);
+ cancel_parser_errposition_callback(&pcbstate);
/*
* If the relation already exists and the user specified "IF NOT EXISTS",
@@ -190,10 +198,7 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
&& stmt->relation->relpersistence != RELPERSISTENCE_TEMP)
stmt->relation->schemaname = get_namespace_name(namespaceid);
- /* Set up pstate and CreateStmtContext */
- pstate = make_parsestate(NULL);
- pstate->p_sourcetext = queryString;
-
+ /* Set up CreateStmtContext */
cxt.pstate = pstate;
if (IsA(stmt, CreateForeignTableStmt))
{
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 35451d5..34b5fc1 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -212,11 +212,15 @@ INSERT INTO unlogged1 VALUES (42);
CREATE UNLOGGED TABLE public.unlogged2 (a int primary key); -- also OK
CREATE UNLOGGED TABLE pg_temp.unlogged3 (a int primary key); -- not OK
ERROR: only temporary relations may be created in temporary schemas
+LINE 1: CREATE UNLOGGED TABLE pg_temp.unlogged3 (a int primary key);
+ ^
CREATE TABLE pg_temp.implicitly_temp (a int primary key); -- OK
CREATE TEMP TABLE explicitly_temp (a int primary key); -- also OK
CREATE TEMP TABLE pg_temp.doubly_temp (a int primary key); -- also OK
CREATE TEMP TABLE public.temp_to_perm (a int primary key); -- not OK
ERROR: cannot create temporary relation in non-temporary schema
+LINE 1: CREATE TEMP TABLE public.temp_to_perm (a int primary key);
+ ^
DROP TABLE unlogged1, public.unlogged2;
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Agreed; the attached patch does it that way. (I notice that we have the
pstate as first arg in many places; I put at the end for
make_oper_cache_key, together with location. Is there some convention
to have it as first arg?)
Yes, parser-related functions always have pstate as first arg if they
need it at all. Please follow that convention.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Álvaro,
I think, there are few open questions here and thus marking it back to "Waiting on Author".
Please have your views on the review comments already posted.
Also make changes as Tom suggested about placing pstate at the beginning.
I am more concerned about this:
1.
postgres=# create or replace function
f1(a abc.test.id%type) returns int as
$$ select 1; $$
language sql;
ERROR: schema "abc" does not exist
Is that expected?
2.
Also what about pushing setup_parser_errposition_callback() inside func_get_detail() as well, just to limit it for namespace lookup?
Thanks
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jeevan Chalke wrote:
�lvaro,
I think, there are few open questions here and thus marking it back to "Waiting on Author".
Please have your views on the review comments already posted.
For some reason I missed your previous email.
Also make changes as Tom suggested about placing pstate at the beginning.
Sure.
1.
postgres=# create or replace function
f1(a abc.test.id%type) returns int as
$$ select 1; $$
language sql;
ERROR: schema "abc" does not existIs that expected?
Type resolution for function arguments happens at execution time,
in interpret_function_parameter_list() as called by CreateFunction().
We don't have a pstate at that point which is why location is not
reported. Fixing that seems like a whole new project.
2.
Also what about pushing setup_parser_errposition_callback() inside func_get_detail() as well, just to limit it for namespace lookup?
Hm, that sounds more reasonable. Actually it means we have to drill
all the way down to FuncnameGetCandidates.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jeevan Chalke <jeevan.chalke@gmail.com> writes:
I am more concerned about this:
1.
postgres=# create or replace function
f1(a abc.test.id%type) returns int as
$$ select 1; $$
language sql;
ERROR: schema "abc" does not exist
Is that expected?
Yes, or at least, if it's not what we want it's not this patch's fault.
That behavior is pre-existing.
Also what about pushing setup_parser_errposition_callback() inside func_get_detail() as well, just to limit it for namespace lookup?
Adding a pstate parameter to func_get_detail would be rather invasive;
not sure it's worth it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jeevan,
Thanks for the review.
Jeevan Chalke wrote:
I think, there are few open questions here and thus marking it back to "Waiting on Author".
Please have your views on the review comments already posted.
Also make changes as Tom suggested about placing pstate at the beginning.
Pushed with that one change.
postgres=# create or replace function
f1(a abc.test.id%type) returns int as
$$ select 1; $$
language sql;
ERROR: schema "abc" does not existIs that expected?
We can leave this for a future patch.
2.
Also what about pushing setup_parser_errposition_callback() inside func_get_detail() as well, just to limit it for namespace lookup?
Seemed messy enough that I left this as-is; and as Tom said elsewhere, I
think it's okay to have cursor position in other errors too. At the
very least, the user will know for certain what's the function being
processed that caused whatever failure.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers