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
