Re: Fwd: patch: format function - fixed oid

---------- Forwarded message ----------
From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Date: 2010/11/18
Subject: Re: patch: format function, next generation
To: Jeff Janes <jeff(dot)janes(at)gmail(dot)com>
Kopie: pgsql-hackers-owner(at)postgresql(dot)org

Hello

somebody takes my oid :)

updated patch is in attachment

Regards

Pavel Stehule

2010/11/18 Jeff Janes <jeff(dot)janes(at)gmail(dot)com>:
>>
>> Hello
>>
>> I reworked a implementation of format function. This respects last discussion:
>>
>> * support a positional placeholders - syntax %99$x,
>> * support a tags: %s, I, L,
>> * enhanced documentation,
>> * enhanced reggress tests
>>
>> Regards
>>
>> Pavel Stehule
>
> Dear Pavel,
>
> Your patch no longer passes "make check" against git HEAD.
>
> It looks like the backend sync commit has claimed OID 3063
>
> creating directory
> /home/jjanes/postgres/git/src/test/regress/./tmp_check/data ... ok
> creating subdirectories ... ok
> selecting default max_connections ... 100
> selecting default shared_buffers ... 32MB
> creating configuration files ... ok
> creating template1 database in
> /home/jjanes/postgres/git/src/test/regress/./tmp_check/data/base/1 ...
> FATAL:  could not create unique index "pg_proc_oid_index"
> DETAIL:  Key (oid)=(3063) is duplicated.
> child process exited with exit code 1
> initdb: data directory
> "/home/jjanes/postgres/git/src/test/regress/./tmp_check/data" not
> removed at user's request
>
> Cheers,
>
> Jeff
>

On Thu, Nov 18, 2010 at 11:54 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> ---------- Forwarded message ----------
> From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> Date: 2010/11/18
> Subject: Re: patch: format function, next generation
> To: Jeff Janes <jeff(dot)janes(at)gmail(dot)com>
> Kopie: pgsql-hackers-owner(at)postgresql(dot)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

Hello

2010/11/20 Jeff Janes <jeff(dot)janes(at)gmail(dot)com>:
> On Thu, Nov 18, 2010 at 11:54 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> ---------- Forwarded message ----------
>> From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> Date: 2010/11/18
>> Subject: Re: patch: format function, next generation
>> To: Jeff Janes <jeff(dot)janes(at)gmail(dot)com>
>> Kopie: pgsql-hackers-owner(at)postgresql(dot)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".
>

I am not native speaker, so I invite any correction in documentation -
anything what I wrote is more/less just skelet for somebody, whu can
speak better than me. I am not against to note - so this function is
similar to sprintf function, but then should be specified - so this
function is different - designed to request PL languages and
environment. I have not a problem with replacing "tags" by "conversion
or positional specifiers". Somewhere is used term placeholder??

> "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.
>

I am for it

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

probably output functions are described in some hacker parts
http://www.postgresql.org/docs/9.0/interactive/xtypes.html

>
> 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>
>

+1

>
> 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.
>

I think so you are a correct. Who will a submit this final version? You or me?

Thank you very much

Regards

Pavel Stehule

>
> Thanks,
>
> Jeff
>

On Sat, Nov 20, 2010 at 2:59 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> I think so you are a correct. Who will a submit this final version? You or me?

I've got it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2010/11/20 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sat, Nov 20, 2010 at 2:59 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> I think so you are a correct. Who will a submit this final version? You or me?
>
> I've got it.
>

Thank you

nice a day

Pavel

> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

On Sat, Nov 20, 2010 at 7:29 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2010/11/20 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sat, Nov 20, 2010 at 2:59 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> I think so you are a correct. Who will a submit this final version? You or me?
>>
>> I've got it.
>>
>
> Thank you
>
> nice a day

OK, I've committed this, after a fairly heavy rewrite.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2010/11/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sat, Nov 20, 2010 at 7:29 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> 2010/11/20 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Sat, Nov 20, 2010 at 2:59 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>>> I think so you are a correct. Who will a submit this final version? You or me?
>>>
>>> I've got it.
>>>
>>
>> Thank you
>>
>> nice a day
>
> OK, I've committed this, after a fairly heavy rewrite.

thank you very much

regards

Pavel Stehule
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

On Nov 21, 2010, at 1:09 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>
>> OK, I've committed this, after a fairly heavy rewrite.
>
> thank you very much

Ah, nuts. I forgot to bump catversion.

...Robert