Horton

Thread

Commits

  1. Make pg_get_expr() more bulletproof.

  2. Implement table partitioning.

  1. sqlsmith: ERROR: XX000: bogus varno: 2

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-19T20:54:22Z

    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=<optimized out>, 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.

  2. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-19T21:17:16Z

    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

  3. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Robert Haas <robertmhaas@gmail.com> — 2021-12-20T14:49:09Z

    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

  4. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-20T16:25:55Z

    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

  5. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Robert Haas <robertmhaas@gmail.com> — 2021-12-20T18:00:12Z

    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

  6. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-20T18:13:51Z

    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

  7. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Robert Haas <robertmhaas@gmail.com> — 2021-12-20T18:50:37Z

    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

  8. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-20T19:36:35Z

    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

  9. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Robert Haas <robertmhaas@gmail.com> — 2021-12-20T20:17:01Z

    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

  10. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-20T21:20:11Z

    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

  11. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-20T23:17:49Z

    Here's a less hasty version of the patch.

			regards, tom lane

  12. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Amit Langote <amitlangote09@gmail.com> — 2022-01-06T06:43:59Z

    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

  13. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Amit Langote <amitlangote09@gmail.com> — 2022-01-06T12:44:25Z

    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

  14. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Robert Haas <robertmhaas@gmail.com> — 2022-01-06T14:37:54Z

    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

  15. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-06T15:24:09Z

    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

  16. Re: sqlsmith: ERROR: XX000: bogus varno: 2

    Amit Langote <amitlangote09@gmail.com> — 2022-01-07T13:10:23Z

    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