Horton

Thread

Commits

  1. Fix no-longer-valid shortcuts in expression_returns_set().

  2. Disallow set-returning functions inside CASE or COALESCE.

  1. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Regina Obe <lr@pcorp.us> — 2017-05-26T05:13:43Z

    > Did something change with how exclusion constraints are handled?  I'm
trying to troubleshoot a regression we are having with PostGIS raster
support.

> As best I can guess, it's because exclusion constraints that used to work
in past versions are failing in PostgreSQL 10 with an error something like
> this:

> ERROR:  conflicting key value violates exclusion constraint
"enforce_spatially_unique_test_raster_columns_rast"
> ERROR:  new row for relation "test_raster_columns" violates check
constraint "enforce_coverage_tile_rast"

> Unfortunately I don't know how long this has been an issue since we had an
earlier test failing preventing the raster ones  from being tested.

> Thanks,
> Regina


I figured out the culprit was the change in CASE WHEN behavior with set
returning functions

Had a criteria something of the form:

CASE WHEN some_condition_dependent_on_sometable_that_resolves_to_false THEN
(regexp_matches(...))[1] ELSE ...  END
FROM sometable;


One thing that seems a little odd to me is why these return a record


SELECT CASE WHEN strpos('ABC', 'd') > 1 THEN (regexp_matches('a (b) c',
'd'))[1] ELSE 'a' END;

SELECT CASE WHEN false THEN (regexp_matches('a (b) c', 'd'))[1] ELSE 'a' END
FROM pg_tables;



And this doesn't - I'm guessing it has to do with this being a function of
the value of table, but it seems unintuitive 
>From a user perspective.

SELECT CASE WHEN  strpos(f.tablename, 'ANY (ARRAY[') > 1 THEN
(regexp_matches('a (b) c', 'd'))[1] ELSE 'a' END
FROM pg_tables AS f;


Pre-PostgreSQL 10 this would return a row for each record in pg_tables



Thanks,
Regina

  2. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-26T14:38:54Z

    "Regina Obe" <lr@pcorp.us> writes:
> I figured out the culprit was the change in CASE WHEN behavior with set
> returning functions
> Had a criteria something of the form:
> CASE WHEN some_condition_dependent_on_sometable_that_resolves_to_false THEN
> (regexp_matches(...))[1] ELSE ...  END
> FROM sometable;

You might want to consider changing such usages to use regexp_match()
instead of regexp_matches().

			regards, tom lane

  3. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Regina Obe <lr@pcorp.us> — 2017-05-26T19:14:37Z

    > "Regina Obe" <lr@pcorp.us> writes:
>> I figured out the culprit was the change in CASE WHEN behavior with 
>> set returning functions Had a criteria something of the form:
>> CASE WHEN some_condition_dependent_on_sometable_that_resolves_to_false 
>>  THEN (regexp_matches(...))[1] ELSE ...  END FROM sometable;

> You might want to consider changing such usages to use regexp_match()
instead of regexp_matches().

>			regards, tom lane

Thanks.  I ended up swapping out with substring which was a bit shorter than
regexp_match()[].

But I've got similar problems with PostGIS topology logic and the easiest
change to make was take advantage
of the fact that you guys are treating CASE constant ... THEN  SRF ...

Differently 

Than 

CASE not_constant_based_on_table_value THEN  SRF ..


So I switched those to constant checks.  This feels a little dirty and
fragile to me though.

Is this behavior going to stay or change?

It seems inconsistent from a user perspective that

CASE constant .... == short-circuit skipping over SRFs that may  otherwise
fail

While 

CASE not_constant_table_dependent  doesn't short-circuit.

I can understand the motive behind it, it just feels a little inconsistent
from an end-user POV.


Thanks,
Regina

  4. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-28T18:03:26Z

    "Regina Obe" <lr@pcorp.us> writes:
> Is this behavior going to stay or change?
> It seems inconsistent from a user perspective that
> CASE constant .... == short-circuit skipping over SRFs that may  otherwise
> fail
> While 
> CASE not_constant_table_dependent  doesn't short-circuit.
> I can understand the motive behind it, it just feels a little inconsistent
> from an end-user POV.

After thinking about this for awhile, I agree that what we've got now
isn't too satisfactory.  We can make an analogy between SRFs and
aggregate functions: both of them look like simple function calls
syntactically, but they have global effects on the semantics of the
query, particularly on how many rows are returned.

In the case of aggregates, there is long-standing precedent that we
can optimize away individual aggregate calls but the query semantics
do not change, ie you get one output row (or one per GROUP BY group)
even if every last aggregate call disappears due to CASE simplification.
The same was true for deletion of SRFs by CASE-simplification before
v10, but now we've broken that, which seems like a clear bug.

I think it would be possible to teach eval_const_expressions that
it must not discard CASE/COALESCE subexpressions that contain SRFs,
which would preserve the rule that expression simplification doesn't
change the query semantics.

Another possibility is to say that we've broken this situation
irretrievably and we should start throwing errors for SRFs in
places where they'd be conditionally evaluated.  That's not real
nice perhaps, but it's better than the way things are right now.

Thoughts?

			regards, tom lane

  5. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Andres Freund <andres@anarazel.de> — 2017-06-03T02:45:03Z

    Hi,

On 2017-05-28 14:03:26 -0400, Tom Lane wrote:
> I think it would be possible to teach eval_const_expressions that
> it must not discard CASE/COALESCE subexpressions that contain SRFs,
> which would preserve the rule that expression simplification doesn't
> change the query semantics.

That sounds like a good idea.  Do you want to write up a patch, or
should I?  I can, but I'd want to finish the walsender panic and other
signal handling stuff first (mostly waiting for review for now).


> Another possibility is to say that we've broken this situation
> irretrievably and we should start throwing errors for SRFs in
> places where they'd be conditionally evaluated.  That's not real
> nice perhaps, but it's better than the way things are right now.

I'd be ok with that too, but I don't really see a strong need so far.

Greetings,

Andres Freund

  6. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-03T02:53:00Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-05-28 14:03:26 -0400, Tom Lane wrote:
>> I think it would be possible to teach eval_const_expressions that
>> it must not discard CASE/COALESCE subexpressions that contain SRFs,
>> which would preserve the rule that expression simplification doesn't
>> change the query semantics.

> That sounds like a good idea.  Do you want to write up a patch, or
> should I?  I can, but I'd want to finish the walsender panic and other
> signal handling stuff first (mostly waiting for review for now).

I think you've got enough on your plate.  I can take care of whatever
we decide to do here.  What I was looking for was opinions on which
way to address it.

>> Another possibility is to say that we've broken this situation
>> irretrievably and we should start throwing errors for SRFs in
>> places where they'd be conditionally evaluated.  That's not real
>> nice perhaps, but it's better than the way things are right now.

> I'd be ok with that too, but I don't really see a strong need so far.

The argument for this way is basically that it's better to break
apps visibly than silently.  The behavior for SRF-inside-CASE is
not going to be the same as before even if we implement the fix
I suggest above, and it's arguable that this new behavior is not
at all intuitive.

I'm not really sure which way to jump, which is why I was hoping
for some discussion here.

			regards, tom lane

  7. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Andres Freund <andres@anarazel.de> — 2017-06-03T03:11:27Z

    Hi,


On 2017-06-02 22:53:00 -0400, Tom Lane wrote:
> I think you've got enough on your plate.  I can take care of whatever
> we decide to do here.

Thanks.


> Andres Freund <andres@anarazel.de> writes:
> >> Another possibility is to say that we've broken this situation
> >> irretrievably and we should start throwing errors for SRFs in
> >> places where they'd be conditionally evaluated.  That's not real
> >> nice perhaps, but it's better than the way things are right now.
> 
> > I'd be ok with that too, but I don't really see a strong need so far.
> 
> The argument for this way is basically that it's better to break
> apps visibly than silently.

Right, I got that.


> The behavior for SRF-inside-CASE is
> not going to be the same as before even if we implement the fix
> I suggest above, and it's arguable that this new behavior is not
> at all intuitive.

Yea, I'm not a big fan of the either the pre v10 or the v10 behaviour of
SRFs inside coalesce/case.  Neither is really resonable imo - I'm not
sure a reasonable behaviour even exists.  IIRC I'd argued in the
original SRF thread that we should just throw an error, and I think we'd
concluded that we'd not do so for now.


> I'm not really sure which way to jump, which is why I was hoping
> for some discussion here.

There not really being an intuitive behaviour seems to be a bit of a
reason to disallow. Another argument that I can see is that it'll be
easier to allow it again later, than to do the reverse.  But I think the
new behaviour can also be useful, and I suspect not that many people
will hit this...

- Andres

  8. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Mark Dilger <hornschnorter@gmail.com> — 2017-06-04T18:55:03Z

    > On Jun 2, 2017, at 8:11 PM, Andres Freund <andres@anarazel.de> wrote:
> 
> Hi,
> 
> 
> On 2017-06-02 22:53:00 -0400, Tom Lane wrote:
>> I think you've got enough on your plate.  I can take care of whatever
>> we decide to do here.
> 
> Thanks.
> 
> 
>> Andres Freund <andres@anarazel.de> writes:
>>>> Another possibility is to say that we've broken this situation
>>>> irretrievably and we should start throwing errors for SRFs in
>>>> places where they'd be conditionally evaluated.  That's not real
>>>> nice perhaps, but it's better than the way things are right now.
>> 
>>> I'd be ok with that too, but I don't really see a strong need so far.
>> 
>> The argument for this way is basically that it's better to break
>> apps visibly than silently.
> 
> Right, I got that.
> 
> 
>> The behavior for SRF-inside-CASE is
>> not going to be the same as before even if we implement the fix
>> I suggest above, and it's arguable that this new behavior is not
>> at all intuitive.
> 
> Yea, I'm not a big fan of the either the pre v10 or the v10 behaviour of
> SRFs inside coalesce/case.  Neither is really resonable imo - I'm not
> sure a reasonable behaviour even exists.  IIRC I'd argued in the
> original SRF thread that we should just throw an error, and I think we'd
> concluded that we'd not do so for now.

I am trying to get my head around the type of query you and Tom
are discussing.  When you say you are unsure a reasonable behavior
even exists, are you saying such queries have no intuitive meaning?

Can you give an example of such a query which has no intuitive
meaning?  Perhaps I am not thinking about the right kind of queries.
I have been thinking about examples like:

SELECT x, CASE WHEN y THEN generate_series(1,z) ELSE 5 END
	FROM table_with_columns_x_and_y_and_z;

Which to me gives 'z' output rows per table row where y is true, and
one output row per table row where y is false.  That could be changed
with an aggregate function such as:

SELECT x, CASE WHEN y THEN SUM(generate_series(1,z)) ELSE 5 END
	FROM table_with_columns_x_and_y;

Which to me gives one output row per table row regardless of whether y
is true.


Thanks, and my apologies if I am merely lacking sufficient imagination to
think of a proper example.

Mark Dilger

  9. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Mark Dilger <hornschnorter@gmail.com> — 2017-06-04T19:19:06Z

    > On Jun 4, 2017, at 11:55 AM, Mark Dilger <hornschnorter@gmail.com> wrote:
> 
> SELECT x, CASE WHEN y THEN SUM(generate_series(1,z)) ELSE 5 END
> 	FROM table_with_columns_x_and_y;

Sorry, this table is supposed to be the same as the previous one,

table_with_columns_x_and_y_and_z

  10. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Andres Freund <andres@anarazel.de> — 2017-06-04T19:35:21Z

    Hi Mark,

On 2017-06-04 11:55:03 -0700, Mark Dilger wrote:
> > Yea, I'm not a big fan of the either the pre v10 or the v10 behaviour of
> > SRFs inside coalesce/case.  Neither is really resonable imo - I'm not
> > sure a reasonable behaviour even exists.  IIRC I'd argued in the
> > original SRF thread that we should just throw an error, and I think we'd
> > concluded that we'd not do so for now.
> 
> I am trying to get my head around the type of query you and Tom
> are discussing.  When you say you are unsure a reasonable behavior
> even exists, are you saying such queries have no intuitive meaning?

I'm not saying that there aren't some cases where it's intuitive, but
there's definitely lots that don't have intuitive meaning.  Especially
when there are multiple SRFs in the same targetlist.

Do I understand correctly that you're essentially advocating for the <
v10 behaviour?  It'd be nontrivial to implement that, without loosing
the significant benefits (Significantly slower, higher code complexity,
weird behaviour around multiple SRFs) gained by removing the previous
implementation.   I'd really like to see some examples of when all of
this is useful - I've yet to see a realistic one that's not just as
easily written differently


> Can you give an example of such a query which has no intuitive
> meaning?  Perhaps I am not thinking about the right kind of queries.
> I have been thinking about examples like:
> 
> SELECT x, CASE WHEN y THEN generate_series(1,z) ELSE 5 END
> 	FROM table_with_columns_x_and_y_and_z;
> 
> Which to me gives 'z' output rows per table row where y is true, and
> one output row per table row where y is false.

Try any query that has one SRF outside of the CASE, and one inside.  In
the old behaviour that'll make the total number of rows returned nearly
undeterministic because of the least-common-multiple behaviour.


> That could be changed with an aggregate function such as:
> SELECT x, CASE WHEN y THEN SUM(generate_series(1,z)) ELSE 5 END
> 	FROM table_with_columns_x_and_y;

That query doesn't work.  First off, aggregates don't take set arguments
(neither in old nor new releases), secondly aggregates are evaluated
independently of CASE/COALESCE statements, thirdly you're missing group
bys.  Those all are independent of the v10 changes.


> Thanks, and my apologies if I am merely lacking sufficient imagination to
> think of a proper example.

Might be worthwhile to reread the thread about the SRF reimplementation.

https://www.postgresql.org/message-id/20160822214023.aaxz5l4igypowyri@alap3.anarazel.de

- Andres

  11. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Mark Dilger <hornschnorter@gmail.com> — 2017-06-04T21:16:14Z

    > On Jun 4, 2017, at 12:35 PM, Andres Freund <andres@anarazel.de> wrote:
> 
> Hi Mark,
> 
> On 2017-06-04 11:55:03 -0700, Mark Dilger wrote:
>>> Yea, I'm not a big fan of the either the pre v10 or the v10 behaviour of
>>> SRFs inside coalesce/case.  Neither is really resonable imo - I'm not
>>> sure a reasonable behaviour even exists.  IIRC I'd argued in the
>>> original SRF thread that we should just throw an error, and I think we'd
>>> concluded that we'd not do so for now.
>> 
>> I am trying to get my head around the type of query you and Tom
>> are discussing.  When you say you are unsure a reasonable behavior
>> even exists, are you saying such queries have no intuitive meaning?
> 
> I'm not saying that there aren't some cases where it's intuitive, but
> there's definitely lots that don't have intuitive meaning.  Especially
> when there are multiple SRFs in the same targetlist.
> 
> Do I understand correctly that you're essentially advocating for the <
> v10 behaviour?

No, I'm not advocating either way.  I merely wanted to know which queries
you and Tom were talking about.

>  It'd be nontrivial to implement that, without loosing
> the significant benefits (Significantly slower, higher code complexity,
> weird behaviour around multiple SRFs) gained by removing the previous
> implementation.   I'd really like to see some examples of when all of
> this is useful - I've yet to see a realistic one that's not just as
> easily written differently
> 
> 
>> Can you give an example of such a query which has no intuitive
>> meaning?  Perhaps I am not thinking about the right kind of queries.
>> I have been thinking about examples like:
>> 
>> SELECT x, CASE WHEN y THEN generate_series(1,z) ELSE 5 END
>> 	FROM table_with_columns_x_and_y_and_z;
>> 
>> Which to me gives 'z' output rows per table row where y is true, and
>> one output row per table row where y is false.
> 
> Try any query that has one SRF outside of the CASE, and one inside.  In
> the old behaviour that'll make the total number of rows returned nearly
> undeterministic because of the least-common-multiple behaviour.
> 
>> That could be changed with an aggregate function such as:
>> SELECT x, CASE WHEN y THEN SUM(generate_series(1,z)) ELSE 5 END
>> 	FROM table_with_columns_x_and_y;
> 
> That query doesn't work.  First off, aggregates don't take set arguments
> (neither in old nor new releases), secondly aggregates are evaluated
> independently of CASE/COALESCE statements, thirdly you're missing group
> bys.  Those all are independent of the v10 changes.

Sorry, I was not clear.  What I meant to get at was that if you remove from the
executor all support for SRFs inside case statements, you might foreclose the option
of extending the syntax at some later date to allow aggregates over SRFs.  I'm
not saying that this works currently, but in principle if you allowed that SUM() that
I put up there, you'd get back exactly one row from it, same as you get from the
ELSE clause.  That would seem to solve the problem without going so far as
completely disallowing the SRF altogether.  The reasons for not putting a GROUP BY
clause in the example are (a) I don't know where it would go, syntactically, and (b)
it would defeat the purpose, because once you are grouping, you again have the
possibility of getting more than one row.

I'm not advocating implementing this; I'm just speculating about possible future
features in which SRFs inside a case statement might be allowed.

> 
>> Thanks, and my apologies if I am merely lacking sufficient imagination to
>> think of a proper example.
> 
> Might be worthwhile to reread the thread about the SRF reimplementation.
> 
> https://www.postgresql.org/message-id/20160822214023.aaxz5l4igypowyri@alap3.anarazel.de

This helps, thanks!

Mark Dilger

  12. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Andres Freund <andres@anarazel.de> — 2017-06-04T21:19:42Z

    On 2017-06-04 14:16:14 -0700, Mark Dilger wrote:
> Sorry, I was not clear.  What I meant to get at was that if you remove from the
> executor all support for SRFs inside case statements, you might foreclose the option
> of extending the syntax at some later date to allow aggregates over
> SRFs.

Seems very unlikely that we'd ever want to do that.  The right way to do
this is to simply move the SRF into the from list.  Having the executor
support arbitrary sources of tuples would just complicate and slow down
already complicated and slow code...


> I'm
> not saying that this works currently, but in principle if you allowed that SUM() that
> I put up there, you'd get back exactly one row from it, same as you get from the
> ELSE clause.  That would seem to solve the problem without going so far as
> completely disallowing the SRF altogether.

But what would the benefit be?


Greetings,

Andres Freund

  13. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Mark Dilger <hornschnorter@gmail.com> — 2017-06-04T21:32:24Z

    > On Jun 4, 2017, at 2:19 PM, Andres Freund <andres@anarazel.de> wrote:
> 
> On 2017-06-04 14:16:14 -0700, Mark Dilger wrote:
>> Sorry, I was not clear.  What I meant to get at was that if you remove from the
>> executor all support for SRFs inside case statements, you might foreclose the option
>> of extending the syntax at some later date to allow aggregates over
>> SRFs.
> 
> Seems very unlikely that we'd ever want to do that.  The right way to do
> this is to simply move the SRF into the from list.  Having the executor
> support arbitrary sources of tuples would just complicate and slow down
> already complicated and slow code...
> 
> 
>> I'm
>> not saying that this works currently, but in principle if you allowed that SUM() that
>> I put up there, you'd get back exactly one row from it, same as you get from the
>> ELSE clause.  That would seem to solve the problem without going so far as
>> completely disallowing the SRF altogether.
> 
> But what would the benefit be?

In my example, the aggregate function is taking a column from the table as
an argument, so the output of the aggregate function needs to be computed per row,
not just once.  And if the function is expensive, or has side-effects, you might
only want it to execute for those rows where the CASE statement is true, rather
than for all of them.  You may get that same behavior using lateral or some such,
I'm uncertain, but in a complicated CASE statement, it be more straightforward
to write something like:

SELECT
	CASE
		WHEN t.x = 'foo' THEN expensive_aggfunc1(srf1(t.y,t.z))
		WHEN t.x = 'bar' THEN expensive_aggfunc2(srf2(t.y,t.z))
		WHEN t.x = 'baz' THEN expensive_aggfunc3(srf3(t.y,t.z))
		....
		WHEN t.x = 'zzz' THEN expensive_aggfuncN(srfN(t.y,t.z))
		ELSE 5
	END
FROM mytable t;

Than to try to write it any other way.


I'm not advocating anything here, even though it may sound that way to you.
I'm just thinking this thing through, given that you may be committing a removal
of functionality that we want back at some later time.

Out of curiosity, how would you rewrite what I have above such that the
aggregate function is not inside the case statement, and the expensive_aggfuncs
are only called for those (t.y,t.z) that are actually appropriate?


Mark Dilger

  14. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-07T18:57:22Z

    Mark Dilger <hornschnorter@gmail.com> writes:
>> On Jun 4, 2017, at 2:19 PM, Andres Freund <andres@anarazel.de> wrote:
>> Seems very unlikely that we'd ever want to do that.  The right way to do
>> this is to simply move the SRF into the from list.  Having the executor
>> support arbitrary sources of tuples would just complicate and slow down
>> already complicated and slow code...

> In my example, the aggregate function is taking a column from the table as
> an argument, so the output of the aggregate function needs to be computed per row,
> not just once.  And if the function is expensive, or has side-effects, you might
> only want it to execute for those rows where the CASE statement is true, rather
> than for all of them.  You may get that same behavior using lateral or some such,
> I'm uncertain, but in a complicated CASE statement, it be more straightforward
> to write something like:

> SELECT
> 	CASE
> 		WHEN t.x = 'foo' THEN expensive_aggfunc1(srf1(t.y,t.z))
> 		WHEN t.x = 'bar' THEN expensive_aggfunc2(srf2(t.y,t.z))
> 		WHEN t.x = 'baz' THEN expensive_aggfunc3(srf3(t.y,t.z))
> 		....
> 		WHEN t.x = 'zzz' THEN expensive_aggfuncN(srfN(t.y,t.z))
> 		ELSE 5
> 	END
> FROM mytable t;

I think the correct way to do that already exists, namely to use a
correlated sub-select to wrap each SRF+aggregate:

    ...
    WHEN t.x = 'foo' THEN (SELECT expensive_aggfunc1(s) FROM srf1(t.y,t.z) s)
    ...

I don't really feel a need to invent some other notation for that.

After chewing on this for awhile, I'm starting to come to the conclusion
that we'd be best off to throw an error for SRF-inside-CASE (or
COALESCE).  Mark is correct that the simplest case of

	SELECT x, CASE WHEN y THEN generate_series(1,z) ELSE 5 END
	FROM table_with_columns_x_and_y_and_z;

behaves just intuitively enough that people might be using it.  The new
implementation method cannot reasonably duplicate the old semantics for
that, which means that if we let it stand as-is we will be silently
breaking queries, even if we fix up some of the weirder corner cases like
what happens when the CASE can be const-simplified.  So I think we'd be
better off to make this throw an error, and force any affected users to
rewrite in a way that will work in both v10 and older releases.

As to *how* to throw an error, I think it should be possible to teach
parse analysis to detect such cases, with something like the
ParseExprKind mechanism that could be checked to see if we're inside
a subexpression that restricts what's allowed.  There are some other
checks like no-nested-aggregates that perhaps could be folded in as
well.  Checking at parse analysis ought to be sufficient because
rule rewriting could not introduce such a case where it wasn't before,
and planner subquery flattening won't introduce one either because we
don't flatten subqueries with SRFs in their tlists.

If people are on board with throwing an error, I'll go see about
writing a patch.

			regards, tom lane

  15. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    David G. Johnston <david.g.johnston@gmail.com> — 2017-06-07T19:00:44Z

    On Wed, Jun 7, 2017 at 11:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> If people are on board with throwing an error, I'll go see about
> writing a patch.
>

+1 from me.

David J.​

  16. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Regina Obe <lr@pcorp.us> — 2017-06-07T21:07:10Z

    > After chewing on this for awhile, I'm starting to come to the conclusion
that we'd be best off to throw an error for SRF-inside-CASE (or COALESCE).
Mark is correct that the simplest case of

>	SELECT x, CASE WHEN y THEN generate_series(1,z) ELSE 5 END
>	FROM table_with_columns_x_and_y_and_z;

> behaves just intuitively enough that people might be using it.  The new
implementation method cannot reasonably duplicate the old semantics for
that, which means that if we let it stand as-is we will be 

> silently breaking queries, even if we fix up some of the weirder corner
cases like what happens when the CASE can be const-simplified.  So I think
we'd be better off to make this throw an error, and force any 
> affected users to rewrite in a way that will work in both v10 and older
releases.

> As to *how* to throw an error, I think it should be possible to teach
parse analysis to detect such cases, with something like the ParseExprKind
mechanism that could be checked to see if we're inside a 
> subexpression that restricts what's allowed.  There are some other checks
like no-nested-aggregates that perhaps could be folded in as well.  Checking
at parse analysis ought to be sufficient because 
> rule rewriting could not introduce such a case where it wasn't before, and
planner subquery flattening won't introduce one either because we don't
flatten subqueries with SRFs in their tlists.

> If people are on board with throwing an error, I'll go see about writing a
patch.

>			regards, tom lane

+1

I'm not a fan of either solution, but I think what Tom proposes of throwing
an error sounds like least invasive and confusing.

I'd much prefer an error thrown than silent behavior change. Given that we
ran into this in 3 places in PostGIS code, I'm not convinced the issue is
all that rare.

Make sure to point out the breaking change in the release notes though and
syntax to remedy it.

Thanks,
Regina

  17. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-08T02:01:57Z

    "Regina Obe" <lr@pcorp.us> writes:
> I'm not a fan of either solution, but I think what Tom proposes of throwing
> an error sounds like least invasive and confusing.

> I'd much prefer an error thrown than silent behavior change. Given that we
> ran into this in 3 places in PostGIS code, I'm not convinced the issue is
> all that rare.

> Make sure to point out the breaking change in the release notes though and
> syntax to remedy it.

As far as that goes, the best fix I could think of after a few minutes is
to integrate your conditional logic into a custom set-returning function.
For example,

    select x, case when y > 0 then generate_series(1, z) else 5 end from tt;

could become

    create function mysrf(cond bool, start int, fin int, els int)
      returns setof int as $$
    begin
      if cond then
        return query select generate_series(start, fin);
      else
        return query select els;
      end if;
    end$$ language plpgsql;

    select x, mysrf(y > 0, 1, z, 5) from tt;

(adjust the amount of parameterization to taste, of course)

Now, the fact that a fairly mechanical conversion like this is possible
suggests that we *could* solve the problem if we had to, at least for
simple cases like this one.  But it'd be a lot of work, not least because
we'd presumably not want core-defined syntax to depend on an extension
like plpgsql --- and I don't see a way to do this with straight SQL
functions.  So my feeling is that we should not expend that effort.
If it turns out that a lot more people are affected than I currently
think will be the case, maybe we'll have to revisit that choice.

But this line of thinking does strengthen my feeling that throwing an
error is the right thing to do for the moment.  If we allow v10 to accept
such cases but do something different from what we used to, that will
greatly complicate any future attempt to try to restore the old behavior.

			regards, tom lane

  18. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Regina Obe <lr@pcorp.us> — 2017-06-08T15:57:49Z

    > But this line of thinking does strengthen my feeling that throwing an
error is the right thing to do for the moment.  If we allow v10 to accept
such cases but do something different from what we used to, that 
> will greatly complicate any future attempt to try to restore the old
behavior.

>			regards, tom lane

Agreed.  The other side benefit of throwing an error instead of just doing
something different is you'll find out how rampant the old behavior is :).

People are more likely to know to complain when their apps break than they
are if it just silently starts doing something different.

My main concern in these cases is the short-circuiting not happening.
Because in these cases, the code goes into areas that it shouldn't which is
likely to mess up some logic in hard to troubleshoot ways.
I think erroring out is the best compromise.

Thanks,
Regina

  19. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Andres Freund <andres@anarazel.de> — 2017-06-08T17:07:22Z

    On 2017-06-08 11:57:49 -0400, Regina Obe wrote:
> My main concern in these cases is the short-circuiting not happening.

Note there's also no short-circuiting e.g. for aggregates inside case
either.

- Andres

  20. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-08T17:50:57Z

    "'Andres Freund'" <andres@anarazel.de> writes:
> On 2017-06-08 11:57:49 -0400, Regina Obe wrote:
>> My main concern in these cases is the short-circuiting not happening.

> Note there's also no short-circuiting e.g. for aggregates inside case
> either.

Well, depends.  If const-folding manages to get rid of the aggregate
call altogether, it won't be computed.

			regards, tom lane

  21. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-09T03:05:53Z

    I wrote:
> As to *how* to throw an error, I think it should be possible to teach
> parse analysis to detect such cases, with something like the
> ParseExprKind mechanism that could be checked to see if we're inside
> a subexpression that restricts what's allowed.  There are some other
> checks like no-nested-aggregates that perhaps could be folded in as
> well.

I spent some time experimenting with this, and immediately found out
that information_schema.user_mapping_options contains an instance of the
problematic usage :-(.  However, that view also turns out to be a poster
child for why our old SRF semantics are a bad idea, because it's on the
hairy edge of failing completely.  It's got two SRF invocations in its
tlist, which it relies on to execute in lockstep and produce the same
number of rows --- but then it puts a CASE around one of them, with an
ELSE NULL.  So in old versions, that only works because if the CASE
doesn't return a set result at runtime then we just run the tlist the
number of times suggested by the other SRF.  And in HEAD, it only works
because we run the two SRFs in lockstep behind the scenes and then the
CASE is discarding individual results not the whole execution of the
second SRF.  If you don't have a headache at this point, re-read the above
until you do.  Or if you fully understand that and still think it's fine,
consider what will happen if the CASE's test expression generates
different results from one call to the next --- which I think is actually
possible here, because pg_has_role() operates on CatalogSnapshot time and
might possibly change its mind partway through the query.  Pre-v10, that
would have generated completely messed-up output, with a hard-to-predict
number of rows and unexpected combinations of the two SRFs' outputs.

Rewriting this view to use a LATERAL SRF call is just enormously cleaner.

Anyway, to the subject at hand.  My thought when I wrote the previous
message was to implement a "top down" mechanism whereby contexts like
CASE and COALESCE would record their presence in the ParseState while
recursively analyzing their arguments, and then SRF calls would be
responsible for throwing an error by inspecting that context.  The first
attached patch does it that way, and it seems nice and clean, but I ran
into a complete dead end while trying to extend it to handle related cases
such as disallowing SRF-inside-aggregate or nested SRFs in FROM.  The
trouble is that when we are parsing the arguments of a SRF or aggregate,
we don't actually know yet whether it's a SRF or aggregate or plain
function.  We can't find that out until we look up the pg_proc entry,
which we can't do without knowing the argument types, so we have to do
parse analysis of the arguments first.

I then considered a "bottom up" approach where the idea is for each SRF
to report its presence in the ParseState, and then the outer construct
complains if any SRF reported its presence within the outer construct's
arguments.  This isn't hard to implement, just keep a "p_last_srf" pointer
in the ParseState, as in the second attached patch.  This feels more ugly
than the first approach; for one thing, if we want to extend it to
multiple cases of "X cannot contain a Y" then we need a ParseState field
for each kind of Y we care about.  Also, it will tend to complain about
the last Y within a given X construct, not the first one; but only a true
geek would ever even notice that, let alone feel that it's strange.

I didn't actually fix the "bottom up" approach to complain about nested
SRFs.  It's clear to me how to do so, but because we analyze function
arguments before calling ParseFuncOrColumn, we'd have to have the callers
of that function remember the appropriate p_last_srf value (ie, from just
before they analyze the arguments) and then pass it to ParseFuncOrColumn.
That would add a bunch of uninteresting clutter to the patch so I didn't
do it here.

I'm inclined to go with the "bottom up" approach, but I wonder if anyone
has any comments or better ideas?

			regards, tom lane

  22. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Andres Freund <andres@anarazel.de> — 2017-06-09T03:26:19Z

    Hi,

On 2017-06-08 23:05:53 -0400, Tom Lane wrote:
> I spent some time experimenting with this, and immediately found out
> that information_schema.user_mapping_options contains an instance of the
> problematic usage :-(.  However, that view also turns out to be a poster
> child for why our old SRF semantics are a bad idea, because it's on the
> hairy edge of failing completely. [...]

Yuck.


> Anyway, to the subject at hand.  My thought when I wrote the previous
> message was to implement a "top down" mechanism whereby contexts like
> CASE and COALESCE would record their presence in the ParseState while
> recursively analyzing their arguments, and then SRF calls would be
> responsible for throwing an error by inspecting that context.  The first
> attached patch does it that way, and it seems nice and clean, but I ran
> into a complete dead end while trying to extend it to handle related cases
> such as disallowing SRF-inside-aggregate or nested SRFs in FROM.

But, do we really need to handle those?  IOW, isn't handling
CASE/COALESCE, which we can recognize early in parse analysis,
sufficient to error out here?  It'd be a lot nicer if we could error
about SRF arguments to aggregates et al. during analysis rather than
execution, but there's not a comparably huge need to improve there.


> I'm inclined to go with the "bottom up" approach, but I wonder if anyone
> has any comments or better ideas?

I'll have a look at the patches tomorrow, but I'm too tired to
meaningfully read code tonight.

- Andres

  23. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-09T03:37:49Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-06-08 23:05:53 -0400, Tom Lane wrote:
>> ... The first
>> attached patch does it that way, and it seems nice and clean, but I ran
>> into a complete dead end while trying to extend it to handle related cases
>> such as disallowing SRF-inside-aggregate or nested SRFs in FROM.

> But, do we really need to handle those?  IOW, isn't handling
> CASE/COALESCE, which we can recognize early in parse analysis,
> sufficient to error out here?  It'd be a lot nicer if we could error
> about SRF arguments to aggregates et al. during analysis rather than
> execution, but there's not a comparably huge need to improve there.

Yes, we already have guards for those cases, but they return fairly opaque
error messages to the tune of "set-valued function called in context that
cannot accept a set", because the executor hasn't enough context to do
better.  I'd like the messages to be more specific, like "set-valued
function cannot appear within CASE" and so on.  Anyway the point here is
to evaluate different approaches to changing the parser on the basis of
whether they *could* be extended to handle related cases.  Whether we
actually do that right now is less important.

			regards, tom lane

  24. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-09T21:33:45Z

    I wrote:
> Yes, we already have guards for those cases, but they return fairly opaque
> error messages to the tune of "set-valued function called in context that
> cannot accept a set", because the executor hasn't enough context to do
> better.  I'd like the messages to be more specific, like "set-valued
> function cannot appear within CASE" and so on.

Here's an expanded version of the "bottom up" patch that adjusts some
parser APIs to allow these additional messages to be thrown.  This changes
all occurrences of "set-valued function called in context that cannot
accept a set" in the core regression tests into something more specific.
There are probably some remaining cases that this doesn't cover, but the
existing execution-time checks will still catch those.

In passing, I added explicit checks that the operator doesn't return
set to transformAExprNullIf and make_distinct_op.  We used to be a bit
laissez-faire about that, expecting that the executor would throw
errors for such cases.  But now there are hard-wired assumptions that
only FuncExpr and OpExpr nodes could return sets, so we've got to make
sure that NullIfExpr and DistinctExpr nodes don't.

(In the other direction, I suspect that expression_returns_set() is now
too optimistic about believing that it needn't descend into the arguments
of nodes whose execution code used to reject SRF results.  But that's a
matter for a separate patch.)

			regards, tom lane

  25. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-06-10T18:04:19Z

    Tom Lane wrote:
> I wrote:
> > Yes, we already have guards for those cases, but they return fairly opaque
> > error messages to the tune of "set-valued function called in context that
> > cannot accept a set", because the executor hasn't enough context to do
> > better.  I'd like the messages to be more specific, like "set-valued
> > function cannot appear within CASE" and so on.
> 
> Here's an expanded version of the "bottom up" patch that adjusts some
> parser APIs to allow these additional messages to be thrown.  This changes
> all occurrences of "set-valued function called in context that cannot
> accept a set" in the core regression tests into something more specific.
> There are probably some remaining cases that this doesn't cover, but the
> existing execution-time checks will still catch those.

Interesting stuff.  Here's a small recommendation for a couple of those
new messages.



-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  26. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-11T18:18:41Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Interesting stuff.  Here's a small recommendation for a couple of those
> new messages.

Hm.  I don't object to folding those two messages into one, but now that
I look at it, the text needs some more work anyway, perhaps.  What we're
actually checking is not so much whether the IS DISTINCT FROM construct
returns a set as whether the underlying equality operator does.  If we
want to be pedantic about it, we'd end up writing something like

	"equality operator used by %s must not return a set"

But perhaps it's okay to fuzz the distinction and just write

	"%s must not return a set"

You could justify that on the reasoning that if we were to allow this
then an underlying "=" that returned a set would presumably cause
IS DISTINCT FROM or NULLIF to also return a set.

I'm kind of thinking that the second wording is preferable, but there's
room to argue that the first is more precise.  Opinions?

			regards, tom lane

  27. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Andres Freund <andres@anarazel.de> — 2017-06-13T00:32:28Z

    On 2017-06-09 17:33:45 -0400, Tom Lane wrote:
> diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
> index cbcd6cf..98bcfa0 100644
> --- a/src/backend/catalog/information_schema.sql
> +++ b/src/backend/catalog/information_schema.sql
> @@ -2936,12 +2936,14 @@ CREATE VIEW user_mapping_options AS
>      SELECT authorization_identifier,
>             foreign_server_catalog,
>             foreign_server_name,
> -           CAST((pg_options_to_table(um.umoptions)).option_name AS sql_identifier) AS option_name,
> +           CAST(opts.option_name AS sql_identifier) AS option_name,
>             CAST(CASE WHEN (umuser <> 0 AND authorization_identifier = current_user)
>                         OR (umuser = 0 AND pg_has_role(srvowner, 'USAGE'))
> -                       OR (SELECT rolsuper FROM pg_authid WHERE rolname = current_user) THEN (pg_options_to_table(um.umoptions)).option_value
> +                       OR (SELECT rolsuper FROM pg_authid WHERE rolname = current_user)
> +                     THEN opts.option_value
>                       ELSE NULL END AS character_data) AS option_value
> -    FROM _pg_user_mappings um;
> +    FROM _pg_user_mappings um,
> +         pg_options_to_table(um.umoptions) opts;

This really is a lot better...


>  GRANT SELECT ON user_mapping_options TO PUBLIC;
>  
> diff --git a/src/backend/executor/functions.c b/sindex a35ba32..89aea2f 100644
> --- a/src/backend/executor/functions.c
> +++ b/src/backend/executor/functions.c
> @@ -388,6 +388,7 @@ sql_fn_post_column_ref(ParseState *pstat
>  		param = ParseFuncOrColumn(pstate,
>  								  list_make1(subfield),
>  								  list_make1(param),
> +								  pstate->p_last_srf,
>  								  NULL,
>  								  cref->location);
>  	}
> diff --git a/src/backend/parser/parse_aindex efe1c37..5241fd2 100644
> --- a/src/backend/parser/parse_agg.c
> +++ b/src/backend/parser/parse_agg.c
> @@ -705,6 +705,13 @@ check_agg_arguments_walker(Node *node,
>  		}
>  		/* Continue and descend into subtree */
>  	}
> +	/* We can throw error on sight for a set-returning function */
> +	if ((IsA(node, FuncExpr) &&((FuncExpr *) node)->funcretset) ||
> +		(IsA(node, OpExpr) &&((OpExpr *) node)->opretset))
> +		ereport(ERROR,
> +				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +				 errmsg("aggregate function calls cannot contain set-returning function calls"),
> +				 parser_errposition(context->pstate, exprLocation(node))));

Possibly too hard to be precise enough in a hint, but a number of these
could benefit from one suggesting moving things into FROM, using
LATERAL.

I'm kinda positively surprised at how non-invasive this turned out, I'd
afraid there'd be a lot more verbosity to it.  I think the improved
error messages (message & location), are quite worthwhile an their own.

Greetings,

Andres Freund

  28. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-13T01:03:31Z

    Andres Freund <andres@anarazel.de> writes:
> Possibly too hard to be precise enough in a hint, but a number of these
> could benefit from one suggesting moving things into FROM, using
> LATERAL.

Maybe "You might be able to move the set-returning function into a
LATERAL FROM item."?

> I'm kinda positively surprised at how non-invasive this turned out, I'd
> afraid there'd be a lot more verbosity to it.  I think the improved
> error messages (message & location), are quite worthwhile an their own.

Yeah, me too --- I'm pretty pleased with the result.

			regards, tom lane

  29. Re: PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

    Andres Freund <andres@anarazel.de> — 2017-06-13T01:26:11Z

    On 2017-06-12 21:03:31 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Possibly too hard to be precise enough in a hint, but a number of these
> > could benefit from one suggesting moving things into FROM, using
> > LATERAL.
> 
> Maybe "You might be able to move the set-returning function into a
> LATERAL FROM item."?

WFM, seems at least better than the current and current-as-proposed
state.


- Andres