Re: Relax requirement for INTO with SELECT in pl/pgsql

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