Re: Review: UNNEST (and other functions) WITH ORDINALITY

From: Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Greg Stark <stark(at)mit(dot)edu>, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, David Fetter <david(at)fetter(dot)org>
Subject: Re: Review: UNNEST (and other functions) WITH ORDINALITY
Date: 2013-07-24 21:38:15
Message-ID: ed4cd9f69c022963eecaa20e81bc266e@news-out.riddles.org.uk
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Tom Lane said:
> If we did it with a WithOrdinality expression node, the result would
> always be of type RECORD, and we'd have to use blessed tuple
> descriptors to keep track of exactly which record type a particular
> instance emits. This is certainly do-able (see RowExpr for
> precedent).

Maybe RowExpr is a precedent for something, but it has this
long-standing problem that makes it very hard to use usefully:

postgres=# select (r).* from (select row(a,b) as r from (values (1,2)) v(a,b)) s;
ERROR: record type has not been registered

> It seems way too short on comments. [...]

This can certainly be addressed.

> but it sure looks like it flat out removed several existing
> regression-test cases

Here's why, in rangefuncs.sql:

--invokes ExecReScanFunctionScan
SELECT * FROM foorescan f WHERE f.fooid IN (SELECT fooid FROM foorescan(5002,5004)) ORDER BY 1,2;

I don't think that has invoked ExecReScanFunctionScan since 7.4 or so.
It certainly does not do so now (confirmed by gdb as well as by the
query plan). By all means keep the old tests if you want a
never-remove-tests-for-any-reason policy, but having added tests that
actually _do_ invoke ExecReScanFunctionScan, I figured the old ones
were redundant. (Also, these kinds of tests can be done a bit better
now with values and lateral rather than creating and dropping tables
just for the one test.)

> and a few existing comments as well.

I've double-checked, and I don't see any existing comments removed.

> FWIW, I concur with the gripe I remember seeing upthread that the
> default name of the added column ought not be "?column?".

This seems to be a common complaint, but gives rise to two questions:

1) what should the name be?

2) should users be depending on it?

I've yet to find another db that actually documents a specific column
name for the ordinality column; it's always taken for granted that the
user should always be supplying an alias. (Admittedly there are not
many dbs that support it at all; DB2 does, and I believe Teradata.)

--
Andrew (irc:RhodiumToad)

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2013-07-24 21:50:52 Re: ilist.h is not useful as-is
Previous Message Andres Freund 2013-07-24 21:34:06 Re: dynamic background workers, round two