Re: Fwd: patch: format function - fixed oid

On Thu, Nov 18, 2010 at 11:54 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> ---------- Forwarded message ----------
> From: Pavel Stehule <pavel.stehule@gmail.com>
> Date: 2010/11/18
> Subject: Re: patch: format function, next generation
> To: Jeff Janes <jeff.janes@gmail.com>
> Kopie: pgsql-hackers-owner@postgresql.org
>
>
> Hello
>
> somebody takes my oid :)
>
> updated patch is in attachment
>
> Regards
>
> Pavel Stehule
>

Dear Pavel and Hackers,

I've reviewed this patch.  It applied, makes, and passes make check.
It has added regression tests that seem appropriate.  I think the
feature added matches the consensus that emerged from the very long
email discussion.  The C code seems fine (to my meager abilities to
judge that).

But I think the documentation does need some work.  From func.sgml:

        This functions can be used to create a formated string or
message. There are allowed        three types of tags: %s as string, %I as SQL identifiers and
%L as SQL literals. Attention:        result for %I and %L must not be same as result of
<function>quote_ident</function> and        <function>quote_literal</function> functions, because this
function doesn't try to coerce        parameters to <type>text</type> type and directly use a
type's output functions. The placeholder        can be related to some explicit parameter with using a
optional n$ specification inside format.

Should we make it explicit that this is inspired by C's sprintf?  Do
we want to call them "tags"?  This is introducing what seems to be a
new word to describe what are usually (I think) called "conversion
specifiers".

"Must not be the same"  should be "Might not be the same".   However,
it does not appear that quote_ident is willing to use coercion at all,
and the %L behavior is more comparable to quote_nullable.

Maybe:

This function can be used to create a formatted string suitable for
use as dynamic
SQL or as a message.  There are three types of conversion specifiers:
%s for literal strings, %I
for SQL identifiers, and %L for SQL literals.  Note that the results
of the %L conversion
might not be the same as the results of the
<function>quote_nullable</function> function, as
the latter coerces its argument to <type>text</type> while
<function>format</function>
uses a type's output function.  A conversion can reference an explicit
parameter position
by using an optional "n$" in the format specification.

Does "type's output function" need to cross-reference someplace?
coercion is described elsewhere in this section of docs, but output
functions are not.


And for the changes to plpgsql.sgml, I would propose:
   <para>    Building a string for dynamic SQL statement can be simplified    by using the <function>format</function>
function(see <xref    linkend="functions-string">):
 
<programlisting>
EXECUTE format('UPDATE tbl SET %I = %L WHERE key = %L', colname,
newvalue, keyvalue);
</programlisting>   The <function>format</function> format can be used together with    the <literal>USING</literal>
clause:
<programlisting>
EXECUTE format('UPDATE tbl SET %I = $1 WHERE key = $2', colname)  USING newvalue, keyvalue;
</programlisting>    This form is more efficient because the parameters    <literal>newvalue</literal> and
<literal>keyvalue</literal>are
 
not converted to text.   </para>


These are mostly grammatical changes, but with the last three lines I
may have missed the meaning of what you originally intended--I'm not
sure on that.


Thanks,

Jeff