Thread: row literal problem

I'm chasing up an issue from a client who has this problem (in 9.1):

with q as
(    some query here
)
select q.* from q

yields:
                   job_scope                   | checked_col
-----------------------------------------------+------------------------------------------ Co Revenues: Co Revenues
$100to $999 Million | <input panel=data 
 
type=checkbox checked> Metropolitan Area: Austin-Round Rock          | <input panel=data 
type=checkbox checked>

which is as expected.

However,

with q as
(    same query here
)
select q from q

yields:

q
----------------------------------------------------------------------------------------------- ("Co Revenues: Co
Revenues$100 to $999 Million","<input panel=data 
 
type=checkbox checked>",) ("Metropolitan Area: Austin-Round Rock","<input panel=data 
type=checkbox checked>",)


Note the trailing comma inside the (), which certainly looks bogus to 
me. If I replace "select q" with "select row(q.*)" it disappears.

It doesn't happen in all cases, and I'm trying to work out a minimal 
reproducible example. But it sure is puzzling.

cheers

andrew

On Wed, Jul 18, 2012 at 1:58 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> I'm chasing up an issue from a client who has this problem (in 9.1):
>
> with q as
> (
>     some query here
> )
> select q.* from q
>
> yields:
>
>                    job_scope                   | checked_col
> -----------------------------------------------+------------------------------------------
>  Co Revenues: Co Revenues $100 to $999 Million | <input panel=data
> type=checkbox checked>
>  Metropolitan Area: Austin-Round Rock          | <input panel=data
> type=checkbox checked>
>
> which is as expected.
>
> However,
>
> with q as
> (
>     same query here
> )
> select q from q
>
> yields:
>
> q
> -----------------------------------------------------------------------------------------------
>  ("Co Revenues: Co Revenues $100 to $999 Million","<input panel=data
> type=checkbox checked>",)
>  ("Metropolitan Area: Austin-Round Rock","<input panel=data type=checkbox
> checked>",)
>
>
> Note the trailing comma inside the (), which certainly looks bogus to me. If
> I replace "select q" with "select row(q.*)" it disappears.
>
> It doesn't happen in all cases, and I'm trying to work out a minimal
> reproducible example. But it sure is puzzling.

there are no null fields, right? if the last field is sometimes null
you'd see that (you probably ruled that out though).  when you say
'sometimes', do you mean for some rows and not others? or for some
queries?

merlin

On 07/18/2012 03:18 PM, Merlin Moncure wrote:
> On Wed, Jul 18, 2012 at 1:58 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> I'm chasing up an issue from a client who has this problem (in 9.1):
>>
>> with q as
>> (
>>      some query here
>> )
>> select q.* from q
>>
>> yields:
>>
>>                     job_scope                   | checked_col
>> -----------------------------------------------+------------------------------------------
>>   Co Revenues: Co Revenues $100 to $999 Million | <input panel=data
>> type=checkbox checked>
>>   Metropolitan Area: Austin-Round Rock          | <input panel=data
>> type=checkbox checked>
>>
>> which is as expected.
>>
>> However,
>>
>> with q as
>> (
>>      same query here
>> )
>> select q from q
>>
>> yields:
>>
>> q
>> -----------------------------------------------------------------------------------------------
>>   ("Co Revenues: Co Revenues $100 to $999 Million","<input panel=data
>> type=checkbox checked>",)
>>   ("Metropolitan Area: Austin-Round Rock","<input panel=data type=checkbox
>> checked>",)
>>
>>
>> Note the trailing comma inside the (), which certainly looks bogus to me. If
>> I replace "select q" with "select row(q.*)" it disappears.
>>
>> It doesn't happen in all cases, and I'm trying to work out a minimal
>> reproducible example. But it sure is puzzling.
> there are no null fields, right? if the last field is sometimes null
> you'd see that (you probably ruled that out though).  when you say
> 'sometimes', do you mean for some rows and not others? or for some
> queries?
>



No, the inner query has two fields.

It happens for all rows, but not for all two-field-resulting queries as 
q. I'm trying to find a simple case rather than the rather complex query 
my customer is using.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 07/18/2012 03:18 PM, Merlin Moncure wrote:
>> there are no null fields, right? if the last field is sometimes null
>> you'd see that (you probably ruled that out though).  when you say
>> 'sometimes', do you mean for some rows and not others? or for some
>> queries?

> No, the inner query has two fields.

> It happens for all rows, but not for all two-field-resulting queries as 
> q. I'm trying to find a simple case rather than the rather complex query 
> my customer is using.

I'm wondering about a rowtype with a third, dropped column.
        regards, tom lane

On 07/18/2012 03:30 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 07/18/2012 03:18 PM, Merlin Moncure wrote:
>>> there are no null fields, right? if the last field is sometimes null
>>> you'd see that (you probably ruled that out though).  when you say
>>> 'sometimes', do you mean for some rows and not others? or for some
>>> queries?
>> No, the inner query has two fields.
>> It happens for all rows, but not for all two-field-resulting queries as
>> q. I'm trying to find a simple case rather than the rather complex query
>> my customer is using.
> I'm wondering about a rowtype with a third, dropped column.


As usual Tom has hit the nail on the head. Here's a simple test case 
that demonstrates the problem. I could probably have cut it down more 
but I was following the structure of the original somewhat:
   # with q as   (   select max(nspname) as nspname, sum(allind.count) as indices       from (select indrelid, count(*)
           from pg_index     group by indrelid) allind         left outer join pg_class on pg_class.oid =
allind.indrelid    left outer join pg_namespace on pg_class.relnamespace =   pg_namespace.oid       group by
pg_namespace.oid  )   select q from q;
 
             q   --------------------     (pg_catalog,91,11)     (pg_toast,18,99)   (2 rows)


cheers

andrew



>
>             regards, tom lane
>

On Wed, Jul 18, 2012 at 3:42 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> On 07/18/2012 03:30 PM, Tom Lane wrote:
>>
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>>
>>> On 07/18/2012 03:18 PM, Merlin Moncure wrote:
>>>>
>>>> there are no null fields, right? if the last field is sometimes null
>>>> you'd see that (you probably ruled that out though).  when you say
>>>> 'sometimes', do you mean for some rows and not others? or for some
>>> queries?
>>>
>>> No, the inner query has two fields.
>>> It happens for all rows, but not for all two-field-resulting queries as
>>> q. I'm trying to find a simple case rather than the rather complex query
>>> my customer is using.
>>
>> I'm wondering about a rowtype with a third, dropped column.
>
>
>
> As usual Tom has hit the nail on the head. Here's a simple test case that
> demonstrates the problem. I could probably have cut it down more but I was
> following the structure of the original somewhat:
>
>    # with q as
>    (
>    select max(nspname) as nspname, sum(allind.count) as indices
>        from (select indrelid, count(*)
>              from pg_index
>      group by indrelid) allind
>          left outer join pg_class on pg_class.oid = allind.indrelid
>      left outer join pg_namespace on pg_class.relnamespace =
>    pg_namespace.oid
>        group by pg_namespace.oid
>    )
>    select q from q;
>
>              q
>    --------------------
>      (pg_catalog,91,11)
>      (pg_toast,18,99)
>    (2 rows)

hm, it's the 'group by' -- for example if you add group by
pg_namespace.oid, group by pg_namespace.oid || 'abc', you can invent
columns that come back into the rowtype.

merlin

On Wed, Jul 18, 2012 at 3:56 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> hm, it's the 'group by' -- for example if you add group by
> pg_namespace.oid, group by pg_namespace.oid || 'abc', you can invent
> columns that come back into the rowtype.

here's a cut down example:
with q as (select max(v) from (select 1 as v) q group by v) select q from q;

merlin

Merlin Moncure <mmoncure@gmail.com> writes:
> hm, it's the 'group by' -- for example if you add group by
> pg_namespace.oid, group by pg_namespace.oid || 'abc', you can invent
> columns that come back into the rowtype.

Yeah, the whole-row variable is evidently picking up "resjunk" columns
from the inner query.  Haven't looked to see exactly where.
        regards, tom lane

Merlin Moncure <mmoncure@gmail.com> writes:
> here's a cut down example:
> with q as (select max(v) from (select 1 as v) q group by v) select q from q;

I traced through this example, and find that it's another issue in an
area we've hacked at before.  ExecEvalVar, when it finds that it's
dealing with a whole-row Var of type RECORD, just assumes that the tuple
descriptor of the source TupleTableSlot is the correct descriptor for
the whole-row result.  Now, in the case where the whole-row Var has a
named composite type, we have found that we have to go to quite some
lengths to deal with possible discrepancies in the desired and actual
rowtypes; in particular notice this comment:
            * ... Also, we have to allow the case that the slot has            * more columns than the Var's type,
becausewe might be looking            * at the output of a subplan that includes resjunk columns. (XXX            * it
wouldbe nice to verify that the extra columns are all            * marked resjunk, but we haven't got access to the
subplan           * targetlist here...)
 

I think the way to solve this is to do whatever it takes to get access
to the subplan targetlist.  We could then do something a bit cleaner
than what the named-rowtype code is currently doing: if there are
resjunk columns in the subplan targetlist, use the tlist to create a
JunkFilter, and then pass the tuples through that.  After that we can
insist that the tuples don't have any extra columns.

Getting hold of that tlist is going to be a bit messy, though.  I think
what we can do is create a special ExprState variant for whole-row Vars
(which we'd need anyway to hold the JunkFilter), and have ExecInitExpr
store the "parent" PlanState pointer into it.  Then at the first call
of ExecEvalVar, dig down to the appropriate tlist depending on what
type of PlanState we find ourselves running in.  This shouldn't be
too painful because a whole-row Var can only appear in a simple scan
node, not an upper-level plan node, so there are not as many cases
to deal with as you might think.
        regards, tom lane

I wrote:
> I think the way to solve this is to do whatever it takes to get access
> to the subplan targetlist.  We could then do something a bit cleaner
> than what the named-rowtype code is currently doing: if there are
> resjunk columns in the subplan targetlist, use the tlist to create a
> JunkFilter, and then pass the tuples through that.  After that we can
> insist that the tuples don't have any extra columns.

Here's a draft patch for that.  It wasn't quite as ugly as I feared.
A lot of the apparent bulk of the patch is because I chose to split
ExecEvalVar into separate functions for the scalar and whole-row
cases, which seemed appropriate because they now get different
ExprState node types.

            regards, tom lane

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 0ea21ca5f91a704ccde2c765bd92d0c5af7ead8b..ae7987ee97482e1224912f3e96d5a90eaa1f10f7 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
***************
*** 20,26 ****
   *        ExecProject        - form a new tuple by projecting the given tuple
   *
   *     NOTES
!  *        The more heavily used ExecEvalExpr routines, such as ExecEvalVar(),
   *        are hotspots. Making these faster will speed up the entire system.
   *
   *        ExecProject() is used to make tuple projections.  Rather then
--- 20,26 ----
   *        ExecProject        - form a new tuple by projecting the given tuple
   *
   *     NOTES
!  *        The more heavily used ExecEvalExpr routines, such as ExecEvalScalarVar,
   *        are hotspots. Making these faster will speed up the entire system.
   *
   *        ExecProject() is used to make tuple projections.  Rather then
*************** static Datum ExecEvalAggref(AggrefExprSt
*** 68,80 ****
  static Datum ExecEvalWindowFunc(WindowFuncExprState *wfunc,
                     ExprContext *econtext,
                     bool *isNull, ExprDoneCond *isDone);
- static Datum ExecEvalVar(ExprState *exprstate, ExprContext *econtext,
-             bool *isNull, ExprDoneCond *isDone);
  static Datum ExecEvalScalarVar(ExprState *exprstate, ExprContext *econtext,
                    bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowVar(ExprState *exprstate, ExprContext *econtext,
                      bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowSlow(ExprState *exprstate, ExprContext *econtext,
                       bool *isNull, ExprDoneCond *isDone);
  static Datum ExecEvalConst(ExprState *exprstate, ExprContext *econtext,
                bool *isNull, ExprDoneCond *isDone);
--- 68,85 ----
  static Datum ExecEvalWindowFunc(WindowFuncExprState *wfunc,
                     ExprContext *econtext,
                     bool *isNull, ExprDoneCond *isDone);
  static Datum ExecEvalScalarVar(ExprState *exprstate, ExprContext *econtext,
                    bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalScalarVar2(ExprState *exprstate, ExprContext *econtext,
!                    bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowVar(WholeRowVarExprState *wrvstate,
!                     ExprContext *econtext,
                      bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowFast(WholeRowVarExprState *wrvstate,
!                      ExprContext *econtext,
!                      bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowSlow(WholeRowVarExprState *wrvstate,
!                      ExprContext *econtext,
                       bool *isNull, ExprDoneCond *isDone);
  static Datum ExecEvalConst(ExprState *exprstate, ExprContext *econtext,
                bool *isNull, ExprDoneCond *isDone);
*************** ExecEvalWindowFunc(WindowFuncExprState *
*** 553,572 ****
  }

  /* ----------------------------------------------------------------
!  *        ExecEvalVar
   *
!  *        Returns a Datum whose value is the value of a range
!  *        variable with respect to given expression context.
   *
!  * Note: ExecEvalVar is executed only the first time through in a given plan;
!  * it changes the ExprState's function pointer to pass control directly to
!  * ExecEvalScalarVar, ExecEvalWholeRowVar, or ExecEvalWholeRowSlow after
!  * making one-time checks.
   * ----------------------------------------------------------------
   */
  static Datum
! ExecEvalVar(ExprState *exprstate, ExprContext *econtext,
!             bool *isNull, ExprDoneCond *isDone)
  {
      Var           *variable = (Var *) exprstate->expr;
      TupleTableSlot *slot;
--- 558,576 ----
  }

  /* ----------------------------------------------------------------
!  *        ExecEvalScalarVar
   *
!  *        Returns a Datum whose value is the value of a scalar (not whole-row)
!  *        range variable with respect to given expression context.
   *
!  * Note: ExecEvalScalarVar is executed only the first time through in a given
!  * plan; it changes the ExprState's function pointer to pass control directly
!  * to ExecEvalScalarVar2 after making one-time checks.
   * ----------------------------------------------------------------
   */
  static Datum
! ExecEvalScalarVar(ExprState *exprstate, ExprContext *econtext,
!                   bool *isNull, ExprDoneCond *isDone)
  {
      Var           *variable = (Var *) exprstate->expr;
      TupleTableSlot *slot;
*************** ExecEvalVar(ExprState *exprstate, ExprCo
*** 596,757 ****

      attnum = variable->varattno;

!     if (attnum != InvalidAttrNumber)
!     {
!         /*
!          * Scalar variable case.
!          *
!          * If it's a user attribute, check validity (bogus system attnums will
!          * be caught inside slot_getattr).    What we have to check for here is
!          * the possibility of an attribute having been changed in type since
!          * the plan tree was created.  Ideally the plan would get invalidated
!          * and not re-used, but until that day arrives, we need defenses.
!          * Fortunately it's sufficient to check once on the first time
!          * through.
!          *
!          * Note: we allow a reference to a dropped attribute.  slot_getattr
!          * will force a NULL result in such cases.
!          *
!          * Note: ideally we'd check typmod as well as typid, but that seems
!          * impractical at the moment: in many cases the tupdesc will have been
!          * generated by ExecTypeFromTL(), and that can't guarantee to generate
!          * an accurate typmod in all cases, because some expression node types
!          * don't carry typmod.
!          */
!         if (attnum > 0)
!         {
!             TupleDesc    slot_tupdesc = slot->tts_tupleDescriptor;
!             Form_pg_attribute attr;
!
!             if (attnum > slot_tupdesc->natts)    /* should never happen */
!                 elog(ERROR, "attribute number %d exceeds number of columns %d",
!                      attnum, slot_tupdesc->natts);
!
!             attr = slot_tupdesc->attrs[attnum - 1];
!
!             /* can't check type if dropped, since atttypid is probably 0 */
!             if (!attr->attisdropped)
!             {
!                 if (variable->vartype != attr->atttypid)
!                     ereport(ERROR,
!                             (errmsg("attribute %d has wrong type", attnum),
!                         errdetail("Table has type %s, but query expects %s.",
!                                   format_type_be(attr->atttypid),
!                                   format_type_be(variable->vartype))));
!             }
!         }
!
!         /* Skip the checking on future executions of node */
!         exprstate->evalfunc = ExecEvalScalarVar;

!         /* Fetch the value from the slot */
!         return slot_getattr(slot, attnum, isNull);
!     }
!     else
      {
-         /*
-          * Whole-row variable.
-          *
-          * If it's a RECORD Var, we'll use the slot's type ID info.  It's
-          * likely that the slot's type is also RECORD; if so, make sure it's
-          * been "blessed", so that the Datum can be interpreted later.
-          *
-          * If the Var identifies a named composite type, we must check that
-          * the actual tuple type is compatible with it.
-          */
          TupleDesc    slot_tupdesc = slot->tts_tupleDescriptor;
!         bool        needslow = false;

!         if (variable->vartype == RECORDOID)
!         {
!             if (slot_tupdesc->tdtypeid == RECORDOID &&
!                 slot_tupdesc->tdtypmod < 0)
!                 assign_record_type_typmod(slot_tupdesc);
!         }
!         else
!         {
!             TupleDesc    var_tupdesc;
!             int            i;

!             /*
!              * We really only care about number of attributes and data type.
!              * Also, we can ignore type mismatch on columns that are dropped
!              * in the destination type, so long as (1) the physical storage
!              * matches or (2) the actual column value is NULL.    Case (1) is
!              * helpful in some cases involving out-of-date cached plans, while
!              * case (2) is expected behavior in situations such as an INSERT
!              * into a table with dropped columns (the planner typically
!              * generates an INT4 NULL regardless of the dropped column type).
!              * If we find a dropped column and cannot verify that case (1)
!              * holds, we have to use ExecEvalWholeRowSlow to check (2) for
!              * each row.  Also, we have to allow the case that the slot has
!              * more columns than the Var's type, because we might be looking
!              * at the output of a subplan that includes resjunk columns. (XXX
!              * it would be nice to verify that the extra columns are all
!              * marked resjunk, but we haven't got access to the subplan
!              * targetlist here...) Resjunk columns should always be at the end
!              * of a targetlist, so it's sufficient to ignore them here; but we
!              * need to use ExecEvalWholeRowSlow to get rid of them in the
!              * eventual output tuples.
!              */
!             var_tupdesc = lookup_rowtype_tupdesc(variable->vartype, -1);

!             if (var_tupdesc->natts > slot_tupdesc->natts)
                  ereport(ERROR,
!                         (errcode(ERRCODE_DATATYPE_MISMATCH),
!                          errmsg("table row type and query-specified row type do not match"),
!                          errdetail_plural("Table row contains %d attribute, but query expects %d.",
!                    "Table row contains %d attributes, but query expects %d.",
!                                           slot_tupdesc->natts,
!                                           slot_tupdesc->natts,
!                                           var_tupdesc->natts)));
!             else if (var_tupdesc->natts < slot_tupdesc->natts)
!                 needslow = true;    /* need to trim trailing atts */
!
!             for (i = 0; i < var_tupdesc->natts; i++)
!             {
!                 Form_pg_attribute vattr = var_tupdesc->attrs[i];
!                 Form_pg_attribute sattr = slot_tupdesc->attrs[i];
!
!                 if (vattr->atttypid == sattr->atttypid)
!                     continue;    /* no worries */
!                 if (!vattr->attisdropped)
!                     ereport(ERROR,
!                             (errcode(ERRCODE_DATATYPE_MISMATCH),
!                              errmsg("table row type and query-specified row type do not match"),
!                              errdetail("Table has type %s at ordinal position %d, but query expects %s.",
!                                        format_type_be(sattr->atttypid),
!                                        i + 1,
!                                        format_type_be(vattr->atttypid))));
!
!                 if (vattr->attlen != sattr->attlen ||
!                     vattr->attalign != sattr->attalign)
!                     needslow = true;    /* need runtime check for null */
!             }
!
!             ReleaseTupleDesc(var_tupdesc);
          }

!         /* Skip the checking on future executions of node */
!         if (needslow)
!             exprstate->evalfunc = ExecEvalWholeRowSlow;
!         else
!             exprstate->evalfunc = ExecEvalWholeRowVar;

!         /* Fetch the value */
!         return (*exprstate->evalfunc) (exprstate, econtext, isNull, isDone);
!     }
  }

  /* ----------------------------------------------------------------
!  *        ExecEvalScalarVar
   *
   *        Returns a Datum for a scalar variable.
   * ----------------------------------------------------------------
   */
  static Datum
! ExecEvalScalarVar(ExprState *exprstate, ExprContext *econtext,
!                   bool *isNull, ExprDoneCond *isDone)
  {
      Var           *variable = (Var *) exprstate->expr;
      TupleTableSlot *slot;
--- 600,664 ----

      attnum = variable->varattno;

!     /* This was checked by ExecInitExpr */
!     Assert(attnum != InvalidAttrNumber);

!     /*
!      * If it's a user attribute, check validity (bogus system attnums will be
!      * caught inside slot_getattr).  What we have to check for here is the
!      * possibility of an attribute having been changed in type since the plan
!      * tree was created.  Ideally the plan will get invalidated and not
!      * re-used, but just in case, we keep these defenses.  Fortunately it's
!      * sufficient to check once on the first time through.
!      *
!      * Note: we allow a reference to a dropped attribute.  slot_getattr will
!      * force a NULL result in such cases.
!      *
!      * Note: ideally we'd check typmod as well as typid, but that seems
!      * impractical at the moment: in many cases the tupdesc will have been
!      * generated by ExecTypeFromTL(), and that can't guarantee to generate an
!      * accurate typmod in all cases, because some expression node types don't
!      * carry typmod.
!      */
!     if (attnum > 0)
      {
          TupleDesc    slot_tupdesc = slot->tts_tupleDescriptor;
!         Form_pg_attribute attr;

!         if (attnum > slot_tupdesc->natts)    /* should never happen */
!             elog(ERROR, "attribute number %d exceeds number of columns %d",
!                  attnum, slot_tupdesc->natts);

!         attr = slot_tupdesc->attrs[attnum - 1];

!         /* can't check type if dropped, since atttypid is probably 0 */
!         if (!attr->attisdropped)
!         {
!             if (variable->vartype != attr->atttypid)
                  ereport(ERROR,
!                         (errmsg("attribute %d has wrong type", attnum),
!                          errdetail("Table has type %s, but query expects %s.",
!                                    format_type_be(attr->atttypid),
!                                    format_type_be(variable->vartype))));
          }
+     }

!     /* Skip the checking on future executions of node */
!     exprstate->evalfunc = ExecEvalScalarVar2;

!     /* Fetch the value from the slot */
!     return slot_getattr(slot, attnum, isNull);
  }

  /* ----------------------------------------------------------------
!  *        ExecEvalScalarVar2
   *
   *        Returns a Datum for a scalar variable.
   * ----------------------------------------------------------------
   */
  static Datum
! ExecEvalScalarVar2(ExprState *exprstate, ExprContext *econtext,
!                    bool *isNull, ExprDoneCond *isDone)
  {
      Var           *variable = (Var *) exprstate->expr;
      TupleTableSlot *slot;
*************** ExecEvalScalarVar(ExprState *exprstate,
*** 788,801 ****
  /* ----------------------------------------------------------------
   *        ExecEvalWholeRowVar
   *
!  *        Returns a Datum for a whole-row variable.
   * ----------------------------------------------------------------
   */
  static Datum
! ExecEvalWholeRowVar(ExprState *exprstate, ExprContext *econtext,
                      bool *isNull, ExprDoneCond *isDone)
  {
!     Var           *variable = (Var *) exprstate->expr;
      TupleTableSlot *slot;
      HeapTuple    tuple;
      TupleDesc    tupleDesc;
--- 695,895 ----
  /* ----------------------------------------------------------------
   *        ExecEvalWholeRowVar
   *
!  *        Returns a Datum whose value is the value of a whole-row range
!  *        variable with respect to given expression context.
!  *
!  * Note: ExecEvalWholeRowVar is executed only the first time through in a
!  * given plan; it changes the ExprState's function pointer to pass control
!  * directly to ExecEvalWholeRowFast or ExecEvalWholeRowSlow after making
!  * one-time checks.
   * ----------------------------------------------------------------
   */
  static Datum
! ExecEvalWholeRowVar(WholeRowVarExprState *wrvstate, ExprContext *econtext,
                      bool *isNull, ExprDoneCond *isDone)
  {
!     Var           *variable = (Var *) wrvstate->xprstate.expr;
!     TupleTableSlot *slot;
!     TupleDesc    slot_tupdesc;
!     bool        needslow = false;
!
!     if (isDone)
!         *isDone = ExprSingleResult;
!
!     /* This was checked by ExecInitExpr */
!     Assert(variable->varattno == InvalidAttrNumber);
!
!     /* Get the input slot we want */
!     switch (variable->varno)
!     {
!         case INNER_VAR: /* get the tuple from the inner node */
!             slot = econtext->ecxt_innertuple;
!             break;
!
!         case OUTER_VAR: /* get the tuple from the outer node */
!             slot = econtext->ecxt_outertuple;
!             break;
!
!             /* INDEX_VAR is handled by default case */
!
!         default:                /* get the tuple from the relation being
!                                  * scanned */
!             slot = econtext->ecxt_scantuple;
!             break;
!     }
!
!     /*
!      * If the input tuple came from a subquery, it might contain "resjunk"
!      * columns (such as GROUP BY or ORDER BY columns), which we don't want
!      * to keep in the whole-row result.  We can get rid of such columns by
!      * passing the tuple through a JunkFilter --- but to make one, we have
!      * to lay our hands on the subquery's targetlist.  Fortunately, there
!      * are not very many cases where this can happen, and we can identify
!      * all of them by examining our parent PlanState.
!      */
!     if (wrvstate->parent)
!     {
!         PlanState  *subplan = NULL;
!
!         switch (nodeTag(wrvstate->parent))
!         {
!             case T_SubqueryScanState:
!                 subplan = ((SubqueryScanState *) wrvstate->parent)->subplan;
!                 break;
!             case T_CteScanState:
!                 subplan = ((CteScanState *) wrvstate->parent)->cteplanstate;
!                 break;
!             default:
!                 break;
!         }
!
!         if (subplan)
!         {
!             bool        junk_filter_needed = false;
!             ListCell   *tlist;
!
!             /* Detect whether subplan tlist actually has any junk columns */
!             foreach(tlist, subplan->plan->targetlist)
!             {
!                 TargetEntry *tle = (TargetEntry *) lfirst(tlist);
!
!                 if (tle->resjunk)
!                 {
!                     junk_filter_needed = true;
!                     break;
!                 }
!             }
!
!             /* If so, build the junkfilter in the query memory context */
!             if (junk_filter_needed)
!             {
!                 MemoryContext oldcontext;
!
!                 oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
!                 wrvstate->wrv_junkFilter =
!                     ExecInitJunkFilter(subplan->plan->targetlist,
!                                        ExecGetResultType(subplan)->tdhasoid,
!                                        ExecInitExtraTupleSlot(wrvstate->parent->state));
!                 MemoryContextSwitchTo(oldcontext);
!             }
!         }
!     }
!
!     /* Apply the junkfilter if any */
!     if (wrvstate->wrv_junkFilter != NULL)
!         slot = ExecFilterJunk(wrvstate->wrv_junkFilter, slot);
!
!     slot_tupdesc = slot->tts_tupleDescriptor;
!
!     /*
!      * If it's a RECORD Var, we'll use the slot's type ID info.  It's likely
!      * that the slot's type is also RECORD; if so, make sure it's been
!      * "blessed", so that the Datum can be interpreted later.
!      *
!      * If the Var identifies a named composite type, we must check that the
!      * actual tuple type is compatible with it.
!      */
!     if (variable->vartype == RECORDOID)
!     {
!         if (slot_tupdesc->tdtypeid == RECORDOID &&
!             slot_tupdesc->tdtypmod < 0)
!             assign_record_type_typmod(slot_tupdesc);
!     }
!     else
!     {
!         TupleDesc    var_tupdesc;
!         int            i;
!
!         /*
!          * We really only care about number of attributes and data type.
!          * Also, we can ignore type mismatch on columns that are dropped in
!          * the destination type, so long as (1) the physical storage matches
!          * or (2) the actual column value is NULL.  Case (1) is helpful in
!          * some cases involving out-of-date cached plans, while case (2) is
!          * expected behavior in situations such as an INSERT into a table with
!          * dropped columns (the planner typically generates an INT4 NULL
!          * regardless of the dropped column type).  If we find a dropped
!          * column and cannot verify that case (1) holds, we have to use
!          * ExecEvalWholeRowSlow to check (2) for each row.
!          */
!         var_tupdesc = lookup_rowtype_tupdesc(variable->vartype, -1);
!
!         if (var_tupdesc->natts != slot_tupdesc->natts)
!             ereport(ERROR,
!                     (errcode(ERRCODE_DATATYPE_MISMATCH),
!                      errmsg("table row type and query-specified row type do not match"),
!                      errdetail_plural("Table row contains %d attribute, but query expects %d.",
!                                       "Table row contains %d attributes, but query expects %d.",
!                                       slot_tupdesc->natts,
!                                       slot_tupdesc->natts,
!                                       var_tupdesc->natts)));
!
!         for (i = 0; i < var_tupdesc->natts; i++)
!         {
!             Form_pg_attribute vattr = var_tupdesc->attrs[i];
!             Form_pg_attribute sattr = slot_tupdesc->attrs[i];
!
!             if (vattr->atttypid == sattr->atttypid)
!                 continue;    /* no worries */
!             if (!vattr->attisdropped)
!                 ereport(ERROR,
!                         (errcode(ERRCODE_DATATYPE_MISMATCH),
!                          errmsg("table row type and query-specified row type do not match"),
!                          errdetail("Table has type %s at ordinal position %d, but query expects %s.",
!                                    format_type_be(sattr->atttypid),
!                                    i + 1,
!                                    format_type_be(vattr->atttypid))));
!
!             if (vattr->attlen != sattr->attlen ||
!                 vattr->attalign != sattr->attalign)
!                 needslow = true;    /* need runtime check for null */
!         }
!
!         ReleaseTupleDesc(var_tupdesc);
!     }
!
!     /* Skip the checking on future executions of node */
!     if (needslow)
!         wrvstate->xprstate.evalfunc = (ExprStateEvalFunc) ExecEvalWholeRowSlow;
!     else
!         wrvstate->xprstate.evalfunc = (ExprStateEvalFunc) ExecEvalWholeRowFast;
!
!     /* Fetch the value */
!     return (*wrvstate->xprstate.evalfunc) ((ExprState *) wrvstate, econtext,
!                                            isNull, isDone);
! }
!
! /* ----------------------------------------------------------------
!  *        ExecEvalWholeRowFast
!  *
!  *        Returns a Datum for a whole-row variable.
!  * ----------------------------------------------------------------
!  */
! static Datum
! ExecEvalWholeRowFast(WholeRowVarExprState *wrvstate, ExprContext *econtext,
!                      bool *isNull, ExprDoneCond *isDone)
! {
!     Var           *variable = (Var *) wrvstate->xprstate.expr;
      TupleTableSlot *slot;
      HeapTuple    tuple;
      TupleDesc    tupleDesc;
*************** ExecEvalWholeRowVar(ExprState *exprstate
*** 824,829 ****
--- 918,927 ----
              break;
      }

+     /* Apply the junkfilter if any */
+     if (wrvstate->wrv_junkFilter != NULL)
+         slot = ExecFilterJunk(wrvstate->wrv_junkFilter, slot);
+
      tuple = ExecFetchSlotTuple(slot);
      tupleDesc = slot->tts_tupleDescriptor;

*************** ExecEvalWholeRowVar(ExprState *exprstate
*** 857,871 ****
  /* ----------------------------------------------------------------
   *        ExecEvalWholeRowSlow
   *
!  *        Returns a Datum for a whole-row variable, in the "slow" cases where
   *        we can't just copy the subplan's output.
   * ----------------------------------------------------------------
   */
  static Datum
! ExecEvalWholeRowSlow(ExprState *exprstate, ExprContext *econtext,
                       bool *isNull, ExprDoneCond *isDone)
  {
!     Var           *variable = (Var *) exprstate->expr;
      TupleTableSlot *slot;
      HeapTuple    tuple;
      TupleDesc    var_tupdesc;
--- 955,969 ----
  /* ----------------------------------------------------------------
   *        ExecEvalWholeRowSlow
   *
!  *        Returns a Datum for a whole-row variable, in the "slow" case where
   *        we can't just copy the subplan's output.
   * ----------------------------------------------------------------
   */
  static Datum
! ExecEvalWholeRowSlow(WholeRowVarExprState *wrvstate, ExprContext *econtext,
                       bool *isNull, ExprDoneCond *isDone)
  {
!     Var           *variable = (Var *) wrvstate->xprstate.expr;
      TupleTableSlot *slot;
      HeapTuple    tuple;
      TupleDesc    var_tupdesc;
*************** ExecEvalWholeRowSlow(ExprState *exprstat
*** 895,913 ****
              break;
      }

!     /*
!      * Currently, the only data modification case handled here is stripping of
!      * trailing resjunk fields, which we do in a slightly chintzy way by just
!      * adjusting the tuple's natts header field.  Possibly there will someday
!      * be a need for more-extensive rearrangements, in which case we'd
!      * probably use tupconvert.c.
!      */
!     Assert(variable->vartype != RECORDOID);
!     var_tupdesc = lookup_rowtype_tupdesc(variable->vartype, -1);

      tuple = ExecFetchSlotTuple(slot);

!     Assert(HeapTupleHeaderGetNatts(tuple->t_data) >= var_tupdesc->natts);

      /* Check to see if any dropped attributes are non-null */
      for (i = 0; i < var_tupdesc->natts; i++)
--- 993,1006 ----
              break;
      }

!     /* Apply the junkfilter if any */
!     if (wrvstate->wrv_junkFilter != NULL)
!         slot = ExecFilterJunk(wrvstate->wrv_junkFilter, slot);

      tuple = ExecFetchSlotTuple(slot);

!     Assert(variable->vartype != RECORDOID);
!     var_tupdesc = lookup_rowtype_tupdesc(variable->vartype, -1);

      /* Check to see if any dropped attributes are non-null */
      for (i = 0; i < var_tupdesc->natts; i++)
*************** ExecEvalWholeRowSlow(ExprState *exprstat
*** 930,937 ****

      /*
       * We have to make a copy of the tuple so we can safely insert the Datum
!      * overhead fields, which are not set in on-disk tuples; not to mention
!      * fooling with its natts field.
       */
      dtuple = (HeapTupleHeader) palloc(tuple->t_len);
      memcpy((char *) dtuple, (char *) tuple->t_data, tuple->t_len);
--- 1023,1029 ----

      /*
       * We have to make a copy of the tuple so we can safely insert the Datum
!      * overhead fields, which are not set in on-disk tuples.
       */
      dtuple = (HeapTupleHeader) palloc(tuple->t_len);
      memcpy((char *) dtuple, (char *) tuple->t_data, tuple->t_len);
*************** ExecEvalWholeRowSlow(ExprState *exprstat
*** 940,947 ****
      HeapTupleHeaderSetTypeId(dtuple, variable->vartype);
      HeapTupleHeaderSetTypMod(dtuple, variable->vartypmod);

-     HeapTupleHeaderSetNatts(dtuple, var_tupdesc->natts);
-
      ReleaseTupleDesc(var_tupdesc);

      return PointerGetDatum(dtuple);
--- 1032,1037 ----
*************** ExecEvalFieldSelect(FieldSelectState *fs
*** 3907,3913 ****
      }

      /* Check for type mismatch --- possible after ALTER COLUMN TYPE? */
!     /* As in ExecEvalVar, we should but can't check typmod */
      if (fselect->resulttype != attr->atttypid)
          ereport(ERROR,
                  (errmsg("attribute %d has wrong type", fieldnum),
--- 3997,4003 ----
      }

      /* Check for type mismatch --- possible after ALTER COLUMN TYPE? */
!     /* As in ExecEvalScalarVar, we should but can't check typmod */
      if (fselect->resulttype != attr->atttypid)
          ereport(ERROR,
                  (errmsg("attribute %d has wrong type", fieldnum),
*************** ExecInitExpr(Expr *node, PlanState *pare
*** 4236,4243 ****
      switch (nodeTag(node))
      {
          case T_Var:
!             state = (ExprState *) makeNode(ExprState);
!             state->evalfunc = ExecEvalVar;
              break;
          case T_Const:
              state = (ExprState *) makeNode(ExprState);
--- 4326,4345 ----
      switch (nodeTag(node))
      {
          case T_Var:
!             if (((Var *) node)->varattno == InvalidAttrNumber)
!             {
!                 WholeRowVarExprState *wstate = makeNode(WholeRowVarExprState);
!
!                 wstate->parent = parent;
!                 wstate->wrv_junkFilter = NULL;
!                 state = (ExprState *) wstate;
!                 state->evalfunc = (ExprStateEvalFunc) ExecEvalWholeRowVar;
!             }
!             else
!             {
!                 state = (ExprState *) makeNode(ExprState);
!                 state->evalfunc = ExecEvalScalarVar;
!             }
              break;
          case T_Const:
              state = (ExprState *) makeNode(ExprState);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index aa3916998718e88d6e0a4472c96f8a13f19864b3..09ed2cd01f1780d82181777a3bbb542ba71f01a0 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct GenericExprState
*** 561,566 ****
--- 561,577 ----
  } GenericExprState;

  /* ----------------
+  *        WholeRowVarExprState node
+  * ----------------
+  */
+ typedef struct WholeRowVarExprState
+ {
+     ExprState    xprstate;
+     struct PlanState *parent;    /* parent PlanState, or NULL if none */
+     JunkFilter *wrv_junkFilter;    /* JunkFilter to remove resjunk cols */
+ } WholeRowVarExprState;
+
+ /* ----------------
   *        AggrefExprState node
   * ----------------
   */
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index a51657df0d43487820b5c946b370458c2b9321ef..5e65922bcf8e34429e93acf8d7ed46ec0758f8a7 100644
*** a/src/include/nodes/nodes.h
--- b/src/include/nodes/nodes.h
*************** typedef enum NodeTag
*** 180,185 ****
--- 180,186 ----
       */
      T_ExprState = 400,
      T_GenericExprState,
+     T_WholeRowVarExprState,
      T_AggrefExprState,
      T_WindowFuncExprState,
      T_ArrayRefExprState,
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index ad870f665fc1ec6a367bb7c32a037e2df986d450..30cff04cb707b7610c14819e4232819f724e2d16 100644
*** a/src/test/regress/expected/subselect.out
--- b/src/test/regress/expected/subselect.out
*************** select (select (a.*)::text) from view_a
*** 505,510 ****
--- 505,535 ----
  (1 row)

  --
+ -- Check that whole-row Vars reading the result of a subselect don't include
+ -- any junk columns therein
+ --
+ select q from (select max(f1) from int4_tbl group by f1 order by f1) q;
+        q
+ ---------------
+  (-2147483647)
+  (-123456)
+  (0)
+  (123456)
+  (2147483647)
+ (5 rows)
+
+ with q as (select max(f1) from int4_tbl group by f1 order by f1)
+   select q from q;
+        q
+ ---------------
+  (-2147483647)
+  (-123456)
+  (0)
+  (123456)
+  (2147483647)
+ (5 rows)
+
+ --
  -- Test case for sublinks pushed down into subselects via join alias expansion
  --
  select
diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index 3cecbc1d41b1eb1134b525ca4a848cee50adf422..e07a30ed03baca56bf3c740f5d9fc616c75370a7 100644
*** a/src/test/regress/sql/subselect.sql
--- b/src/test/regress/sql/subselect.sql
*************** select (select (select view_a)) from vie
*** 325,330 ****
--- 325,339 ----
  select (select (a.*)::text) from view_a a;

  --
+ -- Check that whole-row Vars reading the result of a subselect don't include
+ -- any junk columns therein
+ --
+
+ select q from (select max(f1) from int4_tbl group by f1 order by f1) q;
+ with q as (select max(f1) from int4_tbl group by f1 order by f1)
+   select q from q;
+
+ --
  -- Test case for sublinks pushed down into subselects via join alias expansion
  --

On Fri, Jul 20, 2012 at 1:31 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> I think the way to solve this is to do whatever it takes to get access
>> to the subplan targetlist.  We could then do something a bit cleaner
>> than what the named-rowtype code is currently doing: if there are
>> resjunk columns in the subplan targetlist, use the tlist to create a
>> JunkFilter, and then pass the tuples through that.  After that we can
>> insist that the tuples don't have any extra columns.
>
> Here's a draft patch for that.  It wasn't quite as ugly as I feared.
> A lot of the apparent bulk of the patch is because I chose to split
> ExecEvalVar into separate functions for the scalar and whole-row
> cases, which seemed appropriate because they now get different
> ExprState node types.

Thanks for that!  Applying the patch and confirming the fix turned up
no issues. I did a perfunctory review and it all looks pretty good:
maybe ExecInitExpr could use a comment describing the
InvalidAttrNumber check though...it's somewhat common knowledge that
InvalidAttrNumber means row variables but it's also used to initialize
variables before loops scans and things like that.

merlin

Merlin Moncure <mmoncure@gmail.com> writes:
> On Fri, Jul 20, 2012 at 1:31 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Here's a draft patch for that.  It wasn't quite as ugly as I feared.
>> A lot of the apparent bulk of the patch is because I chose to split
>> ExecEvalVar into separate functions for the scalar and whole-row
>> cases, which seemed appropriate because they now get different
>> ExprState node types.

> Thanks for that!  Applying the patch and confirming the fix turned up
> no issues. I did a perfunctory review and it all looks pretty good:
> maybe ExecInitExpr could use a comment describing the
> InvalidAttrNumber check though...it's somewhat common knowledge that
> InvalidAttrNumber means row variables but it's also used to initialize
> variables before loops scans and things like that.

Thanks for testing.  I added the suggested comment and made some other
cosmetic improvements, and have committed this.
        regards, tom lane