Thread: [REVIEW] Re: Re: BUG #9578: Undocumented behaviour for temp tables created inside query language (SQL) functions

(Cc: to pgsql-bugs dropped.)

At 2014-03-17 18:24:55 +1100, kommi.haribabu@gmail.com wrote:
>
> *** a/doc/src/sgml/xfunc.sgml
> --- b/doc/src/sgml/xfunc.sgml
> ***************
> *** 153,159 **** SELECT clean_emp();
> --- 153,186 ----
>       (<literal>\</>) (assuming escape string syntax) in the body of
>       the function (see <xref linkend="sql-syntax-strings">).
>      </para>
> +    
> +    <sect2 id="xfunc-sql-function-parsing-mechanism">
> +     <title>Parsing mechanism of a function</title>
>   
> +    <indexterm>
> +     <primary>function</primary>
> +     <secondary>parsing mechanism</secondary>
> +    </indexterm>

I suggest "Catalog changes within functions" instead of the above title.

> +     <para>
> +      The body of an SQL function is parsed as if it were a single - multi-part
> +      statement and thus uses a constant snapshot of the system catalogs for
> +      every sub-statement therein. Commands that alter the catalog will likely not
> +      work as expected.
> +     </para>
> + 
> +     <para>  
> +      For example: Issuing "CREATE TEMP TABLE" within an SQL function will add
> +      the table to the catalog but subsequent statements in the same function will
> +      not see those additions and thus the temporary table will be invisible to them.
> +     </para>
> + 
> +     <para>  
> +      Thus it is generally advised that <application>PL/pgSQL</> be used, instead of
> +      <acronym>SQL</acronym>, when any catalog visibilities are required in the same function.
> +     </para>
> +    </sect2>

I don't think that much text is warranted. I suggest something like the
following condensed wording:
   <para>    The body of an SQL function is parsed as if it were a single    multi-part statement, using a constant
snapshotof the system    catalogs. The effect of any commands that alter the catalogs    (e.g. "CREATE TEMP TABLE")
willtherefore not be visible to    subsequent commands in the function body.   </para>
 
   <para>    The recommended workaround is to use <application>PL/PgSQL</>.   </para>

Does that seem sensible to you?

-- Abhijit

On Tue, Jun 17, 2014 at 12:30 AM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
> (Cc: to pgsql-bugs dropped.)
>
> At 2014-03-17 18:24:55 +1100, kommi.haribabu@gmail.com wrote:
>>
>> *** a/doc/src/sgml/xfunc.sgml
>> --- b/doc/src/sgml/xfunc.sgml
>> ***************
>> *** 153,159 **** SELECT clean_emp();
>> --- 153,186 ----
>>       (<literal>\</>) (assuming escape string syntax) in the body of
>>       the function (see <xref linkend="sql-syntax-strings">).
>>      </para>
>> +
>> +    <sect2 id="xfunc-sql-function-parsing-mechanism">
>> +     <title>Parsing mechanism of a function</title>
>>
>> +    <indexterm>
>> +     <primary>function</primary>
>> +     <secondary>parsing mechanism</secondary>
>> +    </indexterm>
>
> I suggest "Catalog changes within functions" instead of the above title.
>
>> +     <para>
>> +      The body of an SQL function is parsed as if it were a single - multi-part
>> +      statement and thus uses a constant snapshot of the system catalogs for
>> +      every sub-statement therein. Commands that alter the catalog will likely not
>> +      work as expected.
>> +     </para>
>> +
>> +     <para>
>> +      For example: Issuing "CREATE TEMP TABLE" within an SQL function will add
>> +      the table to the catalog but subsequent statements in the same function will
>> +      not see those additions and thus the temporary table will be invisible to them.
>> +     </para>
>> +
>> +     <para>
>> +      Thus it is generally advised that <application>PL/pgSQL</> be used, instead of
>> +      <acronym>SQL</acronym>, when any catalog visibilities are required in the same function.
>> +     </para>
>> +    </sect2>
>
> I don't think that much text is warranted. I suggest something like the
> following condensed wording:
>
>     <para>
>      The body of an SQL function is parsed as if it were a single
>      multi-part statement, using a constant snapshot of the system
>      catalogs. The effect of any commands that alter the catalogs
>      (e.g. "CREATE TEMP TABLE") will therefore not be visible to
>      subsequent commands in the function body.
>     </para>
>
>     <para>
>      The recommended workaround is to use <application>PL/PgSQL</>.
>     </para>
>
> Does that seem sensible to you?

Looks good, Thanks for the review.
Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia

Thanks, I've marked this ready for committer.

-- Abhijit

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> Updated patch attached.

I revised this a bit more and committed it.
        regards, tom lane