Re: [HACKERS] RangeTblEntry.joinaliasvars representation change
I wrote:
> After some reflection I think that the best fix is to redefine
> AcquireRewriteLocks' processing of dropped columns so that it puts an
> actual null pointer, not a consed-up Const, into the joinaliasvars list
> item.
Here's a complete patch along that line. Possibly worthy of note is
that I chose to keep expandRTE's API the same as before, ie, it returns
a NULL Const for a dropped column (when include_dropped is true). I had
intended to change it to return a null pointer to match the change in
the underlying data structure, but found that most of the callers
passing include_dropped = true need the Consts, because they're going to
construct a RowExpr that has to have null constants for the dropped
columns.
In HEAD, I'm a bit tempted to move strip_implicit_coercions into
nodes/nodeFuncs.c, so that we don't have the ugliness of the parser
calling an optimizer subroutine. But I propose applying this to back
branches as-is.
regards, tom lane
diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c
index 7eaf8d27bf08bf5dd1776d203876adb8396c73b3..5f736ad6c4060ff4d7dabb3844a04185e01fa3ef 100644
*** a/src/backend/optimizer/util/var.c
--- b/src/backend/optimizer/util/var.c
*** flatten_join_alias_vars_mutator(Node *no
*** 654,660
newvar = (Node *) lfirst(lv);
attnum++;
/* Ignore dropped columns */
! if (IsA(newvar, Const))
continue;
newvar = copyObject(newvar);
--- 654,660
newvar = (Node *) lfirst(lv);
attnum++;
/* Ignore dropped columns */
! if (newvar == NULL)
continue;
newvar = copyObject(newvar);
*** flatten_join_alias_vars_mutator(Node *no
*** 687,692
--- 687,693
/* Expand join alias reference */
Assert(var->varattno > 0);
newvar = (Node *) list_nth(rte->joinaliasvars, var->varattno - 1);
+ Assert(newvar != NULL);
newvar = copyObject(newvar);
/*
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index a9254c8c3a2e33b7c293ef51c53c78a797b1d4f1..42de89f510190877b1f6fa357efb08c81eb7acc9 100644
*** a/src/backend/parser/parse_relation.c
--- b/src/backend/parser/parse_relation.c
***
*** 24,29
--- 24,30
#include "funcapi.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
+ #include "optimizer/clauses.h"
#include "parser/parsetree.h"
#include "parser/parse_relation.h"
#include "parser/parse_type.h"
*** markRTEForSelectPriv(ParseState *pstate,
*** 749,762
* The aliasvar could be either a Var or a COALESCE expression,
* but in the latter case we should already have marked the two
* referent variables as being selected, due to their use in the
! * JOIN clause. So we need only be concerned with the simple Var
! * case.
*/
Var *aliasvar;
Assert(col > 0 && col <= list_length(rte->joinaliasvars));
aliasvar = (Var *) list_nth(rte->joinaliasvars, col - 1);
! if (IsA(aliasvar, Var))
markVarForSelectPriv(pstate, aliasvar, NULL);
}
}
--- 750,764
* The aliasvar could be either a Var or a COALESCE expression,
* but in the latter case we should already have marked the two
* referent variables as being selected, due to their use in the
! * JOIN clause. So we need only be concerned with the Var case.
! * But we do need to drill down through implicit coercions.
*/
Var *aliasvar;
Assert(col > 0 && col <= list_length(rte->joinaliasvars));
aliasvar = (Var *) list_nth(rte->joinaliasvars, col - 1);
! aliasvar = (Var *) strip_implicit_coercions((Node *) aliasvar);
! if (aliasvar && IsA(aliasvar, Var))
markVarForSelectPriv(pstate, aliasvar, NULL);
}
}
*** expandRTE(RangeTblEntry *rte, int rtinde
*** 1841,1850
* deleted columns in the join; but we have to check since
* this routine is also used by the rewriter, and joins
* found in stored rules might have join columns for
! * since-deleted columns. This will be signaled by a NULL
! * Const in the alias-vars list.
*/
! if (IsA(avar, Const))
{
if (include_dropped)
{
--- 1843,1852
* deleted columns in the join; but we have to check since
* this routine is also used by the rewriter, and joins
* found in stored rules might have join columns for
! * since-deleted columns. This will be signaled by a null
! * pointer in the alias-vars list.
*/
! if (avar == NULL)
{
if (include_dropped)
{
*** expandRTE(RangeTblEntry *rte, int rtinde
*** 1852,1859
*colnames = lappend(*colnames,
makeString(pstrdup("")));
if (colvars)
*colvars = lappend(*colvars,
! copyObject(avar));
}
continue;
}
--- 1854,1869 --
[HACKERS] RangeTblEntry.joinaliasvars representation change
In
http://www.postgresql.org/message-id/cak_s-g3-fwveer1c0idvtz0745-7ryifi8whbzcnmsn+hwc...@mail.gmail.com
it's pointed out that commit 2ffa740b was a few bricks shy of a load,
because it failed to cope with the possibility of a joinaliasvars item
containing an implicit coercion. That's not too hard to fix by adding a
strip_implicit_coercions() call, but while testing it I realized that
there's a second bug in the same place: the code also fails to cope with
a Const arising from a dropped input-relation column. (The comment
claiming that this can't happen is flat wrong, because we *have* passed
the view query through AcquireRewriteLocks().) I fixed that too, and
improved the comment in parsenodes.h that led me to overlook implicit
coercions in the first place, as per the attached WIP patch.
I then went looking for other places that might be assuming too much
about what is in joinaliasvars lists, and found several pre-existing
bugs :-(. The nastiest one is in flatten_join_alias_vars_mutator's code
to expand a whole-row reference, which supposes that if it finds a Const
item then that must represent a dropped column. That would be true in
the parser or rewriter, but at this point in planning we have already
done subquery pullup, which means we could find anything including a
Const there. The code would thus mistake a pulled-up constant subquery
output for a dropped column. An example in the regression database is
explain verbose select j from (int4_tbl join (select q1 as f1, q2 as z, 42 as c
from int8_tbl) ss using(f1)) j;
QUERY PLAN
---
Hash Join (cost=1.11..2.23 rows=5 width=16)
Output: ROW(int8_tbl.q1, int8_tbl.q2)
Hash Cond: (int8_tbl.q1 = int4_tbl.f1)
-> Seq Scan on public.int8_tbl (cost=0.00..1.05 rows=5 width=16)
Output: int8_tbl.q1, int8_tbl.q2
-> Hash (cost=1.05..1.05 rows=5 width=4)
Output: int4_tbl.f1
-> Seq Scan on public.int4_tbl (cost=0.00..1.05 rows=5 width=4)
Output: int4_tbl.f1
(9 rows)
The 42 has disappeared entirely from the ROW() construct :-(. This can
be reproduced in all active branches.
After some reflection I think that the best fix is to redefine
AcquireRewriteLocks' processing of dropped columns so that it puts an
actual null pointer, not a consed-up Const, into the joinaliasvars list
item. This is reliably distinguishable from anything we might pull up
from a subquery output, and it doesn't really lose information since the
Const was completely phony anyway. (I think the original design
envisioned having the Const carrying column datatype info, but we
abandoned that idea upon realizing that the dropped column might be of a
dropped type. The phony Const is currently always of type INT4.)
This definitional change will not affect any rules-on-disk since the
dropped-column substitution is only done on rule trees when they are
loaded back into memory.
A risk we'd be taking with this change is that any extension code that
looks at post-rewriter joinaliasvars lists might be confused. However,
I'm not sure why extension code would be looking at those. In any case,
I can't see a different back-patchable change that would reduce such a
risk; we have to change the representation *somehow* if we're going to
distinguish these cases correctly.
Thoughts?
regards, tom lane
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 40b565a11d6ddf40b3cd8e692e625efe1f6dc3a7..3292278774f15851f3ad7ae000139278c3f94fd8 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*** typedef struct
*** 235,240
--- 235,241
* child RTE's attno and rightattnos[i] is zero; and conversely for a
* column of the right child. But for merged columns produced by JOIN
* USING/NATURAL JOIN, both leftattnos[i] and rightattnos[i] are nonzero.
+ * Also, if the column has been dropped, both are zero.
*
* If it's a JOIN USING, usingNames holds the alias names selected for the
* merged columns (these might be different from the original USING list,
*** set_join_column_names(deparse_namespace
*** 3053,3058
--- 3054,3066
char *colname = colinfo->colnames[i];
char *real_colname;
+ /* Ignore dropped column (only possible for non-merged column) */
+ if (colinfo->leftattnos[i] == 0 && colinfo->rightattnos[i] == 0)
+ {
+ Assert(colname == NULL);
+ continue;
+ }
+
/* Get the child column name */
if (colinfo->leftattnos[i] > 0)
real_colname = leftcolinfo->colnames[colinfo->leftattnos[i] - 1];
*** set_join_column_names(deparse_namespace
*** 3061,3075
else
{
/* We're joining system columns --- use eref name */
! real_colname = (char *) list_nth(rte->eref->colnames, i);
! }
!
! /* Ign
