Обсуждение: Relax requirement for INTO with SELECT in pl/pgsql

Patch is trivial (see below), discussion is not :-).

I see no useful reason to require INTO when returning data with
SELECT.  However, requiring queries to indicate not needing data via
PERFORM causes some annoyances:

*) converting routines back and forth between pl/pgsql and pl/sql
requires needless busywork and tends to cause errors to be thrown at
runtime

*) as much as possible, (keywords begin/end remain a problem),
pl/pgsql should be a superset of sql

*) it's much more likely to be burned by accidentally forgetting to
swap in PERFORM than to accidentally leave in a statement with no
actionable target.  Even if you did so in the latter case, it stands
to reason you'd accidentally leave in the target variable, too.

*) the PERFORM requirement hails from the days when only statements
starting with SELECT return data.  There is no PERFORM equivalent for
WITH/INSERT/DELETE/UPDATE and there are real world scenarios where you
might have a RETURNING clause that does something but not necessarily
want to place the result in a variable (for example passing to
volatile function).  Take a look at the errhint() clause below -- we
don't even have a suggestion in that case.

This has come up before, and there was a fair amount of sympathy for
this argument albeit with some dissent -- notably Pavel.  I'd like to
get a hearing on the issue -- thanks.  If we decide to move forward,
this would effectively deprecate PERFORM and the documentation will be
suitably modified as well.

merlin



diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b7f44ca..a860066 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3457,12 +3457,9 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,   }   else   {
-       /* If the statement returned a tuple table, complain */
+       /* If the statement returned a tuple table, free it. */       if (SPI_tuptable != NULL)
-           ereport(ERROR,
-                   (errcode(ERRCODE_SYNTAX_ERROR),
-                    errmsg("query has no destination for result data"),
-                    (rc == SPI_OK_SELECT) ? errhint("If you want to
discard the results of a SELECT, use PERFORM instead.") : 0));
+           SPI_freetuptable(SPI_tuptable);   }
   if (paramLI)

On Mon, Mar 21, 2016 at 4:13 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hi
>
> 2016-03-21 21:24 GMT+01:00 Merlin Moncure <mmoncure@gmail.com>:
>>
>> Patch is trivial (see below), discussion is not :-).
>>
>> I see no useful reason to require INTO when returning data with
>> SELECT.  However, requiring queries to indicate not needing data via
>> PERFORM causes some annoyances:
>>
>> *) converting routines back and forth between pl/pgsql and pl/sql
>> requires needless busywork and tends to cause errors to be thrown at
>> runtime
>>
>> *) as much as possible, (keywords begin/end remain a problem),
>> pl/pgsql should be a superset of sql
>>
>> *) it's much more likely to be burned by accidentally forgetting to
>> swap in PERFORM than to accidentally leave in a statement with no
>> actionable target.  Even if you did so in the latter case, it stands
>> to reason you'd accidentally leave in the target variable, too.
>>
>> *) the PERFORM requirement hails from the days when only statements
>> starting with SELECT return data.  There is no PERFORM equivalent for
>> WITH/INSERT/DELETE/UPDATE and there are real world scenarios where you
>> might have a RETURNING clause that does something but not necessarily
>> want to place the result in a variable (for example passing to
>> volatile function).  Take a look at the errhint() clause below -- we
>> don't even have a suggestion in that case.
>>
>> This has come up before, and there was a fair amount of sympathy for
>> this argument albeit with some dissent -- notably Pavel.  I'd like to
>> get a hearing on the issue -- thanks.  If we decide to move forward,
>> this would effectively deprecate PERFORM and the documentation will be
>> suitably modified as well.
>
>
> My negative opinion is known. The PERFORM statement is much more workaround
> than well designed statement, but I would to see ANSI/SQL based fix. I try
> to compare benefits and loss.

Well, pl/pgsql is based on oracle pl/sql so I don't see how the
standard is involved.  FWICT, "PERFORM" is a postgres extension to
pl/pgsql.  I don't see how the standard plays at all.

> Can you start with analyze what is possible, and what semantic is allowed in
> standard and other well known SQL databases?

Typical use of PERFORM is void returning function.  Oracle allows use
of those functions without any decoration at all.   For example, in
postgres we might do:
PERFORM LogIt('I did something');

in Oracle, you'd simply do:
LogIt('I did something');

I'm not sure what Oracle does for SELECT statements without INTO/BULK
UPDATE.  I'm not really inclined to care -- I'm really curious to see
an argument where usage of PERFORM actually helps in some meaningful
way.  Notably, SELECT without INTO is accepted syntax, but fails only
after running the query.  I think that's pretty much stupid but it's
fair to say I'm not inventing syntax, only disabling the error.

I'm not sure what other databases do is relevant.   They use other
procedure languages than pl//sql (the biggest players are pl/psm and
t-sql) which have a different set of rules in terms of passing
variables in and out of queries.

merlin

On 3/21/16 5:03 PM, Merlin Moncure wrote:
> in Oracle, you'd simply do:
> LogIt('I did something');

It would be *great* if we could support that in plpgsql.

> I'm not sure what Oracle does for SELECT statements without INTO/BULK
> UPDATE.  I'm not really inclined to care -- I'm really curious to see
> an argument where usage of PERFORM actually helps in some meaningful
> way.  Notably, SELECT without INTO is accepted syntax, but fails only
> after running the query.  I think that's pretty much stupid but it's
> fair to say I'm not inventing syntax, only disabling the error.

I don't think it buys much at all.

While we're on the subject, it'd be great if variable := SELECT ... 
worked too.

> I'm not sure what other databases do is relevant.   They use other
> procedure languages than pl//sql (the biggest players are pl/psm and
> t-sql) which have a different set of rules in terms of passing
> variables in and out of queries.

+1
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 3/21/16 5:03 PM, Merlin Moncure wrote:
>> in Oracle, you'd simply do:
>> LogIt('I did something');

> It would be *great* if we could support that in plpgsql.

FWIW, I'm hesitant to just start accepting that syntax as if it were an
equivalent to "SELECT f(x)" or "PERFORM f(x)".  I think that someday
we will want to have the ability to have pass-by-reference parameters
to functions, and that's a fundamentally incompatible behavior so it
ought to be invoked by a new syntax.  I'd like to save "f(x)" as a
bare statement for that purpose.  We could also consider inventing
"CALL f(x)"; but supposing that we already had both "CALL f(x)" (which
would allow x to be pass-by-ref and possibly modified) and "SELECT f(x)"
(which would not), which one would you assign bare "f(x)" to mean?  It
should be "CALL f(x)", not least because that would be the semantics most
comparable to Oracle's behavior (since they have pass-by-ref parameters
already).

So, I'm -1 on not having any keyword at all.  I have no objection
to Merlin's proposal though.  I agree that PERFORM is starting to
look a bit silly, since it doesn't play with WITH for instance.

> While we're on the subject, it'd be great if variable := SELECT ... 
> worked too.

It does, though you might need parens around the sub-select.
        regards, tom lane

On Mon, Mar 21, 2016 at 5:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So, I'm -1 on not having any keyword at all.  I have no objection
> to Merlin's proposal though.  I agree that PERFORM is starting to
> look a bit silly, since it doesn't play with WITH for instance.

All right -- I'll submit a revised patch with documentation
adjustments.  "Basic Statements" needs to be revised, in particular
the section, "Executing a Command With No Result".  I'm inclined to
remove that section completely, and rename the next section from
"Executing a Query with a Single-row Result"  to simply, "Executing a
Query" (or perhaps "Non-Dynamic Query").

I'd like to remove documentation and usage of PERFORM except for a
note mentioning the old syntax...this would replace the current note
that existentially questions the necessity of PERFORM in the first
place.  Thanks.

merlin

On Mon, Mar 21, 2016 at 6:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So, I'm -1 on not having any keyword at all.  I have no objection
> to Merlin's proposal though.  I agree that PERFORM is starting to
> look a bit silly, since it doesn't play with WITH for instance.

Yeah, I think requiring PERFORM is stupid and annoying.  +1 for
letting people write a SELECT with no target.

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

Pavel Stehule <pavel.stehule@gmail.com> writes:
> I can live with SELECT fx(x). It is little bit dangerous, but this risk can
> be easy detected by plpgsql_check.

Dangerous how?

>> So, I'm -1 on not having any keyword at all.  I have no objection
>> to Merlin's proposal though.  I agree that PERFORM is starting to
>> look a bit silly, since it doesn't play with WITH for instance.

> Isn't time to fix PERFORM instead?

I do not think it can be fixed without embedding knowledge of PERFORM into
the core parser, which I doubt anybody would consider a good idea.
        regards, tom lane

On Tue, Mar 22, 2016 at 12:20 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> 2016-03-22 6:06 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
>>
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> > I can live with SELECT fx(x). It is little bit dangerous, but this risk
>> > can
>> > be easy detected by plpgsql_check.
>>
>> Dangerous how?
>
> I afraid of useless and forgotten call of functions. But the risk is same
> like PERFORM - so this is valid from one half. The PERFORM statement holds
> special semantic, and it is interesting.

I see your point here, but the cost of doing that far outweighs the
risks.  And I don't think the arbitrary standard of defining forcing
the user to identify if the query should return data is a good way of
identifying dead code.

Furthermore, after reviewing the docs, it's clear to me they've been
wrong for about a bazillion years.  In a couple of places they
mandated the use of PERFORM when the query returned rows, but it had
to be used even when the query was capable of returning rows
(regardless if it did or not).

Anyways, here's the patch with documentation adjustments as promised.
I ended up keeping the 'without result' section because it contained
useful information about plan caching,

merlin

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index 9786242..512eaa7
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*************** my_record.user_id := 20;
*** 904,910 ****     <title>Executing a Command With No Result</title>
     <para>
!      For any SQL command that does not return rows, for example      <command>INSERT</> without a
<literal>RETURNING</>clause, you can      execute the command within a <application>PL/pgSQL</application> function
just by writing the command.
 
--- 904,910 ----     <title>Executing a Command With No Result</title>
     <para>
!      For any SQL command, for example      <command>INSERT</> without a <literal>RETURNING</> clause, you can
executethe command within a <application>PL/pgSQL</application> function      just by writing the command.
 
*************** my_record.user_id := 20;
*** 925,972 ****      <xref linkend="plpgsql-plan-caching">.     </para>

-     <para>
-      Sometimes it is useful to evaluate an expression or <command>SELECT</>
-      query but discard the result, for example when calling a function
-      that has side-effects but no useful result value.  To do
-      this in <application>PL/pgSQL</application>, use the
-      <command>PERFORM</command> statement:
-
- <synopsis>
- PERFORM <replaceable>query</replaceable>;
- </synopsis>
-
-      This executes <replaceable>query</replaceable> and discards the
-      result.  Write the <replaceable>query</replaceable> the same
-      way you would write an SQL <command>SELECT</> command, but replace the
-      initial keyword <command>SELECT</> with <command>PERFORM</command>.
-      For <command>WITH</> queries, use <command>PERFORM</> and then
-      place the query in parentheses.  (In this case, the query can only
-      return one row.)
-      <application>PL/pgSQL</application> variables will be
-      substituted into the query just as for commands that return no result,
-      and the plan is cached in the same way.  Also, the special variable
-      <literal>FOUND</literal> is set to true if the query produced at
-      least one row, or false if it produced no rows (see
-      <xref linkend="plpgsql-statements-diagnostics">).
-     </para>
-     <note>      <para>
!       One might expect that writing <command>SELECT</command> directly
!       would accomplish this result, but at
!       present the only accepted way to do it is
!       <command>PERFORM</command>.  A SQL command that can return rows,
!       such as <command>SELECT</command>, will be rejected as an error
!       unless it has an <literal>INTO</> clause as discussed in the
!       next section.      </para>     </note>
     <para>      An example: <programlisting>
! PERFORM create_mv('cs_session_page_requests_mv', my_query); </programlisting>     </para>    </sect2>
--- 925,944 ----      <xref linkend="plpgsql-plan-caching">.     </para>
     <note>      <para>
!       In older versions of PostgreSQL, it was mandatory to use
!       <command>PERFORM</command> instead of <command>SELECT</command>
!       when the query could return data that was not captured into
!       variables.  This requirement has been relaxed and usage of
!       <command>PERFORM</command> has been deprecated.      </para>     </note>
     <para>      An example: <programlisting>
! SELECT create_mv('cs_session_page_requests_mv', my_query); </programlisting>     </para>    </sect2>
*************** GET DIAGNOSTICS integer_var = ROW_COUNT;
*** 1480,1491 ****           </listitem>           <listitem>            <para>
!             A <command>PERFORM</> statement sets <literal>FOUND</literal>             true if it produces (and
discards)one or more rows, false if             no row is produced.            </para>           </listitem>
<listitem>           <para>             <command>UPDATE</>, <command>INSERT</>, and <command>DELETE</>
statementsset <literal>FOUND</literal> true if at least one
 
--- 1452,1471 ----           </listitem>           <listitem>            <para>
!             A <command>SELECT</> statement sets <literal>FOUND</literal>             true if it produces (and
discards)one or more rows, false if             no row is produced.            </para>           </listitem>
<listitem>
+            <para>
+             A <command>PERFORM</> statement sets <literal>FOUND</literal>
+             true if it produces (and discards) one or more rows, false if
+             no row is produced.  This statement is equivalent to
+             <command>SELECT</> without INTO.
+            </para>
+           </listitem>
+           <listitem>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index 9786242..512eaa7
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*************** my_record.user_id := 20;
*** 904,910 ****     <title>Executing a Command With No Result</title>
     <para>
!      For any SQL command that does not return rows, for example      <command>INSERT</> without a
<literal>RETURNING</>clause, you can      execute the command within a <application>PL/pgSQL</application> function
just by writing the command.
 
--- 904,910 ----     <title>Executing a Command With No Result</title>
     <para>
!      For any SQL command, for example      <command>INSERT</> without a <literal>RETURNING</> clause, you can
executethe command within a <application>PL/pgSQL</application> function      just by writing the command.
 
*************** my_record.user_id := 20;
*** 925,972 ****      <xref linkend="plpgsql-plan-caching">.     </para>

-     <para>
-      Sometimes it is useful to evaluate an expression or <command>SELECT</>
-      query but discard the result, for example when calling a function
-      that has side-effects but no useful result value.  To do
-      this in <application>PL/pgSQL</application>, use the
-      <command>PERFORM</command> statement:
-
- <synopsis>
- PERFORM <replaceable>query</replaceable>;
- </synopsis>
-
-      This executes <replaceable>query</replaceable> and discards the
-      result.  Write the <replaceable>query</replaceable> the same
-      way you would write an SQL <command>SELECT</> command, but replace the
-      initial keyword <command>SELECT</> with <command>PERFORM</command>.
-      For <command>WITH</> queries, use <command>PERFORM</> and then
-      place the query in parentheses.  (In this case, the query can only
-      return one row.)
-      <application>PL/pgSQL</application> variables will be
-      substituted into the query just as for commands that return no result,
-      and the plan is cached in the same way.  Also, the special variable
-      <literal>FOUND</literal> is set to true if the query produced at
-      least one row, or false if it produced no rows (see
-      <xref linkend="plpgsql-statements-diagnostics">).
-     </para>
-     <note>      <para>
!       One might expect that writing <command>SELECT</command> directly
!       would accomplish this result, but at
!       present the only accepted way to do it is
!       <command>PERFORM</command>.  A SQL command that can return rows,
!       such as <command>SELECT</command>, will be rejected as an error
!       unless it has an <literal>INTO</> clause as discussed in the
!       next section.      </para>     </note>
     <para>      An example: <programlisting>
! PERFORM create_mv('cs_session_page_requests_mv', my_query); </programlisting>     </para>    </sect2>
--- 925,944 ----      <xref linkend="plpgsql-plan-caching">.     </para>
     <note>      <para>
!       In older versions of PostgreSQL, it was mandatory to use
!       <command>PERFORM</command> instead of <command>SELECT</command>
!       when the query could return data that was not captured into
!       variables.  This requirement has been relaxed and usage of
!       <command>PERFORM</command> has been deprecated.      </para>     </note>
     <para>      An example: <programlisting>
! SELECT create_mv('cs_session_page_requests_mv', my_query); </programlisting>     </para>    </sect2>
*************** GET DIAGNOSTICS integer_var = ROW_COUNT;
*** 1480,1491 ****           </listitem>           <listitem>            <para>
!             A <command>PERFORM</> statement sets <literal>FOUND</literal>             true if it produces (and
discards)one or more rows, false if             no row is produced.            </para>           </listitem>
<listitem>           <para>             <command>UPDATE</>, <command>INSERT</>, and <command>DELETE</>
statementsset <literal>FOUND</literal> true if at least one
 
--- 1452,1471 ----           </listitem>           <listitem>            <para>
!             A <command>SELECT</> statement sets <literal>FOUND</literal>             true if it produces (and
discards)one or more rows, false if             no row is produced.            </para>           </listitem>
<listitem>
+            <para>
+             A <command>PERFORM</> statement sets <literal>FOUND</literal>
+             true if it produces (and discards) one or more rows, false if
+             no row is produced.  This statement is equivalent to
+             <command>SELECT</> without INTO.
+            </para>
+           </listitem>
+           <listitem>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index 9786242..512eaa7
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*************** my_record.user_id := 20;
*** 904,910 ****     <title>Executing a Command With No Result</title>
     <para>
!      For any SQL command that does not return rows, for example      <command>INSERT</> without a
<literal>RETURNING</>clause, you can      execute the command within a <application>PL/pgSQL</application> function
just by writing the command.
 
--- 904,910 ----     <title>Executing a Command With No Result</title>
     <para>
!      For any SQL command, for example      <command>INSERT</> without a <literal>RETURNING</> clause, you can
executethe command within a <application>PL/pgSQL</application> function      just by writing the command.
 
*************** my_record.user_id := 20;
*** 925,972 ****      <xref linkend="plpgsql-plan-caching">.     </para>

-     <para>
-      Sometimes it is useful to evaluate an expression or <command>SELECT</>
-      query but discard the result, for example when calling a function
-      that has side-effects but no useful result value.  To do
-      this in <application>PL/pgSQL</application>, use the
-      <command>PERFORM</command> statement:
-
- <synopsis>
- PERFORM <replaceable>query</replaceable>;
- </synopsis>
-
-      This executes <replaceable>query</replaceable> and discards the
-      result.  Write the <replaceable>query</replaceable> the same
-      way you would write an SQL <command>SELECT</> command, but replace the
-      initial keyword <command>SELECT</> with <command>PERFORM</command>.
-      For <command>WITH</> queries, use <command>PERFORM</> and then
-      place the query in parentheses.  (In this case, the query can only
-      return one row.)
-      <application>PL/pgSQL</application> variables will be
-      substituted into the query just as for commands that return no result,
-      and the plan is cached in the same way.  Also, the special variable
-      <literal>FOUND</literal> is set to true if the query produced at
-      least one row, or false if it produced no rows (see
-      <xref linkend="plpgsql-statements-diagnostics">).
-     </para>
-     <note>      <para>
!       One might expect that writing <command>SELECT</command> directly
!       would accomplish this result, but at
!       present the only accepted way to do it is
!       <command>PERFORM</command>.  A SQL command that can return rows,
!       such as <command>SELECT</command>, will be rejected as an error
!       unless it has an <literal>INTO</> clause as discussed in the
!       next section.      </para>     </note>
     <para>      An example: <programlisting>
! PERFORM create_mv('cs_session_page_requests_mv', my_query); </programlisting>     </para>    </sect2>
--- 925,944 ----      <xref linkend="plpgsql-plan-caching">.     </para>
     <note>      <para>
!       In older versions of PostgreSQL, it was mandatory to use
!       <command>PERFORM</command> instead of <command>SELECT</command>
!       when the query could return data that was not captured into
!       variables.  This requirement has been relaxed and usage of
!       <command>PERFORM</command> has been deprecated.      </para>     </note>
     <para>      An example: <programlisting>
! SELECT create_mv('cs_session_page_requests_mv', my_query); </programlisting>     </para>    </sect2>
*************** GET DIAGNOSTICS integer_var = ROW_COUNT;
*** 1480,1491 ****           </listitem>           <listitem>            <para>
!             A <command>PERFORM</> statement sets <literal>FOUND</literal>             true if it produces (and
discards)one or more rows, false if             no row is produced.            </para>           </listitem>
<listitem>           <para>             <command>UPDATE</>, <command>INSERT</>, and <command>DELETE</>
statementsset <literal>FOUND</literal> true if at least one
 
--- 1452,1471 ----           </listitem>           <listitem>            <para>
!             A <command>SELECT</> statement sets <literal>FOUND</literal>             true if it produces (and
discards)one or more rows, false if             no row is produced.            </para>           </listitem>
<listitem>
+            <para>
+             A <command>PERFORM</> statement sets <literal>FOUND</literal>
+             true if it produces (and discards) one or more rows, false if
+             no row is produced.  This statement is equivalent to
+             <command>SELECT</> without INTO.
+            </para>
+           </listitem>
+           <listitem>            <para>             <command>UPDATE</>, <command>INSERT</>, and <command>DELETE</>
       statements set <literal>FOUND</literal> true if at least one
 
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
new file mode 100644
index b63ecac..975e8fe
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** exec_stmt_assign(PLpgSQL_execstate *esta
*** 1557,1562 ****
--- 1557,1563 ----  * exec_stmt_perform      Evaluate query and discard result (but set  *
FOUNDdepending on whether at least one row  *                            was returned).
 
+  *                            This syntax is deprecated.  * ----------  */ static int
*************** exec_stmt_execsql(PLpgSQL_execstate *est
*** 3647,3658 ****   }   else   {
!       /* If the statement returned a tuple table, complain */       if (SPI_tuptable != NULL)
!           ereport(ERROR,
!                   (errcode(ERRCODE_SYNTAX_ERROR),
!                    errmsg("query has no destination for result data"),
!                    (rc == SPI_OK_SELECT) ? errhint("If you want to
discard the results of a SELECT, use PERFORM instead.") : 0));   }
   return PLPGSQL_RC_OK;
--- 3648,3656 ----   }   else   {
!       /* If the statement returned a tuple table without INTO, free it. */       if (SPI_tuptable != NULL)
!           SPI_freetuptable(SPI_tuptable);   }
   return PLPGSQL_RC_OK;




merlin

On 3/22/16 8:37 AM, Merlin Moncure wrote:
>> I afraid of useless and forgotten call of functions. But the risk is same
>> >like PERFORM - so this is valid from one half. The PERFORM statement holds
>> >special semantic, and it is interesting.
> I see your point here, but the cost of doing that far outweighs the
> risks.  And I don't think the arbitrary standard of defining forcing
> the user to identify if the query should return data is a good way of
> identifying dead code.

Not to mention that there's tons of other ways to introduce unintended 
inefficiencies. Off the top of my head, declaring variables that are 
never referenced and have no assignment is a big one.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Wed, Mar 23, 2016 at 10:57 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> On 3/22/16 8:37 AM, Merlin Moncure wrote:
>>>
>>> I afraid of useless and forgotten call of functions. But the risk is same
>>> >like PERFORM - so this is valid from one half. The PERFORM statement
>>> > holds
>>> >special semantic, and it is interesting.
>>
>> I see your point here, but the cost of doing that far outweighs the
>> risks.  And I don't think the arbitrary standard of defining forcing
>> the user to identify if the query should return data is a good way of
>> identifying dead code.
>
> Not to mention that there's tons of other ways to introduce unintended
> inefficiencies. Off the top of my head, declaring variables that are never
> referenced and have no assignment is a big one.

Yes. This comes up most often with dblink for me.   Mainly because of
the slight wonkiness of dblink API, but totally agree this should
never have to be done.  Anyways, I submitted the patch to the next
open commit fest.

Totally off topic aside: are you in dallas for the next PUG? I
unfortunately missed the last one and will likely miss the next one
too -- I coach the company kickball team and we play on Wednesdays --
oh well.  Maybe beers afterwards?

merlin

On Sun, Apr 10, 2016 at 3:13 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Hi
>
> 2016-03-21 22:13 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:
>>
>> Hi
>>
>> 2016-03-21 21:24 GMT+01:00 Merlin Moncure <mmoncure@gmail.com>:
>>>
>>> Patch is trivial (see below), discussion is not :-).
>>>
>>> I see no useful reason to require INTO when returning data with
>>> SELECT.  However, requiring queries to indicate not needing data via
>>> PERFORM causes some annoyances:
>>>
>>> *) converting routines back and forth between pl/pgsql and pl/sql
>>> requires needless busywork and tends to cause errors to be thrown at
>>> runtime
>>>
>>> *) as much as possible, (keywords begin/end remain a problem),
>>> pl/pgsql should be a superset of sql
>>>
>>> *) it's much more likely to be burned by accidentally forgetting to
>>> swap in PERFORM than to accidentally leave in a statement with no
>>> actionable target.  Even if you did so in the latter case, it stands
>>> to reason you'd accidentally leave in the target variable, too.
>>>
>>> *) the PERFORM requirement hails from the days when only statements
>>> starting with SELECT return data.  There is no PERFORM equivalent for
>>> WITH/INSERT/DELETE/UPDATE and there are real world scenarios where you
>>> might have a RETURNING clause that does something but not necessarily
>>> want to place the result in a variable (for example passing to
>>> volatile function).  Take a look at the errhint() clause below -- we
>>> don't even have a suggestion in that case.
>>>
>>> This has come up before, and there was a fair amount of sympathy for
>>> this argument albeit with some dissent -- notably Pavel.  I'd like to
>>> get a hearing on the issue -- thanks.  If we decide to move forward,
>>> this would effectively deprecate PERFORM and the documentation will be
>>> suitably modified as well.
>>
>>
>
> here is another argument why this idea is not good.
>
>
http://stackoverflow.com/questions/36509511/error-query-has-no-destination-for-result-data-when-writing-pl-pgsql-function
>
> Now, when people coming from T-SQL world use some T-SQL constructs, then usually the code should not work with the
error"query has not destination for data ... "
 
>
> When PLpgSQL will be more tolerant, then their code will be executed without any error, but will not work.

I don't think it's a problem requiring people to use RETURN in order
to return data from the function.

SQL functions BTW happily discard results and it's never been an issue
there FWICT.  To address your other argument given below, there are
valid cases where you'd use RETURNING without having any interest in
capturing the set.  For example, you might have a volatile function,
v_func() that does something and returns a value that may not be
essential to the caller (say, a count of rows adjusted).

INSERT INTO foo ...
RETURNING v_func(foo.x);

Scenarios (even if not very common) where dummy variables are required
and/or queries have to be written into more complex forms (say, into a
CTE) where you would not have to do so outside pl/pgsql greatly
outweigh your points that, 'users might do the wrong thing'.  The
wrong thing is actually the right thing in some cases.

Small aside here: One thing that t-sql did right and pl/sql did wrong
was to make the language a proper superset of sql.  pl/pgsql's
hijacking INTO, BEGIN, END, and EXECUTE are really unfortunate as are
any behaviors that are incompatible with the regular language (like
requiring PERFORM); they fork the language and make building stored
procedures in pl/pgsql much more difficult if not impossible.  I'm not
sure this is a really solvable problem, but at least it can be nibbled
at.

What are the rules for pl/psm?

merlin

Anyways, here's the patch with documentation adjustments as promised.
I ended up keeping the 'without result' section because it contained
useful information about plan caching,

merlin

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index 9786242..512eaa7
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*************** my_record.user_id := 20;
*** 904,910 ****
      <title>Executing a Command With No Result</title>

      <para>
!      For any SQL command that does not return rows, for example
       <command>INSERT</> without a <literal>RETURNING</> clause, you can
       execute the command within a <application>PL/pgSQL</application> function
       just by writing the command.
--- 904,910 ----
      <title>Executing a Command With No Result</title>

      <para>
!      For any SQL command, for example
       <command>INSERT</> without a <literal>RETURNING</> clause, you can
       execute the command within a <application>PL/pgSQL</application> function
       just by writing the command.
*************** my_record.user_id := 20;

*** 925,972 ****
       <xref linkend="plpgsql-plan-caching">.
      </para>

-     <para>
-      Sometimes it is useful to evaluate an expression or <command>SELECT</>
-      query but discard the result, for example when calling a function
-      that has side-effects but no useful result value.  To do
-      this in <application>PL/pgSQL</application>, use the
-      <command>PERFORM</command> statement:
-
- <synopsis>
- PERFORM <replaceable>query</replaceable>;
- </synopsis>
-
-      This executes <replaceable>query</replaceable> and discards the
-      result.  Write the <replaceable>query</replaceable> the same
-      way you would write an SQL <command>SELECT</> command, but replace the
-      initial keyword <command>SELECT</> with <command>PERFORM</command>.
-      For <command>WITH</> queries, use <command>PERFORM</> and then
-      place the query in parentheses.  (In this case, the query can only
-      return one row.)
-      <application>PL/pgSQL</application> variables will be
-      substituted into the query just as for commands that return no result,
-      and the plan is cached in the same way.  Also, the special variable
-      <literal>FOUND</literal> is set to true if the query produced at
-      least one row, or false if it produced no rows (see
-      <xref linkend="plpgsql-statements-diagnostics">).
-     </para>
-
      <note>
       <para>
!       One might expect that writing <command>SELECT</command> directly
!       would accomplish this result, but at
!       present the only accepted way to do it is
!       <command>PERFORM</command>.  A SQL command that can return rows,
!       such as <command>SELECT</command>, will be rejected as an error
!       unless it has an <literal>INTO</> clause as discussed in the
!       next section.
       </para>
      </note>

      <para>
       An example:
  <programlisting>
! PERFORM create_mv('cs_session_page_requests_mv', my_query);
  </programlisting>
      </para>
     </sect2>
--- 925,944 ----
       <xref linkend="plpgsql-plan-caching">.
      </para>

      <note>
       <para>
!       In older versions of PostgreSQL, it was mandatory to use
!       <command>PERFORM</command> instead of <command>SELECT</command>
!       when the query could return data that was not captured into
!       variables.  This requirement has been relaxed and usage of
!       <command>PERFORM</command> has been deprecated.
       </para>
      </note>

      <para>
       An example:
  <programlisting>
! SELECT create_mv('cs_session_page_requests_mv', my_query);

  </programlisting>
      </para>
     </sect2>
*************** GET DIAGNOSTICS integer_var = ROW_COUNT;
*** 1480,1491 ****
            </listitem>
            <listitem>
             <para>
!             A <command>PERFORM</> statement sets <literal>FOUND</literal>
              true if it produces (and discards) one or more rows, false if
              no row is produced.
             </para>
            </listitem>
            <listitem>
             <para>
              <command>UPDATE</>, <command>INSERT</>, and <command>DELETE</>
              statements set <literal>FOUND</literal> true if at least one
--- 1452,1471 ----
            </listitem>
            <listitem>
             <para>
!             A <command>SELECT</> statement sets <literal>FOUND</literal>
              true if it produces (and discards) one or more rows, false if
              no row is produced.
             </para>

            </listitem>
            <listitem>
+            <para>
+             A <command>PERFORM</> statement sets <literal>FOUND</literal>
+             true if it produces (and discards) one or more rows, false if
+             no row is produced.  This statement is equivalent to
+             <command>SELECT</> without INTO.
+            </para>
+           </listitem>
+           <listitem>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index 9786242..512eaa7

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
new file mode 100644
index b63ecac..975e8fe
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** exec_stmt_assign(PLpgSQL_execstate *esta
*** 1557,1562 ****
--- 1557,1563 ----
   * exec_stmt_perform      Evaluate query and discard result (but set
   *                            FOUND depending on whether at least one row
   *                            was returned).
+  *                            This syntax is deprecated.
   * ----------
   */
  static int
*************** exec_stmt_execsql(PLpgSQL_execstate *est
*** 3647,3658 ****
    }
    else
    {
!       /* If the statement returned a tuple table, complain */
        if (SPI_tuptable != NULL)
!           ereport(ERROR,
!                   (errcode(ERRCODE_SYNTAX_ERROR),
!                    errmsg("query has no destination for result data"),
!                    (rc == SPI_OK_SELECT) ? errhint("If you want to
discard the results of a SELECT, use PERFORM instead.") : 0));
    }

    return PLPGSQL_RC_OK;
--- 3648,3656 ----
    }
    else
    {
!       /* If the statement returned a tuple table without INTO, free it. */
        if (SPI_tuptable != NULL)
!           SPI_freetuptable(SPI_tuptable);
    }

    return PLPGSQL_RC_OK;

On Sat, Jun 4, 2016 at 10:45 PM, David G. Johnston
<david.g.johnston@gmail.com> wrote:
> On Tuesday, March 22, 2016, Merlin Moncure <mmoncure@gmail.com> wrote:
>>
>>
>> Anyways, here's the patch with documentation adjustments as promised.
>> I ended up keeping the 'without result' section because it contained
>> useful information about plan caching,
>>
>> merlin
>>
>> diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
>> new file mode 100644
>> index 9786242..512eaa7
>> *** a/doc/src/sgml/plpgsql.sgml
>> --- b/doc/src/sgml/plpgsql.sgml
>> *************** my_record.user_id := 20;
>> *** 904,910 ****
>>       <title>Executing a Command With No Result</title>
>>
>>       <para>
>> !      For any SQL command that does not return rows, for example
>>        <command>INSERT</> without a <literal>RETURNING</> clause, you can
>>        execute the command within a <application>PL/pgSQL</application>
>> function
>>        just by writing the command.
>> --- 904,910 ----
>>       <title>Executing a Command With No Result</title>
>>
>>       <para>
>> !      For any SQL command, for example
>>        <command>INSERT</> without a <literal>RETURNING</> clause, you can
>>        execute the command within a <application>PL/pgSQL</application>
>> function
>>        just by writing the command.
>> *************** my_record.user_id := 20;
>
>
> This just seems odd...a bit broader approach here would be nice.  Especially
> since now it's not the command that doesn't have a result but the user
> decision to not capture any result that may be present.  Using insert
> returning for the example here is just, again, odd...
>
> Removing PERFORM requires a bit more reframing of the docs than you've done
> here; too much was influenced by its presence.
>
>>
>> *** 925,972 ****
>>        <xref linkend="plpgsql-plan-caching">.
>>       </para>
>>
>> -     <para>
>> -      Sometimes it is useful to evaluate an expression or
>> <command>SELECT</>
>> -      query but discard the result, for example when calling a function
>> -      that has side-effects but no useful result value.  To do
>> -      this in <application>PL/pgSQL</application>, use the
>> -      <command>PERFORM</command> statement:
>> -
>> - <synopsis>
>> - PERFORM <replaceable>query</replaceable>;
>> - </synopsis>
>> -
>> -      This executes <replaceable>query</replaceable> and discards the
>> -      result.  Write the <replaceable>query</replaceable> the same
>> -      way you would write an SQL <command>SELECT</> command, but replace
>> the
>> -      initial keyword <command>SELECT</> with <command>PERFORM</command>.
>> -      For <command>WITH</> queries, use <command>PERFORM</> and then
>> -      place the query in parentheses.  (In this case, the query can only
>> -      return one row.)
>> -      <application>PL/pgSQL</application> variables will be
>> -      substituted into the query just as for commands that return no
>> result,
>> -      and the plan is cached in the same way.  Also, the special variable
>> -      <literal>FOUND</literal> is set to true if the query produced at
>> -      least one row, or false if it produced no rows (see
>> -      <xref linkend="plpgsql-statements-diagnostics">).
>> -     </para>
>> -
>>       <note>
>>        <para>
>> !       One might expect that writing <command>SELECT</command> directly
>> !       would accomplish this result, but at
>> !       present the only accepted way to do it is
>> !       <command>PERFORM</command>.  A SQL command that can return rows,
>> !       such as <command>SELECT</command>, will be rejected as an error
>> !       unless it has an <literal>INTO</> clause as discussed in the
>> !       next section.
>>        </para>
>>       </note>
>>
>>       <para>
>>        An example:
>>   <programlisting>
>> ! PERFORM create_mv('cs_session_page_requests_mv', my_query);
>>   </programlisting>
>>       </para>
>>      </sect2>
>> --- 925,944 ----
>>        <xref linkend="plpgsql-plan-caching">.
>>       </para>
>>
>>       <note>
>>        <para>
>> !       In older versions of PostgreSQL, it was mandatory to use
>> !       <command>PERFORM</command> instead of <command>SELECT</command>
>> !       when the query could return data that was not captured into
>> !       variables.  This requirement has been relaxed and usage of
>> !       <command>PERFORM</command> has been deprecated.
>>        </para>
>>       </note>
>>
>>       <para>
>>        An example:
>>   <programlisting>
>> ! SELECT create_mv('cs_session_page_requests_mv', my_query);
>
>
> I'd consider making the note into a paragraph and then saying that the
> select form and perform form are equivalent by writing both out one after
> the other.  The note brings emphasis that is not necessary if we want to
> de-emphasize perform.
>
>>   </programlisting>
>>       </para>
>>      </sect2>
>> *************** GET DIAGNOSTICS integer_var = ROW_COUNT;
>> *** 1480,1491 ****
>>             </listitem>
>>             <listitem>
>>              <para>
>> !             A <command>PERFORM</> statement sets
>> <literal>FOUND</literal>
>>               true if it produces (and discards) one or more rows, false
>> if
>>               no row is produced.
>>              </para>
>>             </listitem>
>>             <listitem>
>>              <para>
>>               <command>UPDATE</>, <command>INSERT</>, and
>> <command>DELETE</>
>>               statements set <literal>FOUND</literal> true if at least one
>> --- 1452,1471 ----
>>             </listitem>
>>             <listitem>
>>              <para>
>> !             A <command>SELECT</> statement sets <literal>FOUND</literal>
>>               true if it produces (and discards) one or more rows, false
>> if
>>               no row is produced.
>>              </para>
>
>
> This could be worded better. It will return true regardless of whether the
> result is discarded.
>
>>
>>             </listitem>
>>             <listitem>
>> +            <para>
>> +             A <command>PERFORM</> statement sets
>> <literal>FOUND</literal>
>> +             true if it produces (and discards) one or more rows, false
>> if
>> +             no row is produced.  This statement is equivalent to
>> +             <command>SELECT</> without INTO.
>> +            </para>
>> +           </listitem>
>> +           <listitem>
>> diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
>> new file mode 100644
>> index 9786242..512eaa7
>
>
> Duplicate (x2)? copy-paste elided
>
>>
>> diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
>> new file mode 100644
>> index b63ecac..975e8fe
>> *** a/src/pl/plpgsql/src/pl_exec.c
>> --- b/src/pl/plpgsql/src/pl_exec.c
>> *************** exec_stmt_assign(PLpgSQL_execstate *esta
>> *** 1557,1562 ****
>> --- 1557,1563 ----
>>    * exec_stmt_perform      Evaluate query and discard result (but set
>>    *                            FOUND depending on whether at least one
>> row
>>    *                            was returned).
>> +  *                            This syntax is deprecated.
>>    * ----------
>>    */
>>   static int
>> *************** exec_stmt_execsql(PLpgSQL_execstate *est
>> *** 3647,3658 ****
>>     }
>>     else
>>     {
>> !       /* If the statement returned a tuple table, complain */
>>         if (SPI_tuptable != NULL)
>> !           ereport(ERROR,
>> !                   (errcode(ERRCODE_SYNTAX_ERROR),
>> !                    errmsg("query has no destination for result data"),
>> !                    (rc == SPI_OK_SELECT) ? errhint("If you want to
>> discard the results of a SELECT, use PERFORM instead.") : 0));
>>     }
>>
>>     return PLPGSQL_RC_OK;
>> --- 3648,3656 ----
>>     }
>>     else
>>     {
>> !       /* If the statement returned a tuple table without INTO, free it.
>> */
>>         if (SPI_tuptable != NULL)
>> !           SPI_freetuptable(SPI_tuptable);
>>     }
>>
>>     return PLPGSQL_RC_OK;
>>
>
> It should include at least one new test.
>
> The discussion talked about insert/returning behavior with respect to into.
> Not necessarily for this patch related but how does that fit in?
>
> +1 from Jim, Merlin, Robert, Tom and myself.  I think the docs need work -
> and the code looks ok though lacks at least one required test.
>
> -1 from Pavel
>
> Peter E. - you haven't commented but are on this as a reviewer...
>
> Merlin, back in your court for polishing.
>
> Adding myself as reviewer but leaving it in needs review for the moment.

Thanks for the review.  All your comments look pretty reasonable. I'll
touch it up and resubmit.

merlin

On 6/6/16 9:59 AM, Merlin Moncure wrote:
> Thanks for the review.  All your comments look pretty reasonable. I'll
> touch it up and resubmit.

This is the last email in this thread that the commit fest app shows.  I
think we are waiting on an updated patch, with docs and tests.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 9/9/16 2:57 PM, Peter Eisentraut wrote:
> On 6/6/16 9:59 AM, Merlin Moncure wrote:
>> Thanks for the review.  All your comments look pretty reasonable. I'll
>> touch it up and resubmit.
> 
> This is the last email in this thread that the commit fest app shows.  I
> think we are waiting on an updated patch, with docs and tests.

I'm setting this patch to returned with feedback now.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services