Thread: row_to_json bug with index only scans: empty keys!
This is a serious bug in 9.3.5 and 9.4 beta3:
row_to_json() yields empty strings for json keys if the data is
fulfilled by an index only scan.
Example:
testjson=# select count(*) from document_acl;count
------- 426
(1 row)
testjson=# SELECT row_to_json(combined_rows) FROM (
SELECT uuid, user_id AS uid, permission
FROM document_acl_text AS acl
WHERE uuid = '8f774048-8936-4d7f-aa38-1974c91bbef2'
ORDER BY user_id ASC, permission ASC
) as combined_rows; row_to_json
---------------------------------------------------------------------{"":"8f774048-8936-4d7f-aa38-1974c91bbef2","":"admin","":"publish"}
(1 row)
testjson=# explain SELECT row_to_json(combined_rows) FROM (
SELECT uuid, user_id AS uid, permission
FROM document_acl_text AS acl
WHERE uuid = '8f774048-8936-4d7f-aa38-1974c91bbef2'
ORDER BY user_id ASC, permission ASC
) as combined_rows; QUERY PLAN
----------------------------------------------------------------------------------------------------------------Subquery
Scanon combined_rows (cost=0.27..8.30 rows=1 width=76) -> Index Only Scan using document_acl_text_pkey on
document_acl_textacl (cost=0.27..8.29 rows=1 width=52) Index Cond: (uuid =
'8f774048-8936-4d7f-aa38-1974c91bbef2'::text)Planningtime: 0.093 ms
(4 rows)
# set enable_indexonlyscan to off;
SET
testjson=# SELECT row_to_json(combined_rows) FROM (
SELECT uuid, user_id AS uid, permission
FROM document_acl_text AS acl
WHERE uuid = '8f774048-8936-4d7f-aa38-1974c91bbef2'
ORDER BY user_id ASC, permission ASC
) as combined_rows; row_to_json
------------------------------------------------------------------------------------------{"uuid":"8f774048-8936-4d7f-aa38-1974c91bbef2","user_id":"admin","permission":"publish"}
(1 row)
tjson=# explain SELECT row_to_json(combined_rows) FROM (
SELECT uuid, user_id AS uid, permission
FROM document_acl_text AS acl
WHERE uuid = '8f774048-8936-4d7f-aa38-1974c91bbef2'
ORDER BY user_id ASC, permission ASC
) as combined_rows; QUERY PLAN
-----------------------------------------------------------------------------------------------------------Subquery
Scanon combined_rows (cost=0.27..8.30 rows=1 width=76) -> Index Scan using document_acl_text_pkey on
document_acl_textacl (cost=0.27..8.29 rows=1 width=52) Index Cond: (uuid =
'8f774048-8936-4d7f-aa38-1974c91bbef2'::text)Planningtime: 0.095 ms
(4 rows)
We have a table defined as so:
CREATE TYPE permission_type AS ENUM ( 'publish'
);
create table "document_acl" ( "uuid" UUID, "user_id" TEXT, "permission" permission_type NOT NULL,
PRIMARYKEY ("uuid", "user_id", "permission"), FOREIGN KEY ("uuid") REFERENCES document_controls ("uuid")
);
The uuid and enums make no difference - I've made an all text version as well,
same problem.
testjson=# select version(); version
---------------------------------------------------------------------------------------------------------PostgreSQL
9.4beta3on x86_64-unknown-linux-gnu, compiled by gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2, 64-bit
(1 row)
Ross
--
Ross Reedstrom, Ph.D. reedstrm@rice.edu
Systems Engineer & Admin, Research Scientist phone: 713-348-6166
Connexions http://cnx.org fax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE
On 11/07/2014 10:51 AM, Ross Reedstrom wrote:
> This is a serious bug in 9.3.5 and 9.4 beta3:
>
> row_to_json() yields empty strings for json keys if the data is
> fulfilled by an index only scan.
>
> Example:
>
> testjson=# select count(*) from document_acl;
> count
> -------
> 426
> (1 row)
>
> testjson=# SELECT row_to_json(combined_rows) FROM (
> SELECT uuid, user_id AS uid, permission
> FROM document_acl_text AS acl
> WHERE uuid = '8f774048-8936-4d7f-aa38-1974c91bbef2'
> ORDER BY user_id ASC, permission ASC
> ) as combined_rows;
> row_to_json
> ---------------------------------------------------------------------
> {"":"8f774048-8936-4d7f-aa38-1974c91bbef2","":"admin","":"publish"}
That seems odd. Here's what the relevant code does:
td = DatumGetHeapTupleHeader(composite);
/* Extract rowtype info and find a tupdesc */ tupType = HeapTupleHeaderGetTypeId(td); tupTypmod =
HeapTupleHeaderGetTypMod(td); tupdesc = lookup_rowtype_tupdesc(tupType, tupTypmod);
... for (i = 0; i < tupdesc->natts; i++) ...
attname = NameStr(tupdesc->attrs[i]->attname); escape_json(result, attname);
Could this be a bug in lookup_rowtype_tupdesc()?
cheers
andrew
On 11/07/2014 11:17 AM, Andrew Dunstan wrote:
>
> On 11/07/2014 10:51 AM, Ross Reedstrom wrote:
>> This is a serious bug in 9.3.5 and 9.4 beta3:
>>
>> row_to_json() yields empty strings for json keys if the data is
>> fulfilled by an index only scan.
>>
>> Example:
>>
>> testjson=# select count(*) from document_acl;
>> count
>> -------
>> 426
>> (1 row)
>>
>> testjson=# SELECT row_to_json(combined_rows) FROM (
>> SELECT uuid, user_id AS uid, permission
>> FROM document_acl_text AS acl
>> WHERE uuid = '8f774048-8936-4d7f-aa38-1974c91bbef2'
>> ORDER BY user_id ASC, permission ASC
>> ) as combined_rows;
>> row_to_json
>> ---------------------------------------------------------------------
>> {"":"8f774048-8936-4d7f-aa38-1974c91bbef2","":"admin","":"publish"}
>
>
> That seems odd. Here's what the relevant code does:
>
> td = DatumGetHeapTupleHeader(composite);
>
> /* Extract rowtype info and find a tupdesc */
> tupType = HeapTupleHeaderGetTypeId(td);
> tupTypmod = HeapTupleHeaderGetTypMod(td);
> tupdesc = lookup_rowtype_tupdesc(tupType, tupTypmod);
>
> ...
> for (i = 0; i < tupdesc->natts; i++)
> ...
>
> attname = NameStr(tupdesc->attrs[i]->attname);
> escape_json(result, attname);
>
> Could this be a bug in lookup_rowtype_tupdesc()?
>
>
>
Further data point:
There's nothing json-specific about this, BTW:
andrew=# select hstore(q) from (select * from idxo order by a) q; hstore --------- ""=>"1" (1 row)
andrew=# set enable_seqscan = true; SET andrew=# select hstore(q) from (select * from idxo order by a) q;
hstore ------------------------------ "a"=>"1", "b"=>"b", "c"=>"c" (1 row)
So it looks like the index scan only stuff is broken somewhere.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/07/2014 10:51 AM, Ross Reedstrom wrote:
>> row_to_json() yields empty strings for json keys if the data is
>> fulfilled by an index only scan.
> Could this be a bug in lookup_rowtype_tupdesc()?
I think this is probably a variant of bug #11210, in which the problem is
that tupledescs bubbled up from inheritance children never get column
names assigned to them. I've been speculating about ways to fix that
but I've not thought of anything that's not kinda painful.
regards, tom lane
On 11/07/2014 04:59 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 11/07/2014 10:51 AM, Ross Reedstrom wrote: >>> row_to_json() yields empty strings for json keys if the data is >>> fulfilled by an index only scan. >> Could this be a bug in lookup_rowtype_tupdesc()? > I think this is probably a variant of bug #11210, in which the problem is > that tupledescs bubbled up from inheritance children never get column > names assigned to them. I've been speculating about ways to fix that > but I've not thought of anything that's not kinda painful. Yeah, I've been running this down a bit with a debugger, and it looked kinda like that. Maybe we should look for this in places we know it matters (e.g. hstore_from_record() and composite_to_json() and raise an error if we find empty names, with a hint to do (what?). cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/07/2014 04:59 PM, Tom Lane wrote:
>> I think this is probably a variant of bug #11210, in which the problem is
>> that tupledescs bubbled up from inheritance children never get column
>> names assigned to them. I've been speculating about ways to fix that
>> but I've not thought of anything that's not kinda painful.
> Yeah, I've been running this down a bit with a debugger, and it looked
> kinda like that.
After chewing on this for awhile, it occurred to me that the problem could
be solved in ExecEvalWholeRowVar, which is really the only place that
produces composite datums "from scratch". We can look up the RTE that
the whole-row Var references and scrape column aliases from it. The
attached draft patch does that, and appears to fix both Ross' example
and the one in bug #11210.
Although this patch doesn't change any existing regression test outputs
as far as I can find, it's not hard to think of cases where it will change
the results. In particular:
regression=# create table foo (f1 int, f2 int);
CREATE TABLE
regression=# insert into foo values(1,2);
INSERT 0 1
regression=# select row_to_json(f) from foo f;
row_to_json
-----------------
{"f1":1,"f2":2}
(1 row)
regression=# select row_to_json(f) from foo f(x,y);
row_to_json
---------------
{"x":1,"y":2}
(1 row)
Without this patch, you get the same result from the second query as
from the first. I argue that using the column aliases is clearly saner
behavior; I think there have been complaints about that in the past,
but am too lazy to trawl the archives right now. However, it's also
clear that people might have applications that are expecting the old
behavior and don't realize that it might be thought a bug.
I'm hesitant to back-patch this anyway, because I'm not sure that there
aren't any other unexpected side-effects. Note in particular the
assertion I had to weaken in ExecEvalConvertRowtype because a whole-row
Var is now capable of returning tuple datums that are marked with a
different rowtype than the parser marked the Var with. That's a little
bit scary ...
On the positive side, this lets us remove some kluges, like the place in
nodeFunctionscan.c where we were hacking the output tupledesc's column
names in pretty much the same way this patch has ExecEvalWholeRowVar do.
I thought that was a crock when it went in, because no other plan node
is doing anything like that, so it's good to be able to remove it.
Thoughts?
regards, tom lane
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 7cfa63f..3ab460c 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
***************
*** 50,55 ****
--- 50,56 ----
#include "nodes/nodeFuncs.h"
#include "optimizer/planner.h"
#include "parser/parse_coerce.h"
+ #include "parser/parsetree.h"
#include "pgstat.h"
#include "utils/acl.h"
#include "utils/builtins.h"
*************** ExecEvalWholeRowVar(WholeRowVarExprState
*** 712,717 ****
--- 713,720 ----
{
Var *variable = (Var *) wrvstate->xprstate.expr;
TupleTableSlot *slot;
+ TupleDesc output_tupdesc;
+ MemoryContext oldcontext;
bool needslow = false;
if (isDone)
*************** ExecEvalWholeRowVar(WholeRowVarExprState
*** 787,794 ****
/* 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,
--- 790,795 ----
*************** ExecEvalWholeRowVar(WholeRowVarExprState
*** 860,869 ****
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
--- 861,946 ----
needslow = true; /* need runtime check for null */
}
+ /*
+ * Use the variable's declared rowtype as the descriptor for the
+ * output values, modulo possibly assigning new column names below. In
+ * particular, we *must* absorb any attisdropped markings.
+ */
+ oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
+ output_tupdesc = CreateTupleDescCopy(var_tupdesc);
+ MemoryContextSwitchTo(oldcontext);
+
ReleaseTupleDesc(var_tupdesc);
}
+ else
+ {
+ /*
+ * In the RECORD case, we use the input slot's rowtype as the
+ * descriptor for the output values, modulo possibly assigning new
+ * column names below.
+ */
+ oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
+ output_tupdesc = CreateTupleDescCopy(slot->tts_tupleDescriptor);
+ MemoryContextSwitchTo(oldcontext);
+ }
! /*
! * Construct a tuple descriptor for the composite values we'll produce,
! * and make sure its record type is "blessed". The main reason to do this
! * is to be sure that operations such as row_to_json() will see the
! * desired column names when they look up the descriptor from the type
! * information embedded in the composite values.
! *
! * We already got the correct physical datatype info above, but now we
! * should try to find the source RTE and adopt its column aliases, in case
! * they are different from the original rowtype's names. For example, in
! * "SELECT foo(t) FROM tab t(x,y)", the first two columns in the composite
! * output should be named "x" and "y" regardless of tab's column names.
! *
! * If we can't locate the RTE, assume the column names we've got are OK.
! * (As of this writing, the only cases where we can't locate the RTE are
! * in execution of trigger WHEN clauses, and then the Var will have the
! * trigger's relation's rowtype, so its names are fine.)
! */
! if (econtext->ecxt_estate &&
! variable->varno <= list_length(econtext->ecxt_estate->es_range_table))
! {
! RangeTblEntry *rte = rt_fetch(variable->varno,
! econtext->ecxt_estate->es_range_table);
! bool modified = false;
! int colno = 0;
! ListCell *lc;
!
! Assert(output_tupdesc->natts >= list_length(rte->eref->colnames));
! foreach(lc, rte->eref->colnames)
! {
! char *cname = strVal(lfirst(lc));
! Form_pg_attribute attr = output_tupdesc->attrs[colno++];
!
! /* Ignore empty aliases (these must be for dropped columns) */
! if (cname[0] == '\0')
! continue;
! /* Change tupdesc only if alias is actually different */
! if (strcmp(cname, NameStr(attr->attname)) != 0)
! {
! namestrcpy(&(attr->attname), cname);
! modified = true;
! }
! }
! /* If we modified the tupdesc, it's now a new record type */
! if (modified)
! {
! output_tupdesc->tdtypeid = RECORDOID;
! output_tupdesc->tdtypmod = -1;
! }
! }
!
! /* Bless the tupdesc if needed, and save it in the execution state */
! if (output_tupdesc->tdtypeid == RECORDOID && output_tupdesc->tdtypmod < 0)
! assign_record_type_typmod(output_tupdesc);
! wrvstate->wrv_tupdesc = output_tupdesc;
!
! /* Skip all the above on future executions of node */
if (needslow)
wrvstate->xprstate.evalfunc = (ExprStateEvalFunc) ExecEvalWholeRowSlow;
else
*************** ExecEvalWholeRowFast(WholeRowVarExprStat
*** 886,892 ****
{
Var *variable = (Var *) wrvstate->xprstate.expr;
TupleTableSlot *slot;
- TupleDesc slot_tupdesc;
HeapTupleHeader dtuple;
if (isDone)
--- 963,968 ----
*************** ExecEvalWholeRowFast(WholeRowVarExprStat
*** 917,949 ****
slot = ExecFilterJunk(wrvstate->wrv_junkFilter, slot);
/*
- * 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. (Note: we must
- * do this here, not in ExecEvalWholeRowVar, because some plan trees may
- * return different slots at different times. We have to be ready to
- * bless additional slots during the run.)
- */
- slot_tupdesc = slot->tts_tupleDescriptor;
- if (variable->vartype == RECORDOID &&
- slot_tupdesc->tdtypeid == RECORDOID &&
- slot_tupdesc->tdtypmod < 0)
- assign_record_type_typmod(slot_tupdesc);
-
- /*
* Copy the slot tuple and make sure any toasted fields get detoasted.
*/
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * If the Var identifies a named composite type, label the datum with that
! * type; otherwise we'll use the slot's info.
*/
! if (variable->vartype != RECORDOID)
! {
! HeapTupleHeaderSetTypeId(dtuple, variable->vartype);
! HeapTupleHeaderSetTypMod(dtuple, variable->vartypmod);
! }
return PointerGetDatum(dtuple);
}
--- 993,1007 ----
slot = ExecFilterJunk(wrvstate->wrv_junkFilter, slot);
/*
* Copy the slot tuple and make sure any toasted fields get detoasted.
*/
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * Label the datum with the composite type info we identified before.
*/
! HeapTupleHeaderSetTypeId(dtuple, wrvstate->wrv_tupdesc->tdtypeid);
! HeapTupleHeaderSetTypMod(dtuple, wrvstate->wrv_tupdesc->tdtypmod);
return PointerGetDatum(dtuple);
}
*************** ExecEvalWholeRowSlow(WholeRowVarExprStat
*** 997,1004 ****
tuple = ExecFetchSlotTuple(slot);
tupleDesc = slot->tts_tupleDescriptor;
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++)
--- 1055,1063 ----
tuple = ExecFetchSlotTuple(slot);
tupleDesc = slot->tts_tupleDescriptor;
+ /* wrv_tupdesc is a good enough representation of the Var's rowtype */
Assert(variable->vartype != RECORDOID);
! var_tupdesc = wrvstate->wrv_tupdesc;
/* Check to see if any dropped attributes are non-null */
for (i = 0; i < var_tupdesc->natts; i++)
*************** ExecEvalWholeRowSlow(WholeRowVarExprStat
*** 1025,1036 ****
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * Reset datum's type ID fields to match the Var.
*/
! HeapTupleHeaderSetTypeId(dtuple, variable->vartype);
! HeapTupleHeaderSetTypMod(dtuple, variable->vartypmod);
!
! ReleaseTupleDesc(var_tupdesc);
return PointerGetDatum(dtuple);
}
--- 1084,1093 ----
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * Label the datum with the composite type info we identified before.
*/
! HeapTupleHeaderSetTypeId(dtuple, wrvstate->wrv_tupdesc->tdtypeid);
! HeapTupleHeaderSetTypMod(dtuple, wrvstate->wrv_tupdesc->tdtypmod);
return PointerGetDatum(dtuple);
}
*************** ExecEvalConvertRowtype(ConvertRowtypeExp
*** 2850,2857 ****
cstate->initialized = false;
}
! Assert(HeapTupleHeaderGetTypeId(tuple) == cstate->indesc->tdtypeid);
! Assert(HeapTupleHeaderGetTypMod(tuple) == cstate->indesc->tdtypmod);
/* if first time through, initialize conversion map */
if (!cstate->initialized)
--- 2907,2920 ----
cstate->initialized = false;
}
! /*
! * We used to be able to assert that incoming tuples are marked with
! * exactly the rowtype of cstate->indesc. However, now that
! * ExecEvalWholeRowVar might change the tuples' marking to plain RECORD
! * due to inserting aliases, we can only make this weak test:
! */
! Assert(HeapTupleHeaderGetTypeId(tuple) == cstate->indesc->tdtypeid ||
! HeapTupleHeaderGetTypeId(tuple) == RECORDOID);
/* if first time through, initialize conversion map */
if (!cstate->initialized)
*************** ExecInitExpr(Expr *node, PlanState *pare
*** 4375,4380 ****
--- 4438,4444 ----
WholeRowVarExprState *wstate = makeNode(WholeRowVarExprState);
wstate->parent = parent;
+ wstate->wrv_tupdesc = NULL;
wstate->wrv_junkFilter = NULL;
state = (ExprState *) wstate;
state->evalfunc = (ExprStateEvalFunc) ExecEvalWholeRowVar;
diff --git a/src/backend/executor/nodeFunctionscan.c b/src/backend/executor/nodeFunctionscan.c
index 945a414..3a7f0ae 100644
*** a/src/backend/executor/nodeFunctionscan.c
--- b/src/backend/executor/nodeFunctionscan.c
*************** FunctionScanState *
*** 279,286 ****
ExecInitFunctionScan(FunctionScan *node, EState *estate, int eflags)
{
FunctionScanState *scanstate;
- RangeTblEntry *rte = rt_fetch(node->scan.scanrelid,
- estate->es_range_table);
int nfuncs = list_length(node->functions);
TupleDesc scan_tupdesc;
int i,
--- 279,284 ----
*************** ExecInitFunctionScan(FunctionScan *node,
*** 494,515 ****
Assert(attno == natts);
}
- /*
- * Make sure the scan result tupdesc has the column names the query
- * expects. This affects the output of constructs like row_to_json which
- * read the column names from the passed-in tupdesc.
- */
- i = 0;
- foreach(lc, rte->eref->colnames)
- {
- char *attname = strVal(lfirst(lc));
-
- if (i >= scan_tupdesc->natts)
- break; /* shouldn't happen, but just in case */
- namestrcpy(&(scan_tupdesc->attrs[i]->attname), attname);
- i++;
- }
-
ExecAssignScanType(&scanstate->ss, scan_tupdesc);
/*
--- 492,497 ----
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 39d2c10..6d8cb93 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct WholeRowVarExprState
*** 577,582 ****
--- 577,583 ----
{
ExprState xprstate;
struct PlanState *parent; /* parent PlanState, or NULL if none */
+ TupleDesc wrv_tupdesc; /* descriptor for resulting tuples */
JunkFilter *wrv_junkFilter; /* JunkFilter to remove resjunk cols */
} WholeRowVarExprState;
On Fri, Nov 7, 2014 at 11:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thoughts? I'm not sure whether this is safe enough to back-patch, but it seems like we should probably plan to back-patch *something*, because the status quo isn't great either. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 11/08/2014 09:26 AM, Robert Haas wrote: > On Fri, Nov 7, 2014 at 11:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Thoughts? > I'm not sure whether this is safe enough to back-patch, but it seems > like we should probably plan to back-patch *something*, because the > status quo isn't great either. > I confirm that Tom's patch does indeed fix my test case that produces empty field names. We should probably not backpatch it, as it is a behaviour change. However, I do think we should add checks in composite_to_json and hstore_from_record for empty field names, and error out if they are found. That seems like an outright bug which we should defend against, including in the back branches. Giving back json or hstore objects with made up names like f1 is one thing, giving them back with empty names (which, in the hstore case will collapse everything to a single field) is far worse. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/08/2014 09:26 AM, Robert Haas wrote:
>> I'm not sure whether this is safe enough to back-patch, but it seems
>> like we should probably plan to back-patch *something*, because the
>> status quo isn't great either.
> I confirm that Tom's patch does indeed fix my test case that produces
> empty field names.
> We should probably not backpatch it, as it is a behaviour change.
> However, I do think we should add checks in composite_to_json and
> hstore_from_record for empty field names, and error out if they are
> found.
That seems like a pretty silly move: it wouldn't actually fix anything,
and it would break cases that perhaps are acceptable to users today.
We could reduce the risks involved by narrowing the cases in which
ExecEvalWholeRowVar will replace field names it got from the input.
I'd be inclined to propose:
1. If Var is of a named composite type, use *exactly* the field names
associated with that type. (This avoids the need to possibly produce
RECORD outputs from a named-type Var, thus removing the Assert-weakening
issue.)
2. If Var is of type RECORD, replace only empty field names with aliases
from the RTE. (This might sound inconsistent --- could you end up with
some names coming from point A and some from point B? --- but in practice
I think it would always be all-or-nothing, because the issue is whether
or not the planner bothered to attach column names to a lower-level
targetlist.)
regards, tom lane
On 11/08/2014 11:24 AM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 11/08/2014 09:26 AM, Robert Haas wrote: >>> I'm not sure whether this is safe enough to back-patch, but it seems >>> like we should probably plan to back-patch *something*, because the >>> status quo isn't great either. >> I confirm that Tom's patch does indeed fix my test case that produces >> empty field names. >> We should probably not backpatch it, as it is a behaviour change. >> However, I do think we should add checks in composite_to_json and >> hstore_from_record for empty field names, and error out if they are >> found. > That seems like a pretty silly move: it wouldn't actually fix anything, > and it would break cases that perhaps are acceptable to users today. What evidence do you have that it might be acceptable to today's users? The only evidence we have that I know of is Ross' complaint that indicates that it's not acceptable. However, > > We could reduce the risks involved by narrowing the cases in which > ExecEvalWholeRowVar will replace field names it got from the input. > I'd be inclined to propose: > > 1. If Var is of a named composite type, use *exactly* the field names > associated with that type. (This avoids the need to possibly produce > RECORD outputs from a named-type Var, thus removing the Assert-weakening > issue.) > > 2. If Var is of type RECORD, replace only empty field names with aliases > from the RTE. (This might sound inconsistent --- could you end up with > some names coming from point A and some from point B? --- but in practice > I think it would always be all-or-nothing, because the issue is whether > or not the planner bothered to attach column names to a lower-level > targetlist.) > > I assume that's what you would propose for just the stable branches, and that going forward we'd always use the aliases from the RTE? Or maybe I'm not quite understanding enough which cases will be covered. To the extent that I do this sounds OK. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/08/2014 11:24 AM, Tom Lane wrote:
>> That seems like a pretty silly move: it wouldn't actually fix anything,
>> and it would break cases that perhaps are acceptable to users today.
> What evidence do you have that it might be acceptable to today's users?
> The only evidence we have that I know of is Ross' complaint that
> indicates that it's not acceptable.
The fact that we've only gotten two bug reports in however many years the
failure has been possible might mean that few people encounter the case,
or it might mean that it doesn't break things for their usages so badly
that they need to complain. If the latter, throwing an error rather than
fixing it is not going to be an improvement.
> I assume that's what you would propose for just the stable branches, and
> that going forward we'd always use the aliases from the RTE?
That would be my druthers. But given the lack of complaints, maybe we
should just stick to the more-backwards-compatible behavior until someone
does complain. Thoughts?
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> > Andrew Dunstan <andrew@dunslane.net> writes: >> I assume that's what you would propose for just the stable branches, and >> that going forward we'd always use the aliases from the RTE? > > That would be my druthers. But given the lack of complaints, maybe we > should just stick to the more-backwards-compatible behavior until someone > does complain. Thoughts? I think consistent use of the aliases would be less surprising and should be what we do on master. I agree that we should avoid breaking anything that might be working as intended on stable branches. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 11/08/2014 12:14 PM, Tom Lane wrote:
>> I assume that's what you would propose for just the stable branches, and
>> that going forward we'd always use the aliases from the RTE?
> That would be my druthers. But given the lack of complaints, maybe we
> should just stick to the more-backwards-compatible behavior until someone
> does complain. Thoughts?
>
>
Wouldn't that would mean we might not pick up the expected aliases in
select row_to_json(q) from (select a,b from foo) as q(x,y)
? If so, I'm definitely in favor of fixing this now.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/08/2014 12:14 PM, Tom Lane wrote:
>> That would be my druthers. But given the lack of complaints, maybe we
>> should just stick to the more-backwards-compatible behavior until someone
>> does complain. Thoughts?
> Wouldn't that would mean we might not pick up the expected aliases in
> select row_to_json(q) from (select a,b from foo) as q(x,y)
> ? If so, I'm definitely in favor of fixing this now.
Well, it's inconsistent now. In existing releases you might or might not
get x,y:
regression=# create table foo (f1 int, f2 int);
CREATE TABLE
regression=# insert into foo values(1,2);
INSERT 0 1
regression=# select row_to_json(q) from (select f1 as a, f2 as b from foo) as q(x,y); row_to_json
---------------{"x":1,"y":2}
(1 row)
regression=# select row_to_json(q) from (select f1 as a, f2 as b from foo offset 0) as q(x,y); row_to_json
-----------------{"f1":1,"f2":2}
(1 row)
That seems like something that's worth fixing going forward, but it's a
lot harder to make the case that we should change it in back branches.
regards, tom lane
On 11/08/2014 12:40 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 11/08/2014 12:14 PM, Tom Lane wrote:
>>> That would be my druthers. But given the lack of complaints, maybe we
>>> should just stick to the more-backwards-compatible behavior until someone
>>> does complain. Thoughts?
>> Wouldn't that would mean we might not pick up the expected aliases in
>> select row_to_json(q) from (select a,b from foo) as q(x,y)
>> ? If so, I'm definitely in favor of fixing this now.
> Well, it's inconsistent now. In existing releases you might or might not
> get x,y:
>
> regression=# create table foo (f1 int, f2 int);
> CREATE TABLE
> regression=# insert into foo values(1,2);
> INSERT 0 1
> regression=# select row_to_json(q) from (select f1 as a, f2 as b from foo) as q(x,y);
> row_to_json
> ---------------
> {"x":1,"y":2}
> (1 row)
>
> regression=# select row_to_json(q) from (select f1 as a, f2 as b from foo offset 0) as q(x,y);
> row_to_json
> -----------------
> {"f1":1,"f2":2}
> (1 row)
>
> That seems like something that's worth fixing going forward, but it's a
> lot harder to make the case that we should change it in back branches.
>
>
Sure. As long as we fix the empty alias issue in the back branches, just
fixing this prospectively seems fine. But I don't think we should wait
for more complaints.
cheers
andrew
Oh, some more fun: a RowExpr that's labeled with a named composite type
(rather than RECORD) has the same issue of not respecting aliases.
Continuing previous example with table "foo":
regression=# create view vv as select * from foo;
CREATE VIEW
regression=# select row_to_json(q) from vv q; row_to_json
-----------------{"f1":1,"f2":2}
(1 row)
regression=# select row_to_json(q) from vv q(a,b); row_to_json
-----------------{"f1":1,"f2":2}
(1 row)
So that's another case we probably want to change in HEAD but not the back
branches.
regards, tom lane
I've no opinion on the not respecting aliases aspect of this, but both
the hstore and json emtpy keys case breaks the format: it's duplicate keys
that collapse to a single value, and expected keynames are missing.
The insidious bit about this bug though is that it works fine during testing
with the developers typically small data sets. It's only triggered in my case
when we the plan switches to index-only. Even an index scan works fine. I can't
imagine that there is code out there that _depends_ on this behavior. Just as
likely to me are that there exist systems that just have "can't reproduce" bugs
that would be fixed by this.
Ross
On Sat, Nov 08, 2014 at 01:09:23PM -0500, Tom Lane wrote:
> Oh, some more fun: a RowExpr that's labeled with a named composite type
> (rather than RECORD) has the same issue of not respecting aliases.
> Continuing previous example with table "foo":
>
> regression=# create view vv as select * from foo;
> CREATE VIEW
> regression=# select row_to_json(q) from vv q;
> row_to_json
> -----------------
> {"f1":1,"f2":2}
> (1 row)
>
> regression=# select row_to_json(q) from vv q(a,b);
> row_to_json
> -----------------
> {"f1":1,"f2":2}
> (1 row)
>
> So that's another case we probably want to change in HEAD but not the back
> branches.
>
> regards, tom lane
>
--
Ross Reedstrom, Ph.D. reedstrm@rice.edu
Systems Engineer & Admin, Research Scientist phone: 713-348-6166
Connexions http://cnx.org fax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE
On 11/08/2014 04:19 PM, Ross Reedstrom wrote: > I've no opinion on the not respecting aliases aspect of this, but both > the hstore and json emtpy keys case breaks the format: it's duplicate keys > that collapse to a single value, and expected keynames are missing. > > The insidious bit about this bug though is that it works fine during testing > with the developers typically small data sets. It's only triggered in my case > when we the plan switches to index-only. Even an index scan works fine. I can't > imagine that there is code out there that _depends_ on this behavior. Just as > likely to me are that there exist systems that just have "can't reproduce" bugs > that would be fixed by this. > > No, I can't imagine it either - it's utterly broken. That's the piece that Tom is proposing to fix on the back branches. AIUI. cheers andrew
I wrote:
> We could reduce the risks involved by narrowing the cases in which
> ExecEvalWholeRowVar will replace field names it got from the input.
> I'd be inclined to propose:
> 1. If Var is of a named composite type, use *exactly* the field names
> associated with that type. (This avoids the need to possibly produce
> RECORD outputs from a named-type Var, thus removing the Assert-weakening
> issue.)
> 2. If Var is of type RECORD, replace only empty field names with aliases
> from the RTE. (This might sound inconsistent --- could you end up with
> some names coming from point A and some from point B? --- but in practice
> I think it would always be all-or-nothing, because the issue is whether
> or not the planner bothered to attach column names to a lower-level
> targetlist.)
Attached are patches meant for HEAD and 9.2-9.4 respectively. The HEAD
patch extends the prior patch to fix the RowExpr case I mentioned
yesterday. The back-branch patch works as suggested above. I added a
bunch of regression tests that document behavior in this area. The two
patches include the same set of tests but have different expected output
for the cases we are intentionally not fixing in back branches. If you
try the back-branch regression tests on an unpatched backend, you can
verify that the only cases that change behavior are ones where current
sources put out empty field names.
The test cases use row_to_json() so they could not be used directly before
9.2. I tried the same cases using hstore(record) in 9.1. While 9.1 does
some pretty darn dubious things, it does not produce empty field names in
any of these cases, so I think we probably should not consider
back-patching further than 9.2.
regards, tom lane
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 7cfa63f..88af735 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
***************
*** 50,55 ****
--- 50,56 ----
#include "nodes/nodeFuncs.h"
#include "optimizer/planner.h"
#include "parser/parse_coerce.h"
+ #include "parser/parsetree.h"
#include "pgstat.h"
#include "utils/acl.h"
#include "utils/builtins.h"
*************** ExecEvalWholeRowVar(WholeRowVarExprState
*** 712,717 ****
--- 713,720 ----
{
Var *variable = (Var *) wrvstate->xprstate.expr;
TupleTableSlot *slot;
+ TupleDesc output_tupdesc;
+ MemoryContext oldcontext;
bool needslow = false;
if (isDone)
*************** ExecEvalWholeRowVar(WholeRowVarExprState
*** 787,794 ****
/* 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,
--- 790,795 ----
*************** ExecEvalWholeRowVar(WholeRowVarExprState
*** 860,869 ****
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
--- 861,920 ----
needslow = true; /* need runtime check for null */
}
+ /*
+ * Use the variable's declared rowtype as the descriptor for the
+ * output values, modulo possibly assigning new column names below. In
+ * particular, we *must* absorb any attisdropped markings.
+ */
+ oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
+ output_tupdesc = CreateTupleDescCopy(var_tupdesc);
+ MemoryContextSwitchTo(oldcontext);
+
ReleaseTupleDesc(var_tupdesc);
}
+ else
+ {
+ /*
+ * In the RECORD case, we use the input slot's rowtype as the
+ * descriptor for the output values, modulo possibly assigning new
+ * column names below.
+ */
+ oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
+ output_tupdesc = CreateTupleDescCopy(slot->tts_tupleDescriptor);
+ MemoryContextSwitchTo(oldcontext);
+ }
! /*
! * Construct a tuple descriptor for the composite values we'll produce,
! * and make sure its record type is "blessed". The main reason to do this
! * is to be sure that operations such as row_to_json() will see the
! * desired column names when they look up the descriptor from the type
! * information embedded in the composite values.
! *
! * We already got the correct physical datatype info above, but now we
! * should try to find the source RTE and adopt its column aliases, in case
! * they are different from the original rowtype's names. For example, in
! * "SELECT foo(t) FROM tab t(x,y)", the first two columns in the composite
! * output should be named "x" and "y" regardless of tab's column names.
! *
! * If we can't locate the RTE, assume the column names we've got are OK.
! * (As of this writing, the only cases where we can't locate the RTE are
! * in execution of trigger WHEN clauses, and then the Var will have the
! * trigger's relation's rowtype, so its names are fine.)
! */
! if (econtext->ecxt_estate &&
! variable->varno <= list_length(econtext->ecxt_estate->es_range_table))
! {
! RangeTblEntry *rte = rt_fetch(variable->varno,
! econtext->ecxt_estate->es_range_table);
!
! ExecTypeSetColNames(output_tupdesc, rte->eref->colnames);
! }
!
! /* Bless the tupdesc if needed, and save it in the execution state */
! wrvstate->wrv_tupdesc = BlessTupleDesc(output_tupdesc);
!
! /* Skip all the above on future executions of node */
if (needslow)
wrvstate->xprstate.evalfunc = (ExprStateEvalFunc) ExecEvalWholeRowSlow;
else
*************** ExecEvalWholeRowFast(WholeRowVarExprStat
*** 886,892 ****
{
Var *variable = (Var *) wrvstate->xprstate.expr;
TupleTableSlot *slot;
- TupleDesc slot_tupdesc;
HeapTupleHeader dtuple;
if (isDone)
--- 937,942 ----
*************** ExecEvalWholeRowFast(WholeRowVarExprStat
*** 917,949 ****
slot = ExecFilterJunk(wrvstate->wrv_junkFilter, slot);
/*
- * 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. (Note: we must
- * do this here, not in ExecEvalWholeRowVar, because some plan trees may
- * return different slots at different times. We have to be ready to
- * bless additional slots during the run.)
- */
- slot_tupdesc = slot->tts_tupleDescriptor;
- if (variable->vartype == RECORDOID &&
- slot_tupdesc->tdtypeid == RECORDOID &&
- slot_tupdesc->tdtypmod < 0)
- assign_record_type_typmod(slot_tupdesc);
-
- /*
* Copy the slot tuple and make sure any toasted fields get detoasted.
*/
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * If the Var identifies a named composite type, label the datum with that
! * type; otherwise we'll use the slot's info.
*/
! if (variable->vartype != RECORDOID)
! {
! HeapTupleHeaderSetTypeId(dtuple, variable->vartype);
! HeapTupleHeaderSetTypMod(dtuple, variable->vartypmod);
! }
return PointerGetDatum(dtuple);
}
--- 967,981 ----
slot = ExecFilterJunk(wrvstate->wrv_junkFilter, slot);
/*
* Copy the slot tuple and make sure any toasted fields get detoasted.
*/
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * Label the datum with the composite type info we identified before.
*/
! HeapTupleHeaderSetTypeId(dtuple, wrvstate->wrv_tupdesc->tdtypeid);
! HeapTupleHeaderSetTypMod(dtuple, wrvstate->wrv_tupdesc->tdtypmod);
return PointerGetDatum(dtuple);
}
*************** ExecEvalWholeRowSlow(WholeRowVarExprStat
*** 997,1004 ****
tuple = ExecFetchSlotTuple(slot);
tupleDesc = slot->tts_tupleDescriptor;
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++)
--- 1029,1037 ----
tuple = ExecFetchSlotTuple(slot);
tupleDesc = slot->tts_tupleDescriptor;
+ /* wrv_tupdesc is a good enough representation of the Var's rowtype */
Assert(variable->vartype != RECORDOID);
! var_tupdesc = wrvstate->wrv_tupdesc;
/* Check to see if any dropped attributes are non-null */
for (i = 0; i < var_tupdesc->natts; i++)
*************** ExecEvalWholeRowSlow(WholeRowVarExprStat
*** 1025,1036 ****
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * Reset datum's type ID fields to match the Var.
*/
! HeapTupleHeaderSetTypeId(dtuple, variable->vartype);
! HeapTupleHeaderSetTypMod(dtuple, variable->vartypmod);
!
! ReleaseTupleDesc(var_tupdesc);
return PointerGetDatum(dtuple);
}
--- 1058,1067 ----
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * Label the datum with the composite type info we identified before.
*/
! HeapTupleHeaderSetTypeId(dtuple, wrvstate->wrv_tupdesc->tdtypeid);
! HeapTupleHeaderSetTypMod(dtuple, wrvstate->wrv_tupdesc->tdtypmod);
return PointerGetDatum(dtuple);
}
*************** ExecEvalConvertRowtype(ConvertRowtypeExp
*** 2850,2857 ****
cstate->initialized = false;
}
! Assert(HeapTupleHeaderGetTypeId(tuple) == cstate->indesc->tdtypeid);
! Assert(HeapTupleHeaderGetTypMod(tuple) == cstate->indesc->tdtypmod);
/* if first time through, initialize conversion map */
if (!cstate->initialized)
--- 2881,2894 ----
cstate->initialized = false;
}
! /*
! * We used to be able to assert that incoming tuples are marked with
! * exactly the rowtype of cstate->indesc. However, now that
! * ExecEvalWholeRowVar might change the tuples' marking to plain RECORD
! * due to inserting aliases, we can only make this weak test:
! */
! Assert(HeapTupleHeaderGetTypeId(tuple) == cstate->indesc->tdtypeid ||
! HeapTupleHeaderGetTypeId(tuple) == RECORDOID);
/* if first time through, initialize conversion map */
if (!cstate->initialized)
*************** ExecInitExpr(Expr *node, PlanState *pare
*** 4375,4380 ****
--- 4412,4418 ----
WholeRowVarExprState *wstate = makeNode(WholeRowVarExprState);
wstate->parent = parent;
+ wstate->wrv_tupdesc = NULL;
wstate->wrv_junkFilter = NULL;
state = (ExprState *) wstate;
state->evalfunc = (ExprStateEvalFunc) ExecEvalWholeRowVar;
*************** ExecInitExpr(Expr *node, PlanState *pare
*** 4778,4794 ****
/* Build tupdesc to describe result tuples */
if (rowexpr->row_typeid == RECORDOID)
{
! /* generic record, use runtime type assignment */
! rstate->tupdesc = ExecTypeFromExprList(rowexpr->args,
! rowexpr->colnames);
! BlessTupleDesc(rstate->tupdesc);
! /* we won't need to redo this at runtime */
}
else
{
/* it's been cast to a named type, use that */
rstate->tupdesc = lookup_rowtype_tupdesc_copy(rowexpr->row_typeid, -1);
}
/* Set up evaluation, skipping any deleted columns */
Assert(list_length(rowexpr->args) <= rstate->tupdesc->natts);
attrs = rstate->tupdesc->attrs;
--- 4816,4833 ----
/* Build tupdesc to describe result tuples */
if (rowexpr->row_typeid == RECORDOID)
{
! /* generic record, use types of given expressions */
! rstate->tupdesc = ExecTypeFromExprList(rowexpr->args);
}
else
{
/* it's been cast to a named type, use that */
rstate->tupdesc = lookup_rowtype_tupdesc_copy(rowexpr->row_typeid, -1);
}
+ /* In either case, adopt RowExpr's column aliases */
+ ExecTypeSetColNames(rstate->tupdesc, rowexpr->colnames);
+ /* Bless the tupdesc in case it's now of type RECORD */
+ BlessTupleDesc(rstate->tupdesc);
/* Set up evaluation, skipping any deleted columns */
Assert(list_length(rowexpr->args) <= rstate->tupdesc->natts);
attrs = rstate->tupdesc->attrs;
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 7f43441..0811941 100644
*** a/src/backend/executor/execTuples.c
--- b/src/backend/executor/execTuples.c
*************** ExecTypeFromTLInternal(List *targetList,
*** 943,970 ****
/*
* ExecTypeFromExprList - build a tuple descriptor from a list of Exprs
*
! * Caller must also supply a list of field names (String nodes).
*/
TupleDesc
! ExecTypeFromExprList(List *exprList, List *namesList)
{
TupleDesc typeInfo;
! ListCell *le;
! ListCell *ln;
int cur_resno = 1;
- Assert(list_length(exprList) == list_length(namesList));
-
typeInfo = CreateTemplateTupleDesc(list_length(exprList), false);
! forboth(le, exprList, ln, namesList)
{
! Node *e = lfirst(le);
! char *n = strVal(lfirst(ln));
TupleDescInitEntry(typeInfo,
cur_resno,
! n,
exprType(e),
exprTypmod(e),
0);
--- 943,967 ----
/*
* ExecTypeFromExprList - build a tuple descriptor from a list of Exprs
*
! * This is roughly like ExecTypeFromTL, but we work from bare expressions
! * not TargetEntrys. No names are attached to the tupledesc's columns.
*/
TupleDesc
! ExecTypeFromExprList(List *exprList)
{
TupleDesc typeInfo;
! ListCell *lc;
int cur_resno = 1;
typeInfo = CreateTemplateTupleDesc(list_length(exprList), false);
! foreach(lc, exprList)
{
! Node *e = lfirst(lc);
TupleDescInitEntry(typeInfo,
cur_resno,
! NULL,
exprType(e),
exprTypmod(e),
0);
*************** ExecTypeFromExprList(List *exprList, Lis
*** 978,983 ****
--- 975,1028 ----
}
/*
+ * ExecTypeSetColNames - set column names in a TupleDesc
+ *
+ * Column names must be provided as an alias list (list of String nodes).
+ *
+ * For some callers, the supplied tupdesc has a named rowtype (not RECORD)
+ * and it is moderately likely that the alias list matches the column names
+ * already present in the tupdesc. If we do change any column names then
+ * we must reset the tupdesc's type to anonymous RECORD; but we avoid doing
+ * so if no names change.
+ */
+ void
+ ExecTypeSetColNames(TupleDesc typeInfo, List *namesList)
+ {
+ bool modified = false;
+ int colno = 0;
+ ListCell *lc;
+
+ foreach(lc, namesList)
+ {
+ char *cname = strVal(lfirst(lc));
+ Form_pg_attribute attr;
+
+ /* Guard against too-long names list */
+ if (colno >= typeInfo->natts)
+ break;
+ attr = typeInfo->attrs[colno++];
+
+ /* Ignore empty aliases (these must be for dropped columns) */
+ if (cname[0] == '\0')
+ continue;
+
+ /* Change tupdesc only if alias is actually different */
+ if (strcmp(cname, NameStr(attr->attname)) != 0)
+ {
+ namestrcpy(&(attr->attname), cname);
+ modified = true;
+ }
+ }
+
+ /* If we modified the tupdesc, it's now a new record type */
+ if (modified)
+ {
+ typeInfo->tdtypeid = RECORDOID;
+ typeInfo->tdtypmod = -1;
+ }
+ }
+
+ /*
* BlessTupleDesc - make a completed tuple descriptor useful for SRFs
*
* Rowtype Datums returned by a function must contain valid type information.
diff --git a/src/backend/executor/nodeFunctionscan.c b/src/backend/executor/nodeFunctionscan.c
index 945a414..4641708 100644
*** a/src/backend/executor/nodeFunctionscan.c
--- b/src/backend/executor/nodeFunctionscan.c
***************
*** 26,32 ****
#include "executor/nodeFunctionscan.h"
#include "funcapi.h"
#include "nodes/nodeFuncs.h"
- #include "parser/parsetree.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
--- 26,31 ----
*************** FunctionScanState *
*** 279,286 ****
ExecInitFunctionScan(FunctionScan *node, EState *estate, int eflags)
{
FunctionScanState *scanstate;
- RangeTblEntry *rte = rt_fetch(node->scan.scanrelid,
- estate->es_range_table);
int nfuncs = list_length(node->functions);
TupleDesc scan_tupdesc;
int i,
--- 278,283 ----
*************** ExecInitFunctionScan(FunctionScan *node,
*** 494,515 ****
Assert(attno == natts);
}
- /*
- * Make sure the scan result tupdesc has the column names the query
- * expects. This affects the output of constructs like row_to_json which
- * read the column names from the passed-in tupdesc.
- */
- i = 0;
- foreach(lc, rte->eref->colnames)
- {
- char *attname = strVal(lfirst(lc));
-
- if (i >= scan_tupdesc->natts)
- break; /* shouldn't happen, but just in case */
- namestrcpy(&(scan_tupdesc->attrs[i]->attname), attname);
- i++;
- }
-
ExecAssignScanType(&scanstate->ss, scan_tupdesc);
/*
--- 491,496 ----
diff --git a/src/backend/executor/nodeValuesscan.c b/src/backend/executor/nodeValuesscan.c
index 83b1324..d49a473 100644
*** a/src/backend/executor/nodeValuesscan.c
--- b/src/backend/executor/nodeValuesscan.c
***************
*** 25,31 ****
#include "executor/executor.h"
#include "executor/nodeValuesscan.h"
- #include "parser/parsetree.h"
static TupleTableSlot *ValuesNext(ValuesScanState *node);
--- 25,30 ----
*************** ValuesScanState *
*** 189,196 ****
ExecInitValuesScan(ValuesScan *node, EState *estate, int eflags)
{
ValuesScanState *scanstate;
- RangeTblEntry *rte = rt_fetch(node->scan.scanrelid,
- estate->es_range_table);
TupleDesc tupdesc;
ListCell *vtl;
int i;
--- 188,193 ----
*************** ExecInitValuesScan(ValuesScan *node, ESt
*** 242,249 ****
/*
* get info about values list
*/
! tupdesc = ExecTypeFromExprList((List *) linitial(node->values_lists),
! rte->eref->colnames);
ExecAssignScanType(&scanstate->ss, tupdesc);
--- 239,245 ----
/*
* get info about values list
*/
! tupdesc = ExecTypeFromExprList((List *) linitial(node->values_lists));
ExecAssignScanType(&scanstate->ss, tupdesc);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index a44b4cd..f1b65b4 100644
*** a/src/include/executor/executor.h
--- b/src/include/executor/executor.h
*************** extern TupleTableSlot *ExecInitNullTuple
*** 268,274 ****
TupleDesc tupType);
extern TupleDesc ExecTypeFromTL(List *targetList, bool hasoid);
extern TupleDesc ExecCleanTypeFromTL(List *targetList, bool hasoid);
! extern TupleDesc ExecTypeFromExprList(List *exprList, List *namesList);
extern void UpdateChangedParamSet(PlanState *node, Bitmapset *newchg);
typedef struct TupOutputState
--- 268,275 ----
TupleDesc tupType);
extern TupleDesc ExecTypeFromTL(List *targetList, bool hasoid);
extern TupleDesc ExecCleanTypeFromTL(List *targetList, bool hasoid);
! extern TupleDesc ExecTypeFromExprList(List *exprList);
! extern void ExecTypeSetColNames(TupleDesc typeInfo, List *namesList);
extern void UpdateChangedParamSet(PlanState *node, Bitmapset *newchg);
typedef struct TupOutputState
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index b72e605..8c8c01f 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct WholeRowVarExprState
*** 578,583 ****
--- 578,584 ----
{
ExprState xprstate;
struct PlanState *parent; /* parent PlanState, or NULL if none */
+ TupleDesc wrv_tupdesc; /* descriptor for resulting tuples */
JunkFilter *wrv_junkFilter; /* JunkFilter to remove resjunk cols */
} WholeRowVarExprState;
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index 88e7bfa..54525de 100644
*** a/src/test/regress/expected/rowtypes.out
--- b/src/test/regress/expected/rowtypes.out
*************** select (row('Jim', 'Beam')).text; -- er
*** 474,476 ****
--- 474,636 ----
ERROR: could not identify column "text" in record data type
LINE 1: select (row('Jim', 'Beam')).text;
^
+ --
+ -- Test that composite values are seen to have the correct column names
+ -- (bug #11210 and other reports)
+ --
+ select row_to_json(i) from int8_tbl i;
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(i) from int8_tbl i(x,y);
+ row_to_json
+ ----------------------------------------------
+ {"x":123,"y":456}
+ {"x":123,"y":4567890123456789}
+ {"x":4567890123456789,"y":123}
+ {"x":4567890123456789,"y":4567890123456789}
+ {"x":4567890123456789,"y":-4567890123456789}
+ (5 rows)
+
+ create temp view vv1 as select * from int8_tbl;
+ select row_to_json(i) from vv1 i;
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(i) from vv1 i(x,y);
+ row_to_json
+ ----------------------------------------------
+ {"x":123,"y":456}
+ {"x":123,"y":4567890123456789}
+ {"x":4567890123456789,"y":123}
+ {"x":4567890123456789,"y":4567890123456789}
+ {"x":4567890123456789,"y":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1, q2 from int8_tbl) as ss;
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1, q2 from int8_tbl offset 0) as ss;
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl) as ss;
+ row_to_json
+ ----------------------------------------------
+ {"a":123,"b":456}
+ {"a":123,"b":4567890123456789}
+ {"a":4567890123456789,"b":123}
+ {"a":4567890123456789,"b":4567890123456789}
+ {"a":4567890123456789,"b":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl offset 0) as ss;
+ row_to_json
+ ----------------------------------------------
+ {"a":123,"b":456}
+ {"a":123,"b":4567890123456789}
+ {"a":4567890123456789,"b":123}
+ {"a":4567890123456789,"b":4567890123456789}
+ {"a":4567890123456789,"b":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl) as ss(x,y);
+ row_to_json
+ ----------------------------------------------
+ {"x":123,"y":456}
+ {"x":123,"y":4567890123456789}
+ {"x":4567890123456789,"y":123}
+ {"x":4567890123456789,"y":4567890123456789}
+ {"x":4567890123456789,"y":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl offset 0) as ss(x,y);
+ row_to_json
+ ----------------------------------------------
+ {"x":123,"y":456}
+ {"x":123,"y":4567890123456789}
+ {"x":4567890123456789,"y":123}
+ {"x":4567890123456789,"y":4567890123456789}
+ {"x":4567890123456789,"y":-4567890123456789}
+ (5 rows)
+
+ explain (costs off)
+ select row_to_json(q) from
+ (select thousand, tenthous from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ QUERY PLAN
+ -------------------------------------------------------------
+ Subquery Scan on q
+ -> Index Only Scan using tenk1_thous_tenthous on tenk1
+ Index Cond: ((thousand = 42) AND (tenthous < 2000))
+ (3 rows)
+
+ select row_to_json(q) from
+ (select thousand, tenthous from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ row_to_json
+ ---------------------------------
+ {"thousand":42,"tenthous":42}
+ {"thousand":42,"tenthous":1042}
+ (2 rows)
+
+ select row_to_json(q) from
+ (select thousand as x, tenthous as y from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ row_to_json
+ -------------------
+ {"x":42,"y":42}
+ {"x":42,"y":1042}
+ (2 rows)
+
+ select row_to_json(q) from
+ (select thousand as x, tenthous as y from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q(a,b);
+ row_to_json
+ -------------------
+ {"a":42,"b":42}
+ {"a":42,"b":1042}
+ (2 rows)
+
+ create temp table tt1 as select * from int8_tbl limit 2;
+ create temp table tt2 () inherits(tt1);
+ insert into tt2 values(0,0);
+ select row_to_json(r) from (select q2,q1 from tt1 offset 0) r;
+ row_to_json
+ ----------------------------------
+ {"q2":456,"q1":123}
+ {"q2":4567890123456789,"q1":123}
+ {"q2":0,"q1":0}
+ (3 rows)
+
diff --git a/src/test/regress/sql/rowtypes.sql b/src/test/regress/sql/rowtypes.sql
index 65ebdc5..bc3f021 100644
*** a/src/test/regress/sql/rowtypes.sql
--- b/src/test/regress/sql/rowtypes.sql
*************** select cast (row('Jim', 'Beam') as text)
*** 227,229 ****
--- 227,273 ----
select (row('Jim', 'Beam'))::text;
select text(row('Jim', 'Beam')); -- error
select (row('Jim', 'Beam')).text; -- error
+
+ --
+ -- Test that composite values are seen to have the correct column names
+ -- (bug #11210 and other reports)
+ --
+
+ select row_to_json(i) from int8_tbl i;
+ select row_to_json(i) from int8_tbl i(x,y);
+
+ create temp view vv1 as select * from int8_tbl;
+ select row_to_json(i) from vv1 i;
+ select row_to_json(i) from vv1 i(x,y);
+
+ select row_to_json(ss) from
+ (select q1, q2 from int8_tbl) as ss;
+ select row_to_json(ss) from
+ (select q1, q2 from int8_tbl offset 0) as ss;
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl) as ss;
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl offset 0) as ss;
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl) as ss(x,y);
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl offset 0) as ss(x,y);
+
+ explain (costs off)
+ select row_to_json(q) from
+ (select thousand, tenthous from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ select row_to_json(q) from
+ (select thousand, tenthous from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ select row_to_json(q) from
+ (select thousand as x, tenthous as y from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ select row_to_json(q) from
+ (select thousand as x, tenthous as y from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q(a,b);
+
+ create temp table tt1 as select * from int8_tbl limit 2;
+ create temp table tt2 () inherits(tt1);
+ insert into tt2 values(0,0);
+ select row_to_json(r) from (select q2,q1 from tt1 offset 0) r;
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 7cfa63f..8a13ac4 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
***************
*** 50,55 ****
--- 50,56 ----
#include "nodes/nodeFuncs.h"
#include "optimizer/planner.h"
#include "parser/parse_coerce.h"
+ #include "parser/parsetree.h"
#include "pgstat.h"
#include "utils/acl.h"
#include "utils/builtins.h"
*************** ExecEvalWholeRowVar(WholeRowVarExprState
*** 712,717 ****
--- 713,720 ----
{
Var *variable = (Var *) wrvstate->xprstate.expr;
TupleTableSlot *slot;
+ TupleDesc output_tupdesc;
+ MemoryContext oldcontext;
bool needslow = false;
if (isDone)
*************** ExecEvalWholeRowVar(WholeRowVarExprState
*** 787,794 ****
/* 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,
--- 790,795 ----
*************** ExecEvalWholeRowVar(WholeRowVarExprState
*** 860,869 ****
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
--- 861,921 ----
needslow = true; /* need runtime check for null */
}
+ /*
+ * Use the variable's declared rowtype as the descriptor for the
+ * output values. In particular, we *must* absorb any attisdropped
+ * markings.
+ */
+ oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
+ output_tupdesc = CreateTupleDescCopy(var_tupdesc);
+ MemoryContextSwitchTo(oldcontext);
+
ReleaseTupleDesc(var_tupdesc);
}
+ else
+ {
+ /*
+ * In the RECORD case, we use the input slot's rowtype as the
+ * descriptor for the output values, modulo possibly assigning new
+ * column names below.
+ */
+ oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
+ output_tupdesc = CreateTupleDescCopy(slot->tts_tupleDescriptor);
+ MemoryContextSwitchTo(oldcontext);
+ }
! /*
! * Construct a tuple descriptor for the composite values we'll produce,
! * and make sure its record type is "blessed". The main reason to do this
! * is to be sure that operations such as row_to_json() will see the
! * desired column names when they look up the descriptor from the type
! * information embedded in the composite values.
! *
! * We already got the correct physical datatype info above, but now we
! * should try to find the source RTE and adopt its column aliases, in case
! * they are different from the original rowtype's names. But to minimize
! * compatibility breakage, don't do this for Vars of named composite
! * types, only for Vars of type RECORD.
! *
! * If we can't locate the RTE, assume the column names we've got are OK.
! * (As of this writing, the only cases where we can't locate the RTE are
! * in execution of trigger WHEN clauses, and then the Var will have the
! * trigger's relation's rowtype, so its names are fine.)
! */
! if (variable->vartype == RECORDOID &&
! econtext->ecxt_estate &&
! variable->varno <= list_length(econtext->ecxt_estate->es_range_table))
! {
! RangeTblEntry *rte = rt_fetch(variable->varno,
! econtext->ecxt_estate->es_range_table);
!
! ExecTypeSetColNames(output_tupdesc, rte->eref->colnames);
! }
!
! /* Bless the tupdesc if needed, and save it in the execution state */
! wrvstate->wrv_tupdesc = BlessTupleDesc(output_tupdesc);
!
! /* Skip all the above on future executions of node */
if (needslow)
wrvstate->xprstate.evalfunc = (ExprStateEvalFunc) ExecEvalWholeRowSlow;
else
*************** ExecEvalWholeRowFast(WholeRowVarExprStat
*** 886,892 ****
{
Var *variable = (Var *) wrvstate->xprstate.expr;
TupleTableSlot *slot;
- TupleDesc slot_tupdesc;
HeapTupleHeader dtuple;
if (isDone)
--- 938,943 ----
*************** ExecEvalWholeRowFast(WholeRowVarExprStat
*** 917,949 ****
slot = ExecFilterJunk(wrvstate->wrv_junkFilter, slot);
/*
- * 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. (Note: we must
- * do this here, not in ExecEvalWholeRowVar, because some plan trees may
- * return different slots at different times. We have to be ready to
- * bless additional slots during the run.)
- */
- slot_tupdesc = slot->tts_tupleDescriptor;
- if (variable->vartype == RECORDOID &&
- slot_tupdesc->tdtypeid == RECORDOID &&
- slot_tupdesc->tdtypmod < 0)
- assign_record_type_typmod(slot_tupdesc);
-
- /*
* Copy the slot tuple and make sure any toasted fields get detoasted.
*/
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * If the Var identifies a named composite type, label the datum with that
! * type; otherwise we'll use the slot's info.
*/
! if (variable->vartype != RECORDOID)
! {
! HeapTupleHeaderSetTypeId(dtuple, variable->vartype);
! HeapTupleHeaderSetTypMod(dtuple, variable->vartypmod);
! }
return PointerGetDatum(dtuple);
}
--- 968,982 ----
slot = ExecFilterJunk(wrvstate->wrv_junkFilter, slot);
/*
* Copy the slot tuple and make sure any toasted fields get detoasted.
*/
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * Label the datum with the composite type info we identified before.
*/
! HeapTupleHeaderSetTypeId(dtuple, wrvstate->wrv_tupdesc->tdtypeid);
! HeapTupleHeaderSetTypMod(dtuple, wrvstate->wrv_tupdesc->tdtypmod);
return PointerGetDatum(dtuple);
}
*************** ExecEvalWholeRowSlow(WholeRowVarExprStat
*** 997,1004 ****
tuple = ExecFetchSlotTuple(slot);
tupleDesc = slot->tts_tupleDescriptor;
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++)
--- 1030,1038 ----
tuple = ExecFetchSlotTuple(slot);
tupleDesc = slot->tts_tupleDescriptor;
+ /* wrv_tupdesc is a good enough representation of the Var's rowtype */
Assert(variable->vartype != RECORDOID);
! var_tupdesc = wrvstate->wrv_tupdesc;
/* Check to see if any dropped attributes are non-null */
for (i = 0; i < var_tupdesc->natts; i++)
*************** ExecEvalWholeRowSlow(WholeRowVarExprStat
*** 1025,1036 ****
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * Reset datum's type ID fields to match the Var.
*/
! HeapTupleHeaderSetTypeId(dtuple, variable->vartype);
! HeapTupleHeaderSetTypMod(dtuple, variable->vartypmod);
!
! ReleaseTupleDesc(var_tupdesc);
return PointerGetDatum(dtuple);
}
--- 1059,1068 ----
dtuple = DatumGetHeapTupleHeader(ExecFetchSlotTupleDatum(slot));
/*
! * Label the datum with the composite type info we identified before.
*/
! HeapTupleHeaderSetTypeId(dtuple, wrvstate->wrv_tupdesc->tdtypeid);
! HeapTupleHeaderSetTypMod(dtuple, wrvstate->wrv_tupdesc->tdtypmod);
return PointerGetDatum(dtuple);
}
*************** ExecInitExpr(Expr *node, PlanState *pare
*** 4375,4380 ****
--- 4407,4413 ----
WholeRowVarExprState *wstate = makeNode(WholeRowVarExprState);
wstate->parent = parent;
+ wstate->wrv_tupdesc = NULL;
wstate->wrv_junkFilter = NULL;
state = (ExprState *) wstate;
state->evalfunc = (ExprStateEvalFunc) ExecEvalWholeRowVar;
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 66515f7..6a0792b 100644
*** a/src/backend/executor/execTuples.c
--- b/src/backend/executor/execTuples.c
*************** ExecTypeFromExprList(List *exprList, Lis
*** 985,990 ****
--- 985,1033 ----
}
/*
+ * ExecTypeSetColNames - set column names in a TupleDesc
+ *
+ * Column names must be provided as an alias list (list of String nodes).
+ * We set names only in TupleDesc columns that lacked one before.
+ */
+ void
+ ExecTypeSetColNames(TupleDesc typeInfo, List *namesList)
+ {
+ bool modified = false;
+ int colno = 0;
+ ListCell *lc;
+
+ foreach(lc, namesList)
+ {
+ char *cname = strVal(lfirst(lc));
+ Form_pg_attribute attr;
+
+ /* Guard against too-long names list */
+ if (colno >= typeInfo->natts)
+ break;
+ attr = typeInfo->attrs[colno++];
+
+ /* Ignore empty aliases (these must be for dropped columns) */
+ if (cname[0] == '\0')
+ continue;
+
+ /* Change tupdesc only if we didn't have a name before */
+ if (NameStr(attr->attname)[0] == '\0')
+ {
+ namestrcpy(&(attr->attname), cname);
+ modified = true;
+ }
+ }
+
+ /* If we modified the tupdesc, it's now a new record type */
+ if (modified)
+ {
+ typeInfo->tdtypeid = RECORDOID;
+ typeInfo->tdtypmod = -1;
+ }
+ }
+
+ /*
* BlessTupleDesc - make a completed tuple descriptor useful for SRFs
*
* Rowtype Datums returned by a function must contain valid type information.
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 0266135..21e9e3b 100644
*** a/src/include/executor/executor.h
--- b/src/include/executor/executor.h
*************** extern TupleTableSlot *ExecInitNullTuple
*** 266,271 ****
--- 266,272 ----
extern TupleDesc ExecTypeFromTL(List *targetList, bool hasoid);
extern TupleDesc ExecCleanTypeFromTL(List *targetList, bool hasoid);
extern TupleDesc ExecTypeFromExprList(List *exprList, List *namesList);
+ extern void ExecTypeSetColNames(TupleDesc typeInfo, List *namesList);
extern void UpdateChangedParamSet(PlanState *node, Bitmapset *newchg);
typedef struct TupOutputState
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index fae2811..aaf9a55 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct WholeRowVarExprState
*** 578,583 ****
--- 578,584 ----
ExprState xprstate;
struct PlanState *parent; /* parent PlanState, or NULL if none */
JunkFilter *wrv_junkFilter; /* JunkFilter to remove resjunk cols */
+ TupleDesc wrv_tupdesc; /* descriptor for resulting tuples */
} WholeRowVarExprState;
/* ----------------
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index 88e7bfa..d7f1dd5 100644
*** a/src/test/regress/expected/rowtypes.out
--- b/src/test/regress/expected/rowtypes.out
*************** select (row('Jim', 'Beam')).text; -- er
*** 474,476 ****
--- 474,636 ----
ERROR: could not identify column "text" in record data type
LINE 1: select (row('Jim', 'Beam')).text;
^
+ --
+ -- Test that composite values are seen to have the correct column names
+ -- (bug #11210 and other reports)
+ --
+ select row_to_json(i) from int8_tbl i;
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(i) from int8_tbl i(x,y);
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ create temp view vv1 as select * from int8_tbl;
+ select row_to_json(i) from vv1 i;
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(i) from vv1 i(x,y);
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1, q2 from int8_tbl) as ss;
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1, q2 from int8_tbl offset 0) as ss;
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl) as ss;
+ row_to_json
+ ----------------------------------------------
+ {"a":123,"b":456}
+ {"a":123,"b":4567890123456789}
+ {"a":4567890123456789,"b":123}
+ {"a":4567890123456789,"b":4567890123456789}
+ {"a":4567890123456789,"b":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl offset 0) as ss;
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl) as ss(x,y);
+ row_to_json
+ ----------------------------------------------
+ {"x":123,"y":456}
+ {"x":123,"y":4567890123456789}
+ {"x":4567890123456789,"y":123}
+ {"x":4567890123456789,"y":4567890123456789}
+ {"x":4567890123456789,"y":-4567890123456789}
+ (5 rows)
+
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl offset 0) as ss(x,y);
+ row_to_json
+ ------------------------------------------------
+ {"q1":123,"q2":456}
+ {"q1":123,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":123}
+ {"q1":4567890123456789,"q2":4567890123456789}
+ {"q1":4567890123456789,"q2":-4567890123456789}
+ (5 rows)
+
+ explain (costs off)
+ select row_to_json(q) from
+ (select thousand, tenthous from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ QUERY PLAN
+ -------------------------------------------------------------
+ Subquery Scan on q
+ -> Index Only Scan using tenk1_thous_tenthous on tenk1
+ Index Cond: ((thousand = 42) AND (tenthous < 2000))
+ (3 rows)
+
+ select row_to_json(q) from
+ (select thousand, tenthous from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ row_to_json
+ ---------------------------------
+ {"thousand":42,"tenthous":42}
+ {"thousand":42,"tenthous":1042}
+ (2 rows)
+
+ select row_to_json(q) from
+ (select thousand as x, tenthous as y from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ row_to_json
+ -------------------
+ {"x":42,"y":42}
+ {"x":42,"y":1042}
+ (2 rows)
+
+ select row_to_json(q) from
+ (select thousand as x, tenthous as y from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q(a,b);
+ row_to_json
+ -------------------
+ {"a":42,"b":42}
+ {"a":42,"b":1042}
+ (2 rows)
+
+ create temp table tt1 as select * from int8_tbl limit 2;
+ create temp table tt2 () inherits(tt1);
+ insert into tt2 values(0,0);
+ select row_to_json(r) from (select q2,q1 from tt1 offset 0) r;
+ row_to_json
+ ----------------------------------
+ {"q2":456,"q1":123}
+ {"q2":4567890123456789,"q1":123}
+ {"q2":0,"q1":0}
+ (3 rows)
+
diff --git a/src/test/regress/sql/rowtypes.sql b/src/test/regress/sql/rowtypes.sql
index 65ebdc5..bc3f021 100644
*** a/src/test/regress/sql/rowtypes.sql
--- b/src/test/regress/sql/rowtypes.sql
*************** select cast (row('Jim', 'Beam') as text)
*** 227,229 ****
--- 227,273 ----
select (row('Jim', 'Beam'))::text;
select text(row('Jim', 'Beam')); -- error
select (row('Jim', 'Beam')).text; -- error
+
+ --
+ -- Test that composite values are seen to have the correct column names
+ -- (bug #11210 and other reports)
+ --
+
+ select row_to_json(i) from int8_tbl i;
+ select row_to_json(i) from int8_tbl i(x,y);
+
+ create temp view vv1 as select * from int8_tbl;
+ select row_to_json(i) from vv1 i;
+ select row_to_json(i) from vv1 i(x,y);
+
+ select row_to_json(ss) from
+ (select q1, q2 from int8_tbl) as ss;
+ select row_to_json(ss) from
+ (select q1, q2 from int8_tbl offset 0) as ss;
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl) as ss;
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl offset 0) as ss;
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl) as ss(x,y);
+ select row_to_json(ss) from
+ (select q1 as a, q2 as b from int8_tbl offset 0) as ss(x,y);
+
+ explain (costs off)
+ select row_to_json(q) from
+ (select thousand, tenthous from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ select row_to_json(q) from
+ (select thousand, tenthous from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ select row_to_json(q) from
+ (select thousand as x, tenthous as y from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q;
+ select row_to_json(q) from
+ (select thousand as x, tenthous as y from tenk1
+ where thousand = 42 and tenthous < 2000 offset 0) q(a,b);
+
+ create temp table tt1 as select * from int8_tbl limit 2;
+ create temp table tt2 () inherits(tt1);
+ insert into tt2 values(0,0);
+ select row_to_json(r) from (select q2,q1 from tt1 offset 0) r;
I wrote:
> Attached are patches meant for HEAD and 9.2-9.4 respectively.
BTW, has anyone got an opinion about whether to stick the full fix into
9.4? The argument for, of course, is that we'd get the full fix out to
the public a year sooner. The argument against is that someone might
have already done compatibility testing of their application against
9.4 betas, and then get blindsided if we change this before release.
I'm inclined to think that since we expect json/jsonb usage to really
take off with 9.4, it might be better if we get row_to_json behaving
unsurprisingly now. But I'm not dead set on it.
regards, tom lane
On Mon, Nov 10, 2014 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> Attached are patches meant for HEAD and 9.2-9.4 respectively. > > BTW, has anyone got an opinion about whether to stick the full fix into > 9.4? The argument for, of course, is that we'd get the full fix out to > the public a year sooner. The argument against is that someone might > have already done compatibility testing of their application against > 9.4 betas, and then get blindsided if we change this before release. > > I'm inclined to think that since we expect json/jsonb usage to really > take off with 9.4, it might be better if we get row_to_json behaving > unsurprisingly now. But I'm not dead set on it. I think we should put the full fix in 9.4. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Tom Lane-2 wrote > I wrote: >> Attached are patches meant for HEAD and 9.2-9.4 respectively. > > BTW, has anyone got an opinion about whether to stick the full fix into > 9.4? The argument for, of course, is that we'd get the full fix out to > the public a year sooner. The argument against is that someone might > have already done compatibility testing of their application against > 9.4 betas, and then get blindsided if we change this before release. > > I'm inclined to think that since we expect json/jsonb usage to really > take off with 9.4, it might be better if we get row_to_json behaving > unsurprisingly now. But I'm not dead set on it. I am not one of those people who would be blindsided but I'd much rather inconvenience our beta testers in order to get a better product in place for the masses. Beta testers read release notes and should be able to identify and fix any problematic areas of their code while simultaneously being happy for the improvements. David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/row-to-json-bug-with-index-only-scans-empty-keys-tp5826070p5826338.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
Yes, it's late beta, but especially if we're pushing json/jsonb usage as a major feature of this release, I'd say remove surprising cases now. It's late beta, but that's what beta is for, the last chance for bug fixes, before we live w/ it for the support life of the release. The affected class are people with an app that already uses json, so 9.2 or better, have ran acceptance tests against an early beta of 9.4, and have SQL that returns madeup fieldnames instead of the appropriate aliases? Which they were probably annoyed at when they wrote the code that consumes that output in the first place. I think an item in the list of compatability changes/gotchas should catch anyone who is that on the ball as to be testing the betas anyway. Anyone pushing that envelope is going to do the same acceptance testing against the actual release before rolling to production. Ross On Mon, Nov 10, 2014 at 10:11:25AM -0500, Tom Lane wrote: > I wrote: > > Attached are patches meant for HEAD and 9.2-9.4 respectively. > > BTW, has anyone got an opinion about whether to stick the full fix into > 9.4? The argument for, of course, is that we'd get the full fix out to > the public a year sooner. The argument against is that someone might > have already done compatibility testing of their application against > 9.4 betas, and then get blindsided if we change this before release. > > I'm inclined to think that since we expect json/jsonb usage to really > take off with 9.4, it might be better if we get row_to_json behaving > unsurprisingly now. But I'm not dead set on it. > > regards, tom lane > -- Ross Reedstrom, Ph.D. reedstrm@rice.edu Systems Engineer & Admin, Research Scientist phone: 713-348-6166 Connexions http://cnx.org fax: 713-348-3665 Rice University MS-375, Houston, TX 77005 GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE
On 11/10/2014 10:19 AM, Robert Haas wrote: > On Mon, Nov 10, 2014 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I wrote: >>> Attached are patches meant for HEAD and 9.2-9.4 respectively. >> BTW, has anyone got an opinion about whether to stick the full fix into >> 9.4? The argument for, of course, is that we'd get the full fix out to >> the public a year sooner. The argument against is that someone might >> have already done compatibility testing of their application against >> 9.4 betas, and then get blindsided if we change this before release. >> >> I'm inclined to think that since we expect json/jsonb usage to really >> take off with 9.4, it might be better if we get row_to_json behaving >> unsurprisingly now. But I'm not dead set on it. > I think we should put the full fix in 9.4. > +1 cheers andrew
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Nov 10, 2014 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, has anyone got an opinion about whether to stick the full fix into
>> 9.4?
> I think we should put the full fix in 9.4.
Since nobody spoke against that, I've put the full fix into HEAD and 9.4,
and the restricted version into 9.3 and 9.2.
regards, tom lane