Обсуждение: sqlsmith: ERROR: XX000: bogus varno: 2
I reduced the problematic query to this.
SELECT 1 FROM pg_rewrite WHERE
pg_get_function_arg_default(ev_class, 1) !~~
pg_get_expr(ev_qual, ev_class, false);
#0 pg_re_throw () at elog.c:1800
#1 0x0000563f5d027932 in errfinish () at elog.c:593
#2 0x0000563f5cb874ee in resolve_special_varno (node=0x563f5dd0f7e0, context=0x7ffcf0daf250, callback=0x563f5cfca270
<get_special_variable>,callback_arg=0x0) at ruleutils.c:7319
#3 0x0000563f5cfca044 in get_variable () at ruleutils.c:7086
#4 0x0000563f5cfc7c58 in get_rule_expr () at ruleutils.c:8363
#5 0x0000563f5cfc97a6 in get_oper_expr (context=0x7ffcf0daf250, expr=0x563f5dd0f6f0) at ruleutils.c:9626
#6 get_rule_expr () at ruleutils.c:8472
#7 0x0000563f5cfcdc37 in deparse_expression_pretty (expr=expr@entry=0x563f5dd0f6f0, dpcontext=0x563f5dd10488,
forceprefix=forceprefix@entry=false,showimplicit=showimplicit@entry=false,
prettyFlags=prettyFlags@entry=2, startIndent=0) at ruleutils.c:3558
#8 0x0000563f5cfce661 in pg_get_expr_worker (expr=<optimized out>, relid=12104, relname=0x563f5dd10130 "pg_settings",
prettyFlags=2)at ruleutils.c:2645
#9 0x0000563f5cd6540b in ExecInterpExpr () at execExprInterp.c:1272
#10 0x0000563f5cd73c5f in ExecEvalExprSwitchContext (isNull=0x7ffcf0daf3a7, econtext=0x563f5dd08a00,
state=0x563f5dd0a270)at ../../../src/include/executor/executor.h:339
#11 ExecQual (econtext=0x563f5dd08a00, state=0x563f5dd0a270) at ../../../src/include/executor/executor.h:408
#12 ExecScan (node=0x563f5dd09328, accessMtd=0x563f5cd9e790 <SeqNext>, recheckMtd=0x563f5cd9e780 <SeqRecheck>) at
execScan.c:227
#13 0x0000563f5cd69f73 in ExecProcNode (node=0x563f5dd09328) at ../../../src/include/executor/executor.h:257
#14 ExecutePlan (execute_once=<optimized out>, dest=0x563f5dd18a80, direction=<optimized out>, numberTuples=0,
sendTuples=<optimizedout>, operation=CMD_SELECT,
use_parallel_mode=<optimized out>, planstate=0x563f5dd09328, estate=0x563f5dd08790) at execMain.c:1600
#15 standard_ExecutorRun () at execMain.c:410
#16 0x0000563f5cf0460f in PortalRunSelect () at pquery.c:924
#17 0x0000563f5cf05bf1 in PortalRun () at pquery.c:768
#18 0x0000563f5cf019b2 in exec_simple_query () at postgres.c:1215
#19 0x0000563f5cf0370a in PostgresMain () at postgres.c:4498
#20 0x0000563f5ce6e479 in BackendRun (port=<optimized out>, port=<optimized out>) at postmaster.c:4594
#21 BackendStartup (port=<optimized out>) at postmaster.c:4322
#22 ServerLoop () at postmaster.c:1802
#23 0x0000563f5ce6f47c in PostmasterMain () at postmaster.c:1474
#24 0x0000563f5cb9a0c0 in main (argc=5, argv=0x563f5dc653f0) at main.c:198
While reducing the query, I got a related error:
SELECT 1 FROM pg_rewrite WHERE
pg_get_function_arg_default(ev_class, 1) !~~
pg_get_expr(ev_qual, 0, false);
ERROR: XX000: bogus varlevelsup: 0 offset 0
LOCATION: get_variable, ruleutils.c:7003
Both errors are reproducible back to at least v10.
Justin Pryzby <pryzby@telsasoft.com> writes:
> I reduced the problematic query to this.
> SELECT 1 FROM pg_rewrite WHERE
> pg_get_function_arg_default(ev_class, 1) !~~
> pg_get_expr(ev_qual, ev_class, false);
Or more simply,
regression=# select pg_get_expr(ev_qual, ev_class, false) from pg_rewrite where rulename = 'pg_settings_u';
ERROR: bogus varno: 2
I don't see anything particularly surprising here. pg_get_expr is only
able to cope with expression trees over a single relation, but ON UPDATE
rules can refer to both OLD and NEW relations. Maybe we could make the
error message more friendly, but there's not much else to be done,
I think.
regards, tom lane
On Sun, Dec 19, 2021 at 4:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Justin Pryzby <pryzby@telsasoft.com> writes: > > I reduced the problematic query to this. > > SELECT 1 FROM pg_rewrite WHERE > > pg_get_function_arg_default(ev_class, 1) !~~ > > pg_get_expr(ev_qual, ev_class, false); > > Or more simply, > > regression=# select pg_get_expr(ev_qual, ev_class, false) from pg_rewrite where rulename = 'pg_settings_u'; > ERROR: bogus varno: 2 > > I don't see anything particularly surprising here. pg_get_expr is only > able to cope with expression trees over a single relation, but ON UPDATE > rules can refer to both OLD and NEW relations. Maybe we could make the > error message more friendly, but there's not much else to be done, > I think. +1 for making the error message more friendly. (We would certainly have a difficult time making it less friendly.) -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Dec 19, 2021 at 4:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't see anything particularly surprising here. pg_get_expr is only
>> able to cope with expression trees over a single relation, but ON UPDATE
>> rules can refer to both OLD and NEW relations. Maybe we could make the
>> error message more friendly, but there's not much else to be done,
>> I think.
> +1 for making the error message more friendly.
The problem is that the spot where it's thrown doesn't have a lot of
context. We can fix that by having pg_get_expr itself check for
out-of-spec Vars before starting the recursion, which adds a bit of
overhead but I don't think we're terribly concerned about that.
I figured this would be just a quick hack in ruleutils.c, but was
dismayed to find the regression tests falling over, because some
bozo neglected to teach nodeFuncs.c about partition expressions.
It might be a good idea to back-patch that part, before we find
some other place that fails.
regards, tom lane
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index e276264882..5d4700430c 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2201,6 +2201,26 @@ expression_tree_walker(Node *node,
return true;
}
break;
+ case T_PartitionBoundSpec:
+ {
+ PartitionBoundSpec *pbs = (PartitionBoundSpec *) node;
+
+ if (walker(pbs->listdatums, context))
+ return true;
+ if (walker(pbs->lowerdatums, context))
+ return true;
+ if (walker(pbs->upperdatums, context))
+ return true;
+ }
+ break;
+ case T_PartitionRangeDatum:
+ {
+ PartitionRangeDatum *prd = (PartitionRangeDatum *) node;
+
+ if (walker(prd->value, context))
+ return true;
+ }
+ break;
case T_List:
foreach(temp, (List *) node)
{
@@ -3092,6 +3112,28 @@ expression_tree_mutator(Node *node,
return (Node *) newnode;
}
break;
+ case T_PartitionBoundSpec:
+ {
+ PartitionBoundSpec *pbs = (PartitionBoundSpec *) node;
+ PartitionBoundSpec *newnode;
+
+ FLATCOPY(newnode, pbs, PartitionBoundSpec);
+ MUTATE(newnode->listdatums, pbs->listdatums, List *);
+ MUTATE(newnode->lowerdatums, pbs->lowerdatums, List *);
+ MUTATE(newnode->upperdatums, pbs->upperdatums, List *);
+ return (Node *) newnode;
+ }
+ break;
+ case T_PartitionRangeDatum:
+ {
+ PartitionRangeDatum *prd = (PartitionRangeDatum *) node;
+ PartitionRangeDatum *newnode;
+
+ FLATCOPY(newnode, prd, PartitionRangeDatum);
+ MUTATE(newnode->value, prd->value, Node *);
+ return (Node *) newnode;
+ }
+ break;
case T_List:
{
/*
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 8da525c715..5d9596d445 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2622,6 +2625,7 @@ static text *
pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
{
Node *node;
+ Relids relids;
List *context;
char *exprstr;
char *str;
@@ -2634,6 +2638,27 @@ pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
pfree(exprstr);
+ /*
+ * Throw a user-friendly error if the expression contains Vars we won't be
+ * able to deparse. pull_varnos requires a PlannerInfo, but fortunately
+ * that can be dummy -- it need only contain an empty placeholder_list.
+ */
+ relids = pull_varnos(makeNode(PlannerInfo), node);
+ if (OidIsValid(relid))
+ {
+ if (!bms_is_subset(relids, bms_make_singleton(1)))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("expression contains variables of more than one relation")));
+ }
+ else
+ {
+ if (!bms_is_empty(relids))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("expression contains variables")));
+ }
+
/* Prepare deparse context if needed */
if (OidIsValid(relid))
context = deparse_context_for(relname, relid);
On Mon, Dec 20, 2021 at 11:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I figured this would be just a quick hack in ruleutils.c, but was > dismayed to find the regression tests falling over, because some > bozo neglected to teach nodeFuncs.c about partition expressions. > It might be a good idea to back-patch that part, before we find > some other place that fails. Calling people bozos isn't very nice. Please don't do that. The commit that added PartitionBoundSpec and PartitionRangeDatum was committed by me and authored by Amit Langote. It is the original table partitioning commit -- f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63. I'm reasonably sure that the reason why those didn't get added to expression_tree_walker is that they don't seem like something that can ever appear in an expression. I still don't understand why that's not true. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> The commit that added PartitionBoundSpec and PartitionRangeDatum was
> committed by me and authored by Amit Langote. It is the original table
> partitioning commit -- f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63. I'm
> reasonably sure that the reason why those didn't get added to
> expression_tree_walker is that they don't seem like something that can
> ever appear in an expression. I still don't understand why that's not
> true.
The reason the regression tests fail if I only patch ruleutils is
that psql \d on a partitioned table invokes
... pg_get_expr(c.relpartbound, c.oid) FROM pg_catalog.pg_class c
and evidently relpartbound does contain precisely these node types.
regards, tom lane
On Mon, Dec 20, 2021 at 1:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > The reason the regression tests fail if I only patch ruleutils is > that psql \d on a partitioned table invokes > ... pg_get_expr(c.relpartbound, c.oid) FROM pg_catalog.pg_class c > and evidently relpartbound does contain precisely these node types. Right. I'm not surprised that relpartbound uses those node types. I *am* surprised that pg_get_expr() is expected to be able to handle them. IOW, they ARE node trees, consonant with the fact that the column type is pg_node_tree, but they're NOT expressions. If we're going to have a policy that all node types stored in the catalog should be supported by expression_tree_walker even if they're not actually expressions, we ought to have a rather explicit comment about that in the comments for expression_tree_walker, because otherwise somebody might easily make this same mistake again. Alternatively, maybe pg_get_expr() should just fail and tell you that this is not an expression, and if you want to see what's in that column, you should use the SQL-callable functions specifically provided for that purpose (pg_get_partkeydef, I think). I don't know why it should be legitimate for pg_get_expr() to just assume that any random node tree it gets handed must be an expression without doing any sanity checking. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> Right. I'm not surprised that relpartbound uses those node types. I
> *am* surprised that pg_get_expr() is expected to be able to handle
> them. IOW, they ARE node trees, consonant with the fact that the
> column type is pg_node_tree, but they're NOT expressions.
I'm not sure why you're astonished by that, considering that
psql has applied pg_get_expr to relpartbound since f0e44751d,
which was the same commit that put code into ruleutils.c to
make pg_get_expr work on relpartbounds.
It seems a bit late to change our minds on this; and anyway,
if pg_get_expr didn't handle them, we'd just need to invent
another function that did.
> Alternatively, maybe pg_get_expr() should just fail and tell you that
> this is not an expression, and if you want to see what's in that
> column, you should use the SQL-callable functions specifically
> provided for that purpose (pg_get_partkeydef, I think).
pg_get_partkeydef does something different.
regression=# select pg_get_expr(relpartbound,oid) from pg_class where relname = 'beta_neg';
pg_get_expr
----------------------------------
FOR VALUES FROM ('-10') TO ('0')
(1 row)
regression=# select pg_get_partkeydef('beta_neg'::regclass);
pg_get_partkeydef
-------------------
RANGE (b)
(1 row)
> I don't know
> why it should be legitimate for pg_get_expr() to just assume that any
> random node tree it gets handed must be an expression without doing
> any sanity checking.
It does fall over if you try to apply it to stored rules:
regression=# select pg_get_expr(ev_action, 0) from pg_rewrite;
ERROR: unrecognized node type: 232
I'm not terribly excited about that, but maybe we should try to
improve it while we're here.
regards, tom lane
On Mon, Dec 20, 2021 at 2:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm not sure why you're astonished by that, considering that
> psql has applied pg_get_expr to relpartbound since f0e44751d,
> which was the same commit that put code into ruleutils.c to
> make pg_get_expr work on relpartbounds.
>
> It seems a bit late to change our minds on this; and anyway,
> if pg_get_expr didn't handle them, we'd just need to invent
> another function that did.
OK.
> > Alternatively, maybe pg_get_expr() should just fail and tell you that
> > this is not an expression, and if you want to see what's in that
> > column, you should use the SQL-callable functions specifically
> > provided for that purpose (pg_get_partkeydef, I think).
>
> pg_get_partkeydef does something different.
>
> regression=# select pg_get_expr(relpartbound,oid) from pg_class where relname = 'beta_neg';
> pg_get_expr
> ----------------------------------
> FOR VALUES FROM ('-10') TO ('0')
> (1 row)
>
> regression=# select pg_get_partkeydef('beta_neg'::regclass);
> pg_get_partkeydef
> -------------------
> RANGE (b)
> (1 row)
OK ... but my point is that dump and restore does work. So whatever
cases pg_get_expr() doesn't work must be cases that aren't needed for
that to happen. Otherwise this problem would have been found long ago.
> > I don't know
> > why it should be legitimate for pg_get_expr() to just assume that any
> > random node tree it gets handed must be an expression without doing
> > any sanity checking.
>
> It does fall over if you try to apply it to stored rules:
>
> regression=# select pg_get_expr(ev_action, 0) from pg_rewrite;
> ERROR: unrecognized node type: 232
>
> I'm not terribly excited about that, but maybe we should try to
> improve it while we're here.
In my view, the lack of excitement about sanity checks in functions
that deal with node trees in the catalogs is the root of this problem.
I realize that's a deep hole out of which we're unlikely to be able to
climb in the short or even medium term, but we don't have to keep
digging. We either make a rule that pg_get_expr() can apply to
everything stored in the catalogs and produce sensible answers, which
seems to be what you prefer, or we make it return nice errors for the
cases that it can't handle nicely, or some combination of the two. And
whatever we decide, we also document and enforce everywhere.
I don't think it's any more correct for pg_get_expr() to elog(ERROR,
"some internal thing") than it would be for to_timestamp() or
date_bin() or whatever to do something similar. And I think that
careful thinking about supported cases makes life easier for both
users (who know that if they see some junk error report, it's a
mistake rather than intentional) and for developers (who then have a
better chance of knowing what code they need to update to avoid
getting called bozos). Sloppy thinking about which cases are supported
and unsupported leads to bugs, and some of those are likely to be
security bugs.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> OK ... but my point is that dump and restore does work. So whatever
> cases pg_get_expr() doesn't work must be cases that aren't needed for
> that to happen. Otherwise this problem would have been found long ago.
pg_get_expr doesn't (or didn't) depend on expression_tree_walker,
so there wasn't a problem there before. I am worried that there
might be other code paths, now or in future, that could try to apply
expression_tree_walker/mutator to relpartbound trees, which is
why I think it's a reasonable idea to teach them about such trees.
>> It does fall over if you try to apply it to stored rules:
>> regression=# select pg_get_expr(ev_action, 0) from pg_rewrite;
>> ERROR: unrecognized node type: 232
>> I'm not terribly excited about that, but maybe we should try to
>> improve it while we're here.
> In my view, the lack of excitement about sanity checks in functions
> that deal with node trees in the catalogs is the root of this problem.
It's only a problem if you hold the opinion that there should be
no user-reachable ERRCODE_INTERNAL_ERROR errors. Which is a fine
ideal, but I fear we're a pretty long way off from that.
> I realize that's a deep hole out of which we're unlikely to be able to
> climb in the short or even medium term, but we don't have to keep
> digging. We either make a rule that pg_get_expr() can apply to
> everything stored in the catalogs and produce sensible answers, which
> seems to be what you prefer, or we make it return nice errors for the
> cases that it can't handle nicely, or some combination of the two. And
> whatever we decide, we also document and enforce everywhere.
I think having pg_get_expr throw an error for a query, as opposed to an
expression, is fine. What I don't want to do is subdivide things a lot
more finely than that; thus lumping "relpartbound" into "expression"
seems like a reasonable thing to do. Especially since we already did it
six years ago.
In a quick check of catalogs with pg_node_tree columns, I find these
other columns that pg_get_expr can fail on (at least with the
examples available in the regression DB):
regression=# select count(pg_get_expr(prosqlbody,0)) from pg_proc;
ERROR: unrecognized node type: 232
regression=# select count(pg_get_expr(tgqual,tgrelid)) from pg_trigger ;
ERROR: bogus varno: 2
So that looks like the same cases we already knew about: input is
a querytree not an expression, or it contains Vars for more than
one relation.
regards, tom lane
Here's a less hasty version of the patch.
regards, tom lane
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index e276264882..5d4700430c 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2201,6 +2201,26 @@ expression_tree_walker(Node *node,
return true;
}
break;
+ case T_PartitionBoundSpec:
+ {
+ PartitionBoundSpec *pbs = (PartitionBoundSpec *) node;
+
+ if (walker(pbs->listdatums, context))
+ return true;
+ if (walker(pbs->lowerdatums, context))
+ return true;
+ if (walker(pbs->upperdatums, context))
+ return true;
+ }
+ break;
+ case T_PartitionRangeDatum:
+ {
+ PartitionRangeDatum *prd = (PartitionRangeDatum *) node;
+
+ if (walker(prd->value, context))
+ return true;
+ }
+ break;
case T_List:
foreach(temp, (List *) node)
{
@@ -3092,6 +3112,28 @@ expression_tree_mutator(Node *node,
return (Node *) newnode;
}
break;
+ case T_PartitionBoundSpec:
+ {
+ PartitionBoundSpec *pbs = (PartitionBoundSpec *) node;
+ PartitionBoundSpec *newnode;
+
+ FLATCOPY(newnode, pbs, PartitionBoundSpec);
+ MUTATE(newnode->listdatums, pbs->listdatums, List *);
+ MUTATE(newnode->lowerdatums, pbs->lowerdatums, List *);
+ MUTATE(newnode->upperdatums, pbs->upperdatums, List *);
+ return (Node *) newnode;
+ }
+ break;
+ case T_PartitionRangeDatum:
+ {
+ PartitionRangeDatum *prd = (PartitionRangeDatum *) node;
+ PartitionRangeDatum *newnode;
+
+ FLATCOPY(newnode, prd, PartitionRangeDatum);
+ MUTATE(newnode->value, prd->value, Node *);
+ return (Node *) newnode;
+ }
+ break;
case T_List:
{
/*
diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c
index e57c3aa124..9d5b8b9ab9 100644
--- a/src/backend/optimizer/util/var.c
+++ b/src/backend/optimizer/util/var.c
@@ -88,6 +88,9 @@ static Relids alias_relid_set(Query *query, Relids relids);
* Create a set of all the distinct varnos present in a parsetree.
* Only varnos that reference level-zero rtable entries are considered.
*
+ * "root" can be passed as NULL if it is not necessary to process
+ * PlaceHolderVars.
+ *
* NOTE: this is used on not-yet-planned expressions. It may therefore find
* bare SubLinks, and if so it needs to recurse into them to look for uplevel
* references to the desired rtable level! But when we find a completed
@@ -168,9 +171,13 @@ pull_varnos_walker(Node *node, pull_varnos_context *context)
/*
* If a PlaceHolderVar is not of the target query level, ignore it,
* instead recursing into its expression to see if it contains any
- * vars that are of the target level.
+ * vars that are of the target level. We'll also do that when the
+ * caller doesn't pass a "root" pointer. (We probably shouldn't see
+ * PlaceHolderVars at all in such cases, but if we do, this is a
+ * reasonable behavior.)
*/
- if (phv->phlevelsup == context->sublevels_up)
+ if (phv->phlevelsup == context->sublevels_up &&
+ context->root != NULL)
{
/*
* Ideally, the PHV's contribution to context->varnos is its
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 8da525c715..f73ce7ccb5 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2561,6 +2564,12 @@ decompile_column_index_array(Datum column_index_array, Oid relId,
* the one specified by the second parameter. This is sufficient for
* partial indexes, column default expressions, etc. We also support
* Var-free expressions, for which the OID can be InvalidOid.
+ *
+ * We expect this function to work, or throw a reasonably clean error,
+ * for any node tree that can appear in a catalog pg_node_tree column.
+ * Query trees, such as those appearing in pg_rewrite.ev_action, are
+ * excluded. So are expressions in more than one relation, which can
+ * appear in places like pg_rewrite.ev_qual.
* ----------
*/
Datum
@@ -2622,6 +2631,8 @@ static text *
pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
{
Node *node;
+ Node *tst;
+ Relids relids;
List *context;
char *exprstr;
char *str;
@@ -2634,6 +2645,40 @@ pg_get_expr_worker(text *expr, Oid relid, const char *relname, int prettyFlags)
pfree(exprstr);
+ /*
+ * Throw error if the input is a querytree rather than an expression tree.
+ * While we could support queries here, there seems no very good reason
+ * to. In most such catalog columns, we'll see a List of Query nodes, or
+ * even nested Lists, so drill down to a non-List node before checking.
+ */
+ tst = node;
+ while (tst && IsA(tst, List))
+ tst = linitial((List *) tst);
+ if (tst && IsA(tst, Query))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("input is a query, not an expression")));
+
+ /*
+ * Throw error if the expression contains Vars we won't be able to
+ * deparse.
+ */
+ relids = pull_varnos(NULL, node);
+ if (OidIsValid(relid))
+ {
+ if (!bms_is_subset(relids, bms_make_singleton(1)))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("expression contains variables of more than one relation")));
+ }
+ else
+ {
+ if (!bms_is_empty(relids))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("expression contains variables")));
+ }
+
/* Prepare deparse context if needed */
if (OidIsValid(relid))
context = deparse_context_for(relname, relid);
On Tue, Dec 21, 2021 at 6:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > OK ... but my point is that dump and restore does work. So whatever > > cases pg_get_expr() doesn't work must be cases that aren't needed for > > that to happen. Otherwise this problem would have been found long ago. > > pg_get_expr doesn't (or didn't) depend on expression_tree_walker, > so there wasn't a problem there before. I am worried that there > might be other code paths, now or in future, that could try to apply > expression_tree_walker/mutator to relpartbound trees, which is > why I think it's a reasonable idea to teach them about such trees. > > > I realize that's a deep hole out of which we're unlikely to be able to > > climb in the short or even medium term, but we don't have to keep > > digging. We either make a rule that pg_get_expr() can apply to > > everything stored in the catalogs and produce sensible answers, which > > seems to be what you prefer, or we make it return nice errors for the > > cases that it can't handle nicely, or some combination of the two. And > > whatever we decide, we also document and enforce everywhere. > > I think having pg_get_expr throw an error for a query, as opposed to an > expression, is fine. What I don't want to do is subdivide things a lot > more finely than that; thus lumping "relpartbound" into "expression" > seems like a reasonable thing to do. Especially since we already did it > six years ago. I admit that it was an oversight on my part that relpartbound trees are not recognized by nodeFuncs.c. :-( Thanks for addressing that in the patch you posted. I guess fixing only expression_tree_walker/mutator() suffices for now, but curious to know if it was intentional that you decided not to touch the following sites: exprCollation(): it would perhaps make sense to return the collation assigned to the 1st element of listdatums/lowerdatums/upperdatums, especially given that transformPartitionBoundValue() does assign a collation to the values in those lists based on the parent's partition key specification. exprType(): could be handled similarly queryjumble.c: JumbleExpr(): whose header comment says: * expression_tree_walker() does, and therefore it's coded to be as parallel * to that function as possible. * ... * Note: the reason we don't simply use expression_tree_walker() is that the * point of that function is to support tree walkers that don't care about * most tree node types, but here we care about all types. We should complain * about any unrecognized node type. or maybe not, because relpartbound contents ought never reach queryjumble.c? -- Amit Langote EDB: http://www.enterprisedb.com
On Thu, Jan 6, 2022 at 3:43 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Dec 21, 2021 at 6:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > > > OK ... but my point is that dump and restore does work. So whatever > > > cases pg_get_expr() doesn't work must be cases that aren't needed for > > > that to happen. Otherwise this problem would have been found long ago. > > > > pg_get_expr doesn't (or didn't) depend on expression_tree_walker, > > so there wasn't a problem there before. I am worried that there > > might be other code paths, now or in future, that could try to apply > > expression_tree_walker/mutator to relpartbound trees, which is > > why I think it's a reasonable idea to teach them about such trees. > > > > > I realize that's a deep hole out of which we're unlikely to be able to > > > climb in the short or even medium term, but we don't have to keep > > > digging. We either make a rule that pg_get_expr() can apply to > > > everything stored in the catalogs and produce sensible answers, which > > > seems to be what you prefer, or we make it return nice errors for the > > > cases that it can't handle nicely, or some combination of the two. And > > > whatever we decide, we also document and enforce everywhere. > > > > I think having pg_get_expr throw an error for a query, as opposed to an > > expression, is fine. What I don't want to do is subdivide things a lot > > more finely than that; thus lumping "relpartbound" into "expression" > > seems like a reasonable thing to do. Especially since we already did it > > six years ago. > > I admit that it was an oversight on my part that relpartbound trees > are not recognized by nodeFuncs.c. :-( > > Thanks for addressing that in the patch you posted. I guess fixing > only expression_tree_walker/mutator() suffices for now... Also, I wondered if it might be a good idea to expand the comment above NodeTag definition in nodes.h to tell someone adding new types to also look in nodeFuncs.c to check if any of the functions there need to be updated. -- Amit Langote EDB: http://www.enterprisedb.com
On Mon, Dec 20, 2021 at 4:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > pg_get_expr doesn't (or didn't) depend on expression_tree_walker, > so there wasn't a problem there before. I am worried that there > might be other code paths, now or in future, that could try to apply > expression_tree_walker/mutator to relpartbound trees, which is > why I think it's a reasonable idea to teach them about such trees. I agree that doing so is totally reasonable. I merely don't think that previous failure to do so makes anyone a "bozo". It was far from obvious that it was required. > It's only a problem if you hold the opinion that there should be > no user-reachable ERRCODE_INTERNAL_ERROR errors. Which is a fine > ideal, but I fear we're a pretty long way off from that. I do hold that opinion, and I think we ought to work in that direction even if we can't hope to get there quickly. -- Robert Haas EDB: http://www.enterprisedb.com
Amit Langote <amitlangote09@gmail.com> writes:
> Thanks for addressing that in the patch you posted. I guess fixing
> only expression_tree_walker/mutator() suffices for now, but curious to
> know if it was intentional that you decided not to touch the following
> sites:
> exprCollation(): it would perhaps make sense to return the collation
> assigned to the 1st element of listdatums/lowerdatums/upperdatums,
> especially given that transformPartitionBoundValue() does assign a
> collation to the values in those lists based on the parent's partition
> key specification.
But each column could have a different collation, no? I do not
think it's sensible to pick one of those at random and claim
that's the collation of the whole thing. So throwing an error
seems appropriate.
> exprType(): could be handled similarly
The same, in spades. Anybody who is asking for "the type"
of a relpartbound is misguided.
> queryjumble.c: JumbleExpr(): whose header comment says:
If somebody needs that, I wouldn't object to adding support there.
But right now it would just be dead code, so why bother?
regards, tom lane
On Fri, Jan 7, 2022 at 12:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Amit Langote <amitlangote09@gmail.com> writes: > > Thanks for addressing that in the patch you posted. I guess fixing > > only expression_tree_walker/mutator() suffices for now, but curious to > > know if it was intentional that you decided not to touch the following > > sites: > > > exprCollation(): it would perhaps make sense to return the collation > > assigned to the 1st element of listdatums/lowerdatums/upperdatums, > > especially given that transformPartitionBoundValue() does assign a > > collation to the values in those lists based on the parent's partition > > key specification. > > But each column could have a different collation, no? I do not > think it's sensible to pick one of those at random and claim > that's the collation of the whole thing. So throwing an error > seems appropriate. > > > exprType(): could be handled similarly > > The same, in spades. Anybody who is asking for "the type" > of a relpartbound is misguided. Okay, agree there's no need for handling bound nodes in these functions. Most sites that need to see the collation/type OID for bound datums work directly with the individual elements of those lists anyway. > > queryjumble.c: JumbleExpr(): whose header comment says: > > If somebody needs that, I wouldn't object to adding support there. > But right now it would just be dead code, so why bother? Sure, makes sense. -- Amit Langote EDB: http://www.enterprisedb.com