Re: [PATCH] Allow anonymous rowtypes in function return column definition

Elvis Pranskevichus <el(at)prans(dot)net> writes:
> Currently, the following query
> SELECT q.b = row(2)
> FROM unnest(ARRAY[row(1, row(2))]) AS q(a int, b record);
> would fail with
> ERROR: column "b" has pseudo-type record
> This is due to CheckAttributeNamesTypes() being used on a function
> coldeflist as if it was a real relation definition. But in the context
> of a query there seems to be no harm in allowing this,

Hmm, I'm not entirely convinced of that. Looking at the conditions
checked by CheckAttributeType, the first question that pops to mind
is whether allowing "record" doesn't break our defenses against
a rowtype recursively containing itself. Perhaps not --- allowing
an anonymous record to contain another one isn't really recursion,
because since they're both anonymous they can't be the "same" type.
But it could do with more thought than I've given it just now,
and with a comment explaining the reasoning.

(Speaking of which, you did not bother updating the comment immediately
adjacent to the code you changed in CheckAttributeType, even though
this change makes it incomplete/not very sensible.)

I also wonder why we'd allow RECORD but not RECORDARRAY. More
generally, why not any polymorphic type? There's probably a
good reason why not, but having a clear explanation why we're
treating RECORD differently from other polymorphics would go
a long way towards convincing me that this is safe. Again
this is just handwaving, but such an argument might rest on the
fact that a record value is self-identifying as long as you know
it's a record, whereas a random Datum is not a self-identifying
member of the type class "anyelement", for instance.

I feel a bit uncomfortable about defining the new flag to
CheckAttributeNamesTypes as "allow_anonymous_records"; that
seems pretty short-sighted and single-purpose, especially in
view of there being no obvious reason why it shouldn't accept
RECORDARRAY too. Maybe with a clearer explanation of the
issues above, we could define that flag in a more on-point way.

BTW, it strikes me that maybe the reason ANYARRAY isn't insane
to allow in pg_statistic has to do with arrays also being
self-identifying members of that type class, and so possibly
we could get to a place where we're unifying that hack with
this feature addition. I don't insist on doing that as part of
this patch, but as long as we're trying to think through these
issues, it's something to think about.

regards, tom lane