Thread: Removing WITH clause support in CREATE FUNCTION, for isCachable andisStrict

Hi all,

As noticed by Daniel here:
https://www.postgresql.org/message-id/D5F34C9D-3AB7-4419-AF2E-12F67581D71D@yesql.se

Using a WITH clause takes precendence over what is defined in the main
function definition when using isStrict and isCachable. For example,
when using VOLATILE and IMMUTABLE, an error is raised:
=# create function int42(cstring) returns int42 AS 'int4in'
   language internal strict immutable volatile;
ERROR:  42601: conflicting or redundant options
LINE 2:     language internal strict immutable volatile;

However when using for example STABLE/VOLATILE in combination with a
WITH clause, then things get prioritized, and in this case the WITH
clause values are taken into account:
=# create function int42(cstring) returns int42 AS 'int4in'
   language internal strict volatile with (isstrict, iscachable);
CREATE FUNCTION
=# select provolatile from pg_proc where proname = 'int42';
 provolatile
-------------
 i
(1 row)

This clause is marked as deprecated since 7.3, so perhaps it would be
time to remove completely its support? It seems to me that this leads to
more confusion than being helpful. And I have not found a trace of code
using those flags on github or such.

Thanks,
--
Michael

> On 15 Jan 2018, at 03:27, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> Hi all,
>
> As noticed by Daniel here:
> https://www.postgresql.org/message-id/D5F34C9D-3AB7-4419-AF2E-12F67581D71D@yesql.se

In that thread I proposed a patch to fix this, but I certainly don’t object to
just removing it to make both syntax and code clearer. +1

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 15 Jan 2018, at 03:27, Michael Paquier <michael.paquier@gmail.com> wrote:
>> As noticed by Daniel here:
>> https://www.postgresql.org/message-id/D5F34C9D-3AB7-4419-AF2E-12F67581D71D@yesql.se

> In that thread I proposed a patch to fix this, but I certainly don’t object to
> just removing it to make both syntax and code clearer. +1

I poked into the git log and confirmed Michael's statement that
CREATE FUNCTION ... WITH has been documented as deprecated since
7.3 (commit 94bdc4855 to be exact).

I think the original intention was that we'd keep it for PG-specific
function attributes (ie those not found in the SQL spec), so as to
avoid creating keywords for every attribute we thought of.  That
intention has been roundly ignored since then, though, since COST
and ROWS and LEAKPROOF and PARALLEL (UN)SAFE have all been implemented
as separate keywords not WITH options.  I'm dubious that that was a good
plan, but it's probably too late to change direction now.

The only other argument I can see for keeping it is that pre-7.3
dump files could contain CREATE FUNCTION commands with the old
spelling.  But, even assuming somebody is still running <= 7.2
and wants to migrate to >= 11 in one jump, they could use some
intermediate version of pg_dump to produce a dump with the modern
spelling, or just whip out their trusty text editor.  Maintaining
backwards compatibility is good, but 15 years seems like enough.

In short, I'm on board with removing the WITH clause.  I've not
reviewed the patch in detail, but will do so and push it if there's
not objections pretty soon.

            regards, tom lane

On Thu, Jan 25, 2018 at 06:30:04PM -0500, Tom Lane wrote:
> I poked into the git log and confirmed Michael's statement that
> CREATE FUNCTION ... WITH has been documented as deprecated since
> 7.3 (commit 94bdc4855 to be exact).

Thanks for the confirmation.

> I think the original intention was that we'd keep it for PG-specific
> function attributes (ie those not found in the SQL spec), so as to
> avoid creating keywords for every attribute we thought of.  That
> intention has been roundly ignored since then, though, since COST
> and ROWS and LEAKPROOF and PARALLEL (UN)SAFE have all been implemented
> as separate keywords not WITH options.  I'm dubious that that was a good
> plan, but it's probably too late to change direction now.

:(

> In short, I'm on board with removing the WITH clause.  I've not
> reviewed the patch in detail, but will do so and push it if there's
> not objections pretty soon.

Glad to hear that, thanks!
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Thu, Jan 25, 2018 at 06:30:04PM -0500, Tom Lane wrote:
>> In short, I'm on board with removing the WITH clause.  I've not
>> reviewed the patch in detail, but will do so and push it if there's
>> not objections pretty soon.

> Glad to hear that, thanks!

And done.  I failed to resist the temptation to rename
compute_attributes_sql_style, since the "sql_style" bit no longer
conveys anything.  I'd always found compute_attributes_with_style
to be confusingly named --- seemed like it should have a sibling
compute_attributes_gracelessly, or something like that.

            regards, tom lane

On Fri, Jan 26, 2018 at 12:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Thu, Jan 25, 2018 at 06:30:04PM -0500, Tom Lane wrote:
>>> In short, I'm on board with removing the WITH clause.  I've not
>>> reviewed the patch in detail, but will do so and push it if there's
>>> not objections pretty soon.
>
>> Glad to hear that, thanks!
>
> And done.  I failed to resist the temptation to rename
> compute_attributes_sql_style, since the "sql_style" bit no longer
> conveys anything.  I'd always found compute_attributes_with_style
> to be confusingly named --- seemed like it should have a sibling
> compute_attributes_gracelessly, or something like that.

+1 for compute_attributes_gracelessly!  :-)

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

On Fri, Jan 26, 2018 at 12:30:12PM -0500, Tom Lane wrote:
> And done.  I failed to resist the temptation to rename
> compute_attributes_sql_style, since the "sql_style" bit no longer
> conveys anything.  I'd always found compute_attributes_with_style
> to be confusingly named --- seemed like it should have a sibling
> compute_attributes_gracelessly, or something like that.

Nice, thanks for the commit and review!
--
Michael