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
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';
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
