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

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.

David J.