Re: row_to_json bug with index only scans: empty keys!

Andrew Dunstan <andrew(at)dunslane(dot)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