Horton

Thread

Commits

  1. Remove obsolete prohibition on function name matching a column name.

  2. Consider syntactic form when disambiguating function vs column reference.

  1. row_to_json(), NULL values, and AS

    Neil Conway <neil.conway@gmail.com> — 2018-06-14T22:57:27Z

    Hi,

The following behavior does not seem self-consistent to me:

postgres=# select json_agg(row_to_json(x)) from (select *, null from
generate_series(1, 3)) x;
                                                       json_agg
-----------------------------------------------------------------------------------------------------------------------
 [{"generate_series":1,"?column?":null},
{"generate_series":2,"?column?":null},
{"generate_series":3,"?column?":null}]
(1 row)

postgres=# select json_agg(row_to_json(x)) from (select *,
row_to_json(null) as jjj from generate_series(1, 3)) x;
                                                json_agg
--------------------------------------------------------------------------------------------------------
 [{"generate_series":1,"jjj":null}, {"generate_series":2,"jjj":null},
{"generate_series":3,"jjj":null}]
(1 row)

postgres=# select json_agg(row_to_json(x)) from (select *,
row_to_json(null) from generate_series(1, 3)) x;
      json_agg
--------------------
 [null, null, null]
(1 row)

In particular, it is unclear to me why removing the targetlist alias
in the subquery in the third example should change the result set of
the parent query.

Thanks,
Neil

  2. Re: row_to_json(), NULL values, and AS

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-06-14T23:28:24Z

    [ Hi Neil, long time no see ]

Neil Conway <neil.conway@gmail.com> writes:
> The following behavior does not seem self-consistent to me:

Likewise.

> In particular, it is unclear to me why removing the targetlist alias
> in the subquery in the third example should change the result set of
> the parent query.

Looking at "explain verbose" output, it seems like it's not row_to_json's
fault; rather, we seem to be mishandling expansion of the whole-row Var:

regression=# explain verbose select json_agg(row_to_json(x)) from (select *,
row_to_json(null) as jjj from generate_series(1, 3)) x;
                                       QUERY PLAN                                        
-----------------------------------------------------------------------------------------
 Aggregate  (cost=15.00..15.01 rows=1 width=32)
   Output: json_agg(row_to_json(ROW(generate_series.generate_series, NULL::json)))
   ->  Function Scan on pg_catalog.generate_series  (cost=0.00..10.00 rows=1000 width=4)
         Output: generate_series.generate_series
         Function Call: generate_series(1, 3)
(5 rows)

That's fine, but:

regression=# explain verbose select json_agg(row_to_json(x)) from (select *,
row_to_json(null) from generate_series(1, 3)) x;
                                       QUERY PLAN                                        
-----------------------------------------------------------------------------------------
 Aggregate  (cost=12.50..12.51 rows=1 width=32)
   Output: json_agg(NULL::json)
   ->  Function Scan on pg_catalog.generate_series  (cost=0.00..10.00 rows=1000 width=0)
         Output: generate_series.generate_series
         Function Call: generate_series(1, 3)
(5 rows)

That looks like it might be a bug in what we do with whole-row Vars
during subquery flattening.  But if you put an "offset 0" into the
subquery to prevent flattening, you get different but just as weird
misbehavior:

regression=# explain verbose select json_agg(row_to_json(x)) from (select *,
row_to_json(null) as jjj from generate_series(1, 3) offset 0) x;
                                           QUERY PLAN                                           
------------------------------------------------------------------------------------------------
 Aggregate  (cost=25.00..25.02 rows=1 width=32)
   Output: json_agg(row_to_json(x.*))
   ->  Subquery Scan on x  (cost=0.00..20.00 rows=1000 width=28)
         Output: x.*
         ->  Function Scan on pg_catalog.generate_series  (cost=0.00..10.00 rows=1000 width=36)
               Output: generate_series.generate_series, NULL::json
               Function Call: generate_series(1, 3)
(7 rows)

regression=# explain verbose select json_agg(row_to_json(x)) from (select *,
row_to_json(null) from generate_series(1, 3) offset 0) x;
                                        QUERY PLAN                                        
------------------------------------------------------------------------------------------
 Aggregate  (cost=22.50..22.52 rows=1 width=32)
   Output: json_agg((NULL::json))
   ->  Function Scan on pg_catalog.generate_series  (cost=0.00..10.00 rows=1000 width=36)
         Output: NULL::integer, NULL::json
         Function Call: generate_series(1, 3)
(5 rows)

I'm not sure if this is just another artifact of the same problem.
The extra parens in the json_agg() argument are suspicious to put
it mildly, but I've not dug into it to see what the plan tree
really looks like.

			regards, tom lane

  3. Re: row_to_json(), NULL values, and AS

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-06-15T14:53:13Z

    I wrote:
> Neil Conway <neil.conway@gmail.com> writes:
>> The following behavior does not seem self-consistent to me:

> Likewise.

Oh!  What is actually happening is

(1) With no explicit alias, the column name of the sub-select's second
output column is chosen to be "row_to_json".

(2) That makes the outer query's notation row_to_json(x) ambiguous: it
could be a function-syntax reference to the column x.row_to_json.

(3) As it happens, the column interpretation is chosen when it's
ambiguous (cf ParseComplexProjection).

I'm a bit hesitant to muck with this behavior, given that it's stood
for ~20 years.  However, if we did want to touch it, maybe the right
thing would be to give up the absolutist position that f(x) and x.f
are exactly interchangeable.  We could say instead that we prefer the
function interpretation if function syntax is used, and the column
interpretation if column syntax is used.  I don't know how likely
that is to break existing apps ... perhaps not very, but I wouldn't
risk back-patching it in any case.

			regards, tom lane

  4. Re: row_to_json(), NULL values, and AS

    Neil Conway <neil.conway@gmail.com> — 2018-06-15T17:22:30Z

    [ Hi Tom! Hope you're doing well. ]

On Fri, Jun 15, 2018 at 7:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Oh!  What is actually happening is
>
> (1) With no explicit alias, the column name of the sub-select's second
> output column is chosen to be "row_to_json".
>
> (2) That makes the outer query's notation row_to_json(x) ambiguous: it
> could be a function-syntax reference to the column x.row_to_json.
>
> (3) As it happens, the column interpretation is chosen when it's
> ambiguous (cf ParseComplexProjection).
>
> I'm a bit hesitant to muck with this behavior, given that it's stood
> for ~20 years.  However, if we did want to touch it, maybe the right
> thing would be to give up the absolutist position that f(x) and x.f
> are exactly interchangeable.  We could say instead that we prefer the
> function interpretation if function syntax is used, and the column
> interpretation if column syntax is used.

Interesting! Your proposed change seems quite reasonable to me.

Thanks for digging into it.

Neil

  5. Re: row_to_json(), NULL values, and AS

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-06-15T19:34:58Z

    Neil Conway <neil.conway@gmail.com> writes:
> On Fri, Jun 15, 2018 at 7:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> (3) As it happens, the column interpretation is chosen when it's
>> ambiguous (cf ParseComplexProjection).
>> 
>> I'm a bit hesitant to muck with this behavior, given that it's stood
>> for ~20 years.  However, if we did want to touch it, maybe the right
>> thing would be to give up the absolutist position that f(x) and x.f
>> are exactly interchangeable.  We could say instead that we prefer the
>> function interpretation if function syntax is used, and the column
>> interpretation if column syntax is used.

> Interesting! Your proposed change seems quite reasonable to me.

Here's a proposed patch for that.  (It needs to be applied over the
fixes in <14497.1529089235@sss.pgh.pa.us>, which are unrelated but
touch some of the same code.)  I didn't add any test cases yet,
but probably it's desirable to have one.

I wrote the doc change on the assumption that we'd sneak this into
v11, but if anyone doesn't want to do that, I'll add it to the next
commitfest instead.

			regards, tom lane

  6. Re: row_to_json(), NULL values, and AS

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-06-16T14:42:26Z

    I wrote:
> Neil Conway <neil.conway@gmail.com> writes:
>> On Fri, Jun 15, 2018 at 7:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm a bit hesitant to muck with this behavior, given that it's stood
>>> for ~20 years.  However, if we did want to touch it, maybe the right
>>> thing would be to give up the absolutist position that f(x) and x.f
>>> are exactly interchangeable.  We could say instead that we prefer the
>>> function interpretation if function syntax is used, and the column
>>> interpretation if column syntax is used.

>> Interesting! Your proposed change seems quite reasonable to me.

> Here's a proposed patch for that.  (It needs to be applied over the
> fixes in <14497.1529089235@sss.pgh.pa.us>, which are unrelated but
> touch some of the same code.)  I didn't add any test cases yet,
> but probably it's desirable to have one.

It occurred to me this morning that there's actually a dump/reload
hazard in the current behavior.  Consider

regression=# create table t1 (f1 int, f2 text);
CREATE TABLE
regression=# create view v1 as select row_to_json(t1) as j from t1;
CREATE VIEW
regression=# alter table t1 add column row_to_json text;
ALTER TABLE
regression=# \d+ v1
                            View "public.v1"
 Column | Type | Collation | Nullable | Default | Storage  | Description 
--------+------+-----------+----------+---------+----------+-------------
 j      | json |           |          |         | extended | 
View definition:
 SELECT row_to_json(t1.*) AS j
   FROM t1;

At this point, pg_dump will naively dump the view as

CREATE VIEW public.v1 AS
 SELECT row_to_json(t1.*) AS j
   FROM public.t1;

which, with the historical behavior, will be read as a reference to
the subsequently-added column, giving a view definition that means
something else entirely:

regression=# \d+ v1
                            View "public.v1"
 Column | Type | Collation | Nullable | Default | Storage  | Description 
--------+------+-----------+----------+---------+----------+-------------
 j      | text |           |          |         | extended | 
View definition:
 SELECT t1.row_to_json AS j
   FROM t1;

The proposed change fixes this on the parsing side, with no need
for any hacking in ruleutils.c (hence it will fix it for pre-existing
dump files too).

So I'm now pretty well convinced that this is a good change and we
should slip it into v11.  I'm still afraid to back-patch though.
I vaguely recall one or two prior complaints in this area, but not
so many that it's worth taking a chance of breaking applications
in minor releases.

			regards, tom lane

  7. Re: row_to_json(), NULL values, and AS

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-06-17T17:43:26Z

    I wrote:
> So I'm now pretty well convinced that this is a good change and we
> should slip it into v11.

I wrote a test case and was about ready to commit this, when I started
wondering about the behavior for fdresult == FUNCDETAIL_MULTIPLE.
That is, suppose that the notation f(x) matches more than one candidate
function f, and there's also a column named f.  The old code would have
chosen the column interpretation, and so would the patch I posted
previously.  But I don't really see how we can square that behavior with
documenting that "we prefer the function interpretation for functional
notation".  I think you should get a "function is not unique" error in
such a case, not silently picking the column instead.  If it's the
column you want, write it like a column.

So the attached updated patch rearranges the error handling logic to
make that happen.

BTW, while trying to make a test case I was reminded that CREATE FUNCTION
tries to prevent this situation from arising:

regression=# create table t1 (f1 int);
CREATE TABLE
regression=# create function f1(t1) returns int as 'select 42' language sql;
ERROR:  "f1" is already an attribute of type t1

That's totally non-bulletproof, though, since you can still create the
situation by renaming, or by adding the conflicting column after you
create the function; not to mention Neil's original problem where the
conflicting function is declared to take record or some such.
Furthermore, this behavior itself creates a dump/reload hazard, since
once you've done one of those things pg_dump will produce output that
fails this check.

While I've not done anything about it here, I think we should just
remove that error check.  With this patch, functions and columns can
coexist reasonably peacefully, so we shouldn't need bogus hacks that
try to prevent it.

			regards, tom lane

  8. Re: row_to_json(), NULL values, and AS

    Robert Haas <robertmhaas@gmail.com> — 2018-06-19T14:34:49Z

    On Fri, Jun 15, 2018 at 10:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm a bit hesitant to muck with this behavior, given that it's stood
> for ~20 years.  However, if we did want to touch it, maybe the right
> thing would be to give up the absolutist position that f(x) and x.f
> are exactly interchangeable.  We could say instead that we prefer the
> function interpretation if function syntax is used, and the column
> interpretation if column syntax is used.  I don't know how likely
> that is to break existing apps ... perhaps not very, but I wouldn't
> risk back-patching it in any case.

For the record, I fear that this change (committed as
b97a3465d73bfc2a9f5bcf5def1983dbaa0a26f8) is going to break more
things than have been foreseen in this thread.  I do not have a
specific theory about how that's going to happen, just vague unease.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: row_to_json(), NULL values, and AS

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-06-19T15:11:30Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jun 15, 2018 at 10:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm a bit hesitant to muck with this behavior, given that it's stood
>> for ~20 years.  However, if we did want to touch it, maybe the right
>> thing would be to give up the absolutist position that f(x) and x.f
>> are exactly interchangeable.  We could say instead that we prefer the
>> function interpretation if function syntax is used, and the column
>> interpretation if column syntax is used.  I don't know how likely
>> that is to break existing apps ... perhaps not very, but I wouldn't
>> risk back-patching it in any case.

> For the record, I fear that this change (committed as
> b97a3465d73bfc2a9f5bcf5def1983dbaa0a26f8) is going to break more
> things than have been foreseen in this thread.  I do not have a
> specific theory about how that's going to happen, just vague unease.

Yeah, that's why I wouldn't back-patch it.  But certainly the behavior
Neil complained of is pretty bad as well, and it's only going to get
worse as we invent more functions taking record.  I think the new
behavior is a clear improvement.  If it breaks any apps that were
relying on interpreting f(x) as a column, they can fix it by writing
x.f instead.  We inflict worse upgrade pain than that on a regular
basis.

			regards, tom lane