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

On Sat, Jun 4, 2016 at 10:45 PM, David G. Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
> On Tuesday, March 22, 2016, Merlin Moncure <mmoncure(at)gmail(dot)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