Horton

Thread

Commits

  1. Allow RECORD and RECORD[] to be specified in function coldeflists.

  1. [PATCH] Allow anonymous rowtypes in function return column definition

    Elvis Pranskevichus <el@prans.net> — 2018-12-06T22:27:32Z

    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, as other ways of
manipulating anonymous rowtypes work well, e.g.:

    SELECT (ARRAY[ROW(1, ROW(2))])[1];



                                Elvis

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-13T21:57:48Z

    Elvis Pranskevichus <el@prans.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

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

    Elvis Pranskevichus <el@prans.net> — 2019-01-28T17:52:57Z

    On Sunday, January 13, 2019 4:57:48 PM EST Tom Lane wrote:

> I also wonder why we'd allow RECORD but not RECORDARRAY.  

That's by omission.  There's no reason to refuse RECORDARRAY here for 
the same reason why RECORD is accepted.

> More generally, why not any polymorphic type? [...] 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.

Yes that's the reason.  We allow "record" in coldeflist because it 
only happens near a RangeFunction, and set-returning functions always do 
"BlessTupleDesc", which means that RECORDOID data would always have an 
associated TupleDesc and can be processed as a regular composite type.  
Recursion is not an issue, since every instance would have a separate 
TupleDesc even if the shape is the same.

> 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.

I renamed it to "in_coldeflist", which seems like a clearer indication 
that we are validating that, and not a regular table definition.

> 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

That's true.  Array data encode the OID of their element type, but that 
only allows sending the data out, as subscripting or casting anyarray is 
not allowed.  There also seems to be no guarantee that the actual type 
of the array doesn't change from row to row in such a scenario.  Given 
that, I think it would be best to keep anyarray restricted to the system 
catalogs.

I attached the updated patch.
                             
                               Elvis

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-30T22:59:41Z

    Elvis Pranskevichus <el@prans.net> writes:
> On Sunday, January 13, 2019 4:57:48 PM EST Tom Lane wrote:
>> 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.

> I renamed it to "in_coldeflist", which seems like a clearer indication 
> that we are validating that, and not a regular table definition.

I still found this pretty disjointed.  After a bit of thought I propose
the attached reformulation, which has the callers just tell the routines
which types to allow explicitly, with the justification comments living
at the call sites instead of within the routines.

One point that we hadn't really talked about is what happens when
CheckArrayTypes recurses.  The existing logic is that it just passes
down the same restrictions it was told at the top level, and I kept
that here.  Right now it hardly matters what we pass down, because
the base type of a domain or array can't be a pseudotype, and we
don't really expect to find one in a named composite type either.
The one case where it could matter is if someone tries to use
"pg_statistic" as a field's composite type: that would be allowed if
allow_system_table_mods and otherwise not.  But it's not really
hard to imagine future additions where it would matter and it'd
be important to restrict things as we recurse down.  I think this
formulation makes it easier to see what to do in such cases.

			regards, tom lane

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

    Elvis Pranskevichus <el@prans.net> — 2019-01-30T23:35:25Z

    On Wednesday, January 30, 2019 5:59:41 PM EST Tom Lane wrote:
> I still found this pretty disjointed.  After a bit of thought I
> propose the attached reformulation, which has the callers just tell
> the routines which types to allow explicitly, with the justification
> comments living at the call sites instead of within the routines.

This is a much better formulation, thank you!

                                  Elvis

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-31T00:26:27Z

    Elvis Pranskevichus <el@prans.net> writes:
> On Wednesday, January 30, 2019 5:59:41 PM EST Tom Lane wrote:
>> I still found this pretty disjointed.  After a bit of thought I
>> propose the attached reformulation, which has the callers just tell
>> the routines which types to allow explicitly, with the justification
>> comments living at the call sites instead of within the routines.

> This is a much better formulation, thank you!

OK, pushed.

			regards, tom lane