Re: [HACKERS] Re: Query fails when SRFs are part of FROM clause(Commit id: 69f4b9c85f) - Mailing list pgsql-hackers

From Andres Freund
Subject Re: [HACKERS] Re: Query fails when SRFs are part of FROM clause(Commit id: 69f4b9c85f)
Date
Msg-id 20170418044132.7eiwo4btnowsdpqv@alap3.anarazel.de
Whole thread Raw
In response to Re: [HACKERS] Re: Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: [HACKERS] Re: Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)  (Tom Lane <tgl@sss.pgh.pa.us>)
List pgsql-hackers
On 2017-04-17 19:26:11 -0400, Tom Lane wrote:
> I wrote:
> > I'm a bit inclined to agree with the idea of explicitly requiring SRFs
> > in FROM to appear only at the top level of the expression.
> 
> If we are going to go down this road, I think it would be a good idea
> to try to provide a cursor position for the "can't accept a set" error
> message, because otherwise it will be really unclear what's wrong with
> something like

That sounds like a good plan.


> Now that this infrastructure exists, anything that has access to a
> PlanState or ExprContext, plus a parse node containing a location, is able
> to report an error cursor.

I've wished for that ability for a while...


> It took a considerable effort of will not to
> start plastering error position reports on a lot of other executor errors.
> I think we should do that, but it smells new-feature-y, and hence
> something to tackle for v11 not now.  But if v10 is moving the goalposts
> on where you can put an SRF call, I think we should do this much in v10.

FWIW, I'd be ok with relaxing things a bit around this, but let's get
the minimal thing in first.


> Naming note: a name like ExecErrPosition() would have been more consistent
> with the other stuff that execUtils.c exports.  But since this is meant
> to be used in ereport() calls, whose support functions generally aren't
> camel-cased, I thought executor_errposition() was a better choice.
> I'm not particularly set on that though.

Seems slightly better this way.


Looks good to me.


I'm working on a patch that adds a few more tests around the current
behaviour and that updates the code to match what we now know.

I couldn't find any place in the docs that actually documents our SRF
behaviour in any sort of detail ([1], [2]), particularly not documenting
where SRFs are legal, and how nested SRFs are supposed to behave.  I'm
not sure in how much detail we want to go?  If we want do document that,
where?  The closest seems to be "4.2.14. Expression Evaluation Rules"
[3].

Greetings,

Andres Freund

[1] https://www.postgresql.org/docs/devel/static/sql-select.html#sql-from
[2] https://www.postgresql.org/docs/devel/static/queries-table-expressions.html#queries-from
[3] https://www.postgresql.org/docs/devel/static/sql-expressions.html#syntax-express-eval



pgsql-hackers by date:

Previous
From: Amit Langote
Date:
Subject: Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table
Next
From: Noah Misch
Date:
Subject: Re: [HACKERS] scram and \password