Re: patch (for 9.1) string functions

Hello

this patch contains a string formatting function "format"

postgres=# select format('some message: % (current user: %)',
current_date, current_user);
format
------------------------------------------------
some message: 2010-03-09 (current user: pavel)
(1 row)

this patch add new contrib module string functions - contains mainly
sprintf function:

postgres=# select sprintf('some message: %10s (%10s)', current_date,
current_user);
sprintf
---------------------------------------
some message: 2010-03-09 ( pavel)
(1 row)

postgres=# select sprintf('some message: %10s (%-10s)', current_date,
current_user);
sprintf
---------------------------------------
some message: 2010-03-09 (pavel )
(1 row)

some string variadic functions

postgres=# select concat('ahaha',10,null,current_date, true);
concat
------------------------
ahaha,10,,2010-03-09,t
(1 row)

postgres=# select concat_sql('ahaha',10,null,current_date, true);
concat_sql
--------------------------------
'ahaha',10,NULL,'2010-03-09',t
(1 row)

postgres=# select concat_json('ahaha'::text,10,null,current_date, true);
concat_json
-----------------------------------
"ahaha",10,null,"2010-03-09",true
(1 row)

and some basic text function rvrs, left, right.

Regards
Pavel Stehule

On Mar 9, 2010, at 6:30 AM, Pavel Stehule wrote:

> this patch contains a string formatting function "format"
>
> postgres=# select format('some message: % (current user: %)',
> current_date, current_user);
> format
> ------------------------------------------------
> some message: 2010-03-09 (current user: pavel)
> (1 row)
>
> this patch add new contrib module string functions - contains mainly
> sprintf function:

Seems useful. Add it to the CommitFest so it doesn't get lost?

https://commitfest.postgresql.org/action/commitfest_view?id=6

Best,

David

2010/3/9 David E. Wheeler <david(at)kineticode(dot)com>:
> On Mar 9, 2010, at 6:30 AM, Pavel Stehule wrote:
>
>> this patch contains a string formatting function "format"
>>
>> postgres=# select format('some message: % (current user: %)',
>> current_date, current_user);
>>                     format
>> ------------------------------------------------
>> some message: 2010-03-09 (current user: pavel)
>> (1 row)
>>
>> this patch add new contrib module string functions - contains mainly
>> sprintf function:
>
> Seems useful. Add it to the CommitFest so it doesn't get lost?
>
>  https://commitfest.postgresql.org/action/commitfest_view?id=6

https://commitfest.postgresql.org/action/patch_view?id=287

it is there

Pavel

>
> Best,
>
> David
>

Pavel Stehule wrote:
> Hello
>
> this patch contains a string formatting function "format"
>
Hi Pavel,

This is supercool. Haven't tried it out yet but surely will, thanks!

Yeb Havinga

On Tue, Mar 9, 2010 at 9:30 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> postgres=# select concat('ahaha',10,null,current_date, true);
>         concat
> ------------------------
>  ahaha,10,,2010-03-09,t

why are there commas in the output?

merlin

2010/3/9 Merlin Moncure <mmoncure(at)gmail(dot)com>:
> On Tue, Mar 9, 2010 at 9:30 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> postgres=# select concat('ahaha',10,null,current_date, true);
>>         concat
>> ------------------------
>>  ahaha,10,,2010-03-09,t
>
> why are there commas in the output?
>

I though so comma can be default separator - but I see - it is wrong -
separator have to be empty string.

Pavel

> merlin
>

updated version, concat function doesn't use separator

Pavel

2010/3/9 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> 2010/3/9 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>> On Tue, Mar 9, 2010 at 9:30 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> postgres=# select concat('ahaha',10,null,current_date, true);
>>>         concat
>>> ------------------------
>>>  ahaha,10,,2010-03-09,t
>>
>> why are there commas in the output?
>>
>
> I though so comma can be default separator - but I see - it is wrong -
> separator have to be empty string.
>
> Pavel
>
>
>> merlin
>>
>

On Tue, Mar 9, 2010 at 1:45 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> updated version, concat function doesn't use separator

btw...very cool stuff. I took a brief look at the sprintf
implementation. The main switch:
switch (pdesc.field_type)

It looks like format codes we choose not to support (like %p) are
silently ignored. Is this the correct behavior?

merlin

2010/3/9 Merlin Moncure <mmoncure(at)gmail(dot)com>:
> On Tue, Mar 9, 2010 at 1:45 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> updated version, concat function doesn't use separator
>
> btw...very cool stuff.  I took a brief look at the sprintf
> implementation.  The main switch:
> switch (pdesc.field_type)
>
> It looks like format codes we choose not to support (like %p) are
> silently ignored.  Is this the correct behavior?

it could be enhanced. sprintf is little bit baroque function and I am
not able to take all details. Please, add comment with proposals for
change.

Pavel

>
> merlin
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> updated version, concat function doesn't use separator

BTW, didn't you forget stringfunc.sql.in for contrib/stringfunc ?
So, I have not check stringfunc module yet.

I reviewed your patch, and format() in the core is almost ok. It's very cool!
On the other hand, contrib/stringfunc tries to implement safe-sprintf. It's
very complex, and I have questions about multi-byte character handling in it.

* How to print NULL value.
format() function prints NULL as "NULL", but RAISE statement in PL/pgSQL
does as "<NULL>". Do we need the same result for them?

postgres=# SELECT format('% vs %', 'NULL', NULL);
format
--------------
NULL vs NULL
(1 row)

postgres=# DO $$ BEGIN RAISE NOTICE '% vs %', 'NULL', NULL; END; $$;
NOTICE: NULL vs <NULL>
DO

* Error messages: "too few/many parameters"
For the same reason, "too few/many parameters specified for format()"
might be better for the messages.

For RAISE in PL/pgSQL:
ERROR: too few parameters specified for RAISE
ERROR: too many parameters specified for RAISE

* Why do you need convert multi-byte characters to wide char?
Length specifier in stringfunc_sprintf() means "character length".
But is pg_encoding_mbcliplen() enough for the purpose?

* Character-length vs. disp-length in length specifier for sprintf()
For example, '%10s' for sprintf() means "10 characters" in the code.
But there might be usages to format text values for display. In such
case, display length might be better for the length specifier.
How about having both "s" and "S"?
"%10s" -- 10 characters
"%10S" -- 10 disp length; we could use pg_dsplen() for the purpse.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

Hello

2010/7/8 Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>:
>
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
>> updated version, concat function doesn't use separator
>
> BTW, didn't you forget stringfunc.sql.in for contrib/stringfunc ?
> So, I have not check stringfunc module yet.

sorry, attached fixed patch
>
> I reviewed your patch, and format() in the core is almost ok. It's very cool!
> On the other hand, contrib/stringfunc tries to implement safe-sprintf. It's
> very complex, and I have questions about multi-byte character handling in it.
>

I use same mechanism as RAISE statement does. And it working for mer

postgres=# select sprintf('žlutý%dkůň',10);
sprintf
------------
žlutý10kůň
(1 row)

Time: 0,647 ms

postgres=# select sprintf('%s žlutý kůň','příliš');
sprintf
------------------
příliš žlutý kůň
(1 row)

Time: 11,017 ms
postgres=# select sprintf('%10s žlutý kůň','příliš');
sprintf
----------------------
příliš žlutý kůň
(1 row)

Time: 0,439 ms

> * How to print NULL value.
> format() function prints NULL as "NULL", but RAISE statement in PL/pgSQL
> does as "<NULL>". Do we need the same result for them?

I prefer just NULL. You can add "<" and ">" simple if you want. But
removing is little bit dificult.

postgres=# select sprintf('%s', coalesce(NULL, '<NULL>'));
sprintf
---------
<NULL>
(1 row)

maybe some GUC variable

stringfunc.null_string = '<NULL>' in future??

>
>    postgres=# SELECT format('% vs %', 'NULL', NULL);
>        format
>    --------------
>     NULL vs NULL
>    (1 row)
>
>    postgres=# DO $$ BEGIN RAISE NOTICE '% vs %', 'NULL', NULL; END; $$;
>    NOTICE:  NULL vs <NULL>
>    DO
>
> * Error messages: "too few/many parameters"
>  For the same reason, "too few/many parameters specified for format()"
>  might be better for the messages.
>
>  For RAISE in PL/pgSQL:
>    ERROR:  too few parameters specified for RAISE
>    ERROR:  too many parameters specified for RAISE

ook, I agree

>
> * Why do you need convert multi-byte characters to wide char?
> Length specifier in stringfunc_sprintf() means "character length".
> But is pg_encoding_mbcliplen() enough for the purpose?
>

No, I need it. I use a swprintf function - for output of formated
strings - and there are not some sprintf function for multibyte chars
:(. Without this function I don't need a multibyte->widechars
conversion, but sprintf function will be much more larger and complex.

> * Character-length vs. disp-length in length specifier for sprintf()
> For example, '%10s' for sprintf() means "10 characters" in the code.
> But there might be usages to format text values for display. In such
> case, display length might be better for the length specifier.
> How about having both "s" and "S"?
>    "%10s" -- 10 characters
>    "%10S" -- 10 disp length; we could use pg_dsplen() for the purpse.

it is independent, because I use swprintf function

postgres=# select length(sprintf('%5s', 'ščř'));
length
--------
5
(1 row)

Time: 45,485 ms
postgres=# select length(sprintf('%5s', 'abc'));
length
--------
5
(1 row)

Time: 0,499 ms

so it is equal to using a pg_dsplen()

probably original one byte behave have sense for bytea data type. But
I am not sure if we would to complicate this function for binary data.

>
> Regards,

Thank you very much for review

Pavel Stehule
> ---
> Takahiro Itagaki
> NTT Open Source Software Center
>
>
>

2010/7/8 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> sorry, attached fixed patch

Make installcheck for contrib/stringfunc is broken.
Please run regression test with --enable-cassert build.
test stringfunc ... TRAP:
FailedAssertion("!(!lc_ctype_is_c())", File: "mbutils.c", Line: 715)
LOG: server process (PID 15121) was terminated by signal 6: Aborted

This patch contains several functions.
- format(fmt text, VARIADIC args "any")
- sprintf(fmt text, VARIADIC args "any")
- concat(VARIADIC args "any")
- concat_ws(separator text, VARIADIC args "any")
- concat_json(VARIADIC args "any")
- concat_sql(VARIADIC args "any")
- rvrs(str text)
- left(str text, n int)
- right(str text, n int)

The first one is in the core, and others are in contrib/stringfunc.
But I think almost
all of them should be in the core, because users want to write portable SQLs.
Contrib modules are not always available. Note that concat() is
supported by Oracle,
MySQL, and DB2. Also left() and right() are supported by MySQL, DB2,
and SQL Server.

Functions that depend on GUC settings should be marked as VOLATILE
instead of IMMUTABLE. I think format(), sprintf(), and all of
concat()s should be
volatile because at least timestamp depends on datestyle parameter.

concat_ws() and rvrs() should be renamed to non-abbreviated forms.
How about concat_with_sep() and reverse() ?

I think we should avoid concat_json() at the moment because there is another
development project for JSON support. The result type will be JOIN type rather
than text then.

I'm not sure usefulness of concat_sql(). Why don't you just quote all values
with quotes and separate them with comma?

>> format() function prints NULL as "NULL", but RAISE statement in PL/pgSQL
>> does as "<NULL>".
> I prefer just NULL.
> maybe some GUC variable
> stringfunc.null_string = '<NULL>' in future??

We have some choices for NULL representation. For example, empty string,
NULL, <NULL>, or (null) . What will be our choice? Each of them looks
equally reasonable for me. GUC idea is also good because we need to
mark format() as VOLATILE anyway. We have nothing to lose.

---
Takahiro Itagaki

hello

2010/7/9 Takahiro Itagaki <itagaki(dot)takahiro(at)gmail(dot)com>:
> 2010/7/8 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> sorry, attached fixed patch
>
> Make installcheck for contrib/stringfunc is broken.
> Please run regression test with --enable-cassert build.
>  test stringfunc           ... TRAP:
> FailedAssertion("!(!lc_ctype_is_c())", File: "mbutils.c", Line: 715)
>  LOG:  server process (PID 15121) was terminated by signal 6: Aborted
>

> This patch contains several functions.
> - format(fmt text, VARIADIC args "any")
> - sprintf(fmt text, VARIADIC args "any")
> - concat(VARIADIC args "any")
> - concat_ws(separator text, VARIADIC args "any")
> - concat_json(VARIADIC args "any")
> - concat_sql(VARIADIC args "any")
> - rvrs(str text)
> - left(str text, n int)
> - right(str text, n int)
>
> The first one is in the core, and others are in contrib/stringfunc.
> But I think almost
> all of them should be in the core, because users want to write portable SQLs.
> Contrib modules are not always available.  Note that concat() is
> supported by Oracle,
> MySQL, and DB2. Also left() and right() are supported by MySQL, DB2,
> and SQL Server.
>
> Functions that depend on GUC settings should be marked as VOLATILE
> instead of IMMUTABLE. I think format(), sprintf(), and all of
> concat()s should be
> volatile because at least timestamp depends on datestyle parameter.
>

ok, I'll fix it

> concat_ws() and rvrs() should be renamed to non-abbreviated forms.
> How about concat_with_sep() and reverse() ?
>

I used a well known names - concat_ws (MySQL) and rvrs (Oracle rdbms),
I like concat_ws - concat_with_sep is maybe too long. rvrs is too
short, so I'll rename it to reverse - ok?

> I think we should avoid concat_json() at the moment because there is another
> development project for JSON support. The result type will be JOIN type rather
> than text then.
>

ok

> I'm not sure usefulness of concat_sql(). Why don't you just quote all values
> with quotes and separate them with comma?
>

concat_xxx functions are helpers to serialisation. So when when you
would to generate INSERT statements for some export, and you cannot
use a COPY statement, you can do

FOR r IN
SELECT ....
LOOP
RETURN NEXT 'INSERT INTO tab(..) VALUES (' || concat_sql(r.a, r.b, r.c, ... )
END LOOP;
RETURN;

you don't need to solve anything and output is well formated SQL. Some
databases dislike quoted numeric values - and quoted nums can be
sonfusing

>>> format() function prints NULL as "NULL", but RAISE statement in PL/pgSQL
>>> does as "<NULL>".
>> I prefer just NULL.
>> maybe some GUC variable
>> stringfunc.null_string = '<NULL>' in future??
>
> We have some choices for NULL representation. For example, empty string,
> NULL, <NULL>, or (null) . What will be our choice?   Each of them looks
> equally reasonable for me. GUC idea is also good because we need to
> mark format() as VOLATILE anyway. We have nothing to lose.
>

Can ve to solve it other patch? I know to aversion core hackers to new
GUC. Now I propose just "NULL". The GUC for NULL representation has
bigger consequences - probably have to related to RAISE statement, and
to proposed functions to_string, to_array.

> ---
> Takahiro Itagaki
>

Thank You very much, I'do fix it

Pavel

Hello

I am sending a actualised patch

* removed concat_json
* renamed function rvsr to reverse
* functions format, sprintf and concat* are stable now (as to_char for example)

2010/7/9 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> hello
>
> 2010/7/9 Takahiro Itagaki <itagaki(dot)takahiro(at)gmail(dot)com>:
>> 2010/7/8 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>>> sorry, attached fixed patch
>>
>> Make installcheck for contrib/stringfunc is broken.
>> Please run regression test with --enable-cassert build.
>>  test stringfunc           ... TRAP:
>> FailedAssertion("!(!lc_ctype_is_c())", File: "mbutils.c", Line: 715)
>>  LOG:  server process (PID 15121) was terminated by signal 6: Aborted

it worked on my station :( - Fedora 64bit

can you send a backtrace, please

Regards

Pavel Stehule

>>
>
>
>> This patch contains several functions.
>> - format(fmt text, VARIADIC args "any")
>> - sprintf(fmt text, VARIADIC args "any")
>> - concat(VARIADIC args "any")
>> - concat_ws(separator text, VARIADIC args "any")
>> - concat_json(VARIADIC args "any")
>> - concat_sql(VARIADIC args "any")
>> - rvrs(str text)
>> - left(str text, n int)
>> - right(str text, n int)
>>
>> The first one is in the core, and others are in contrib/stringfunc.
>> But I think almost
>> all of them should be in the core, because users want to write portable SQLs.
>> Contrib modules are not always available.  Note that concat() is
>> supported by Oracle,
>> MySQL, and DB2. Also left() and right() are supported by MySQL, DB2,
>> and SQL Server.
>>
>> Functions that depend on GUC settings should be marked as VOLATILE
>> instead of IMMUTABLE. I think format(), sprintf(), and all of
>> concat()s should be
>> volatile because at least timestamp depends on datestyle parameter.
>>
>
> ok, I'll fix it
>
>> concat_ws() and rvrs() should be renamed to non-abbreviated forms.
>> How about concat_with_sep() and reverse() ?
>>
>
> I used a well known names - concat_ws (MySQL) and rvrs (Oracle rdbms),
> I like concat_ws - concat_with_sep is maybe too long. rvrs is too
> short, so I'll rename it to reverse - ok?
>
>> I think we should avoid concat_json() at the moment because there is another
>> development project for JSON support. The result type will be JOIN type rather
>> than text then.
>>
>
> ok
>
>> I'm not sure usefulness of concat_sql(). Why don't you just quote all values
>> with quotes and separate them with comma?
>>
>
> concat_xxx functions are helpers to serialisation. So when when you
> would to generate INSERT statements for some export, and you cannot
> use a COPY statement, you can do
>
> FOR r IN
>  SELECT ....
> LOOP
>  RETURN NEXT 'INSERT INTO tab(..) VALUES (' || concat_sql(r.a, r.b, r.c, ... )
> END LOOP;
> RETURN;
>
> you don't need to solve anything and output is well formated SQL. Some
> databases dislike quoted numeric values - and quoted nums can be
> sonfusing
>
>
>>>> format() function prints NULL as "NULL", but RAISE statement in PL/pgSQL
>>>> does as "<NULL>".
>>> I prefer just NULL.
>>> maybe some GUC variable
>>> stringfunc.null_string = '<NULL>' in future??
>>
>> We have some choices for NULL representation. For example, empty string,
>> NULL, <NULL>, or (null) . What will be our choice?   Each of them looks
>> equally reasonable for me. GUC idea is also good because we need to
>> mark format() as VOLATILE anyway. We have nothing to lose.
>>
>
> Can ve to solve it other patch? I know to aversion core hackers to new
> GUC. Now I propose just "NULL". The GUC for NULL representation has
> bigger consequences - probably have to related to RAISE statement, and
> to proposed functions to_string, to_array.
>
>> ---
>> Takahiro Itagaki
>>
>
> Thank You very much, I'do fix it
>
> Pavel
>

2010/7/9 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> I am sending a actualised patch
> * removed concat_json
> * renamed function rvsr to reverse
> * functions format, sprintf and concat* are stable now (as to_char for example)

I'd like to move all proposed functions into the core, and not to add
contrib/stringfunc.
I think those functions are very useful and worth adding in core.
* concat(), concat_ws(), reverse(), left() and right() are ready to commit.
* format() is almost ready, except consensus of NULL representation.
* sprintf() is also useful, but we cannot use swprintf() in it because
there are many problems in converting to wide chars. We should
develop mbchar-aware version of %s formatter.
* IMHO, concat_sql() has very limited use cases. Boolean and numeric
values are not quoted, but still need product-specific conversions because
some DBs prefer 1/0 instead of true/false.
Also, dblink_build_sql_insert() provides similar functionality. Will
we have both?

> it worked on my station :( - Fedora 64bit
Still failed :-( I used UTF8 database with *locale=C* on 64bit Linux.
char2wchar() doesn't seem to work on C locale. We should avoid
using the function and converting mb chars to wide chars.

select sprintf('>>>%10s %10d<<<', 'hello', 10);
! server closed the connection unexpectedly
TRAP: FailedAssertion("!(!lc_ctype_is_c())", File: "mbutils.c", Line: 715)

#0 0x00000038c0c332f5 in raise () from /lib64/libc.so.6
#1 0x00000038c0c34b20 in abort () from /lib64/libc.so.6
#2 0x00000000006e951d in ExceptionalCondition (conditionName=<value
optimized out>, errorType=<value optimized out>, fileName=<value
optimized out>,
lineNumber=<value optimized out>) at assert.c:57
#3 0x00000000006fa8bf in char2wchar (to=0x1daf188 L"", tolen=16,
from=0x1da95b0 "%*s", fromlen=3) at mbutils.c:715
#4 0x00007f8e8c436d37 in stringfunc_sprintf (fcinfo=0x7fff9bdcd4b0)
at stringfunc.c:463

--
Itagaki Takahiro

On Sun, Jul 11, 2010 at 10:30 PM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> 2010/7/9 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> I am sending a actualised patch
>> * removed concat_json
>> * renamed function rvsr to reverse
>> * functions format, sprintf and concat* are stable now (as to_char for example)
>
> I'd like to move all proposed functions into the core, and not to add
> contrib/stringfunc.
> I think those functions are very useful and worth adding in core.
> * concat(), concat_ws(), reverse(), left() and right() are ready to commit.
> * format() is almost ready, except consensus of NULL representation.
> * sprintf() is also useful, but we cannot use swprintf() in it because
>  there are many problems in converting to wide chars. We should
>  develop mbchar-aware version of %s formatter.
> * IMHO, concat_sql() has very limited use cases. Boolean  and numeric
>  values are not quoted, but still need product-specific conversions because
>  some DBs prefer 1/0 instead of true/false.
>  Also, dblink_build_sql_insert() provides similar functionality. Will
> we have both?

I'm all in favor of putting such things in core as are supported by
multiple competing products, but is that really true for all of these?

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

2010/7/12 Robert Haas <robertmhaas(at)gmail(dot)com>:
> I'm all in favor of putting such things in core as are supported by
> multiple competing products, but is that really true for all of these?

- concat() : MySQL, Oracle, DB2
- concat_ws() : MySQL,
- left(), right() : MySQL, SQL Server, DB2
- reverse() : MySQL, SQL Server, Oracle (as utl_raw.reverse)

concat_sql(), format(), and sprintf() will be our unique features.

--
Itagaki Takahiro

Hello

2010/7/12 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Jul 11, 2010 at 10:30 PM, Itagaki Takahiro
> <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
>> 2010/7/9 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>>> I am sending a actualised patch
>>> * removed concat_json
>>> * renamed function rvsr to reverse
>>> * functions format, sprintf and concat* are stable now (as to_char for example)
>>
>> I'd like to move all proposed functions into the core, and not to add
>> contrib/stringfunc.
>> I think those functions are very useful and worth adding in core.
>> * concat(), concat_ws(), reverse(), left() and right() are ready to commit.
>> * format() is almost ready, except consensus of NULL representation.

what solution for this moment - be a consistent with RAISE statement ???

>> * sprintf() is also useful, but we cannot use swprintf() in it because
>>  there are many problems in converting to wide chars. We should
>>  develop mbchar-aware version of %s formatter.

ook I'll work on this - but there is same problem with NULL like a
format function

>> * IMHO, concat_sql() has very limited use cases. Boolean  and numeric
>>  values are not quoted, but still need product-specific conversions because
>>  some DBs prefer 1/0 instead of true/false.
>>  Also, dblink_build_sql_insert() provides similar functionality. Will
>> we have both?
>

I can remove it - when I checked it I found so it doesn't well
serialize PostgreSQL specific types as array or record, so I am not
against to remove it now.

> I'm all in favor of putting such things in core as are supported by
> multiple competing products, but is that really true for all of these?
>

I have not a strong opinion on this - I would to like see reverse and
format in core. But I think, so contrib is enought. Can somebody from
commiters to decide it, please? Any sprintf implemenation needs lots
of code - minimally for this function I prefer contrib for this
function.

Regards

Pavel Stehule

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

On Jul 11, 2010, at 10:32 PM, Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> 2010/7/12 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> I'm all in favor of putting such things in core as are supported by
>> multiple competing products, but is that really true for all of these?
>
> - concat() : MySQL, Oracle, DB2
> - concat_ws() : MySQL,
> - left(), right() : MySQL, SQL Server, DB2
> - reverse() : MySQL, SQL Server, Oracle (as utl_raw.reverse)
>
> concat_sql(), format(), and sprintf() will be our unique features.

I think concat, left, right, and reverse should go into core, and perhaps format also. The rest sound like contrib material to me.

...Robert

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:

> I'd like to move all proposed functions into the core, and not to
> add contrib/stringfunc.

> Still failed :-( I used UTF8 database with *locale=C* on 64bit
> Linux.
> char2wchar() doesn't seem to work on C locale. We should avoid
> using the function and converting mb chars to wide chars.
>
> select sprintf('>>>%10s %10d<<<', 'hello', 10);
> ! server closed the connection unexpectedly
> TRAP: FailedAssertion("!(!lc_ctype_is_c())", File: "mbutils.c",
> Line: 715)
>
> #0 0x00000038c0c332f5 in raise () from /lib64/libc.so.6
> #1 0x00000038c0c34b20 in abort () from /lib64/libc.so.6
> #2 0x00000000006e951d in ExceptionalCondition
> (conditionName=<value optimized out>, errorType=<value optimized
> out>, fileName=<value optimized out>, lineNumber=<value optimized
> out>) at assert.c:57
> #3 0x00000000006fa8bf in char2wchar (to=0x1daf188 L"", tolen=16,
> from=0x1da95b0 "%*s", fromlen=3) at mbutils.c:715
> #4 0x00007f8e8c436d37 in stringfunc_sprintf
> (fcinfo=0x7fff9bdcd4b0)
> at stringfunc.c:463

Based on this and subsequent posts, I've changed this patch's status
to "Waiting on Author".

-Kevin

2010/7/12 Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov>:
> Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
>
>> I'd like to move all proposed functions into the core, and not to
>> add contrib/stringfunc.
>
>> Still failed :-(  I used UTF8 database with *locale=C* on 64bit
>> Linux.
>> char2wchar() doesn't seem to work on C locale. We should avoid
>> using the function and converting mb chars to wide chars.
>>
>>   select sprintf('>>>%10s %10d<<<', 'hello', 10);
>> ! server closed the connection unexpectedly
>> TRAP: FailedAssertion("!(!lc_ctype_is_c())", File: "mbutils.c",
>> Line: 715)
>>
>> #0  0x00000038c0c332f5 in raise () from /lib64/libc.so.6
>> #1  0x00000038c0c34b20 in abort () from /lib64/libc.so.6
>> #2  0x00000000006e951d in ExceptionalCondition
>> (conditionName=<value optimized out>, errorType=<value optimized
>> out>, fileName=<value optimized out>, lineNumber=<value optimized
>> out>) at assert.c:57
>> #3  0x00000000006fa8bf in char2wchar (to=0x1daf188 L"", tolen=16,
>> from=0x1da95b0 "%*s", fromlen=3) at mbutils.c:715
>> #4  0x00007f8e8c436d37 in stringfunc_sprintf
>> (fcinfo=0x7fff9bdcd4b0)
>> at stringfunc.c:463
>
> Based on this and subsequent posts, I've changed this patch's status
> to "Waiting on Author".

ook - I'll send actualised version tomorrow

Pavel

>
> -Kevin
>

Hello

so this is actualised patch:

* concat_sql removed
* left, right, reverse and concat are in core
* printf and concat_ws are in contrib
* format show "<NULL>" as NULL string
* removed an using of wide chars

todo:

NULL handling for printf function

Query:
what is corect result for

* printf(">>%3.2d<<", NULL) ??
* printf(">>%3.2s", NULL) ??

Regards

Pavel Stehule

2010/7/12 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> 2010/7/12 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> I'm all in favor of putting such things in core as are supported by
>> multiple competing products, but is that really true for all of these?
>
> - concat() : MySQL, Oracle, DB2
> - concat_ws() : MySQL,
> - left(), right() : MySQL, SQL Server, DB2
> - reverse() : MySQL, SQL Server, Oracle (as utl_raw.reverse)
>
> concat_sql(), format(), and sprintf() will be our unique features.
>
> --
> Itagaki Takahiro
>

2010/7/13 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> so this is actualised patch:
> * concat_sql removed
> * left, right, reverse and concat are in core
> * printf and concat_ws are in contrib
> * format show "<NULL>" as NULL string
> * removed an using of wide chars

I think function codes in the core (concat, format, left, right,
and reverse) are ready for committers. They also have docs, but
the names are not listed in Index page (bookindex.html).
Please add
<indexterm>
<primary>funcname</primary>
</indexterm>
in func.sgml for each new function.

However, I have a couple of comments to stringfunc module. sprintf()
and concat_ws() are not installed by default, but provided by the module.

> todo:
> NULL handling for printf function

I like <NULL> for null arguments. It is just same as format() and RAISE.

=== Questions ===
* concat_ws() transforms NULLs into empty strings.
Is it an intended behavior and compatible with MySQL?
Note that string_agg() doesn't add separators to NULLs.

=# SELECT coalesce(concat_ws(',', 'A', NULL, 'B'), '(null)');
coalesce
----------
A,,B
(1 row)

* concat_ws() returns NULL when the separator is NULL.
Is it an intended behavior and compatible with MySQL?

=# SELECT coalesce(concat_ws(NULL, 'A', NULL, 'B'), '(null)');
coalesce
----------
(null)
(1 row)

=== Trivial issues ===
* Some function prototypes are declared but not used.
We can just remove them.
- mb_string_info()
- stringfunc_concat(PG_FUNCTION_ARGS);
- stringfunc_left(PG_FUNCTION_ARGS);
- stringfunc_right(PG_FUNCTION_ARGS);
- stringfunc_reverse(PG_FUNCTION_ARGS);

* Some error messages need to be improved.
For example, "1th" is wrong.
=# select sprintf('>>>%*s<<<', NULL, 'abcdef');
ERROR: null value not allowed
HINT: width (1th) arguments is NULL

* sprintf() has some typos in error messages
For example, "sprinf".

--
Itagaki Takahiro

Hello

2010/7/13 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> 2010/7/13 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> so this is actualised patch:
>> * concat_sql removed
>> * left, right, reverse and concat are in core
>> * printf and concat_ws are in contrib
>> * format show "<NULL>" as NULL string
>> * removed an using of wide chars
>
> I think function codes in the core (concat, format, left, right,
> and reverse) are ready for committers. They also have docs, but
> the names are not listed in Index page (bookindex.html).
> Please add
>   <indexterm>
>    <primary>funcname</primary>
>   </indexterm>
> in func.sgml for each new function.
>

fixed
> However, I have a couple of comments to stringfunc module. sprintf()
> and concat_ws() are not installed by default, but provided by the module.
>
>> todo:
>> NULL handling for printf function
>
> I like <NULL> for null arguments. It is just same as format() and RAISE.

done

>
> === Questions ===
> * concat_ws() transforms NULLs into empty strings.
> Is it an intended behavior and compatible with MySQL?
> Note that string_agg() doesn't add separators to NULLs.
>

no I was wrong - original concat_ws just ignore NULL - fixed, now
concat_ws has same behave like original.

>  =# SELECT coalesce(concat_ws(',', 'A', NULL, 'B'), '(null)');
>   coalesce
>  ----------
>   A,,B
>  (1 row)
>
> * concat_ws() returns NULL when the separator is NULL.
> Is it an intended behavior and compatible with MySQL?
>
>  =# SELECT coalesce(concat_ws(NULL, 'A', NULL, 'B'), '(null)');
>   coalesce
>  ----------
>   (null)
>  (1 row)
>
> === Trivial issues ===
> * Some function prototypes are declared but not used.
>  We can just remove them.
>  - mb_string_info()
>  - stringfunc_concat(PG_FUNCTION_ARGS);
>  - stringfunc_left(PG_FUNCTION_ARGS);
>  - stringfunc_right(PG_FUNCTION_ARGS);
>  - stringfunc_reverse(PG_FUNCTION_ARGS);
>
> * Some error messages need to be improved.
>  For example, "1th" is wrong.
>    =# select sprintf('>>>%*s<<<', NULL, 'abcdef');
>    ERROR:  null value not allowed
>    HINT:  width (1th) arguments is NULL

have you a some idea about it?

>
> * sprintf() has some typos in error messages
>  For example, "sprinf".
>

fixed

> --
> Itagaki Takahiro
>

Regards

Pavel

Hello

I have a one idea nonstandard enhancing of sprintf - relatie often job
is a quoting in PostgreSQL. So sprintf should have a special formats
for quoted values. What do you think about

%lq ... literal quoted
%iq ... ident quoted

??

Regards

Pavel

2010/7/13 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> 2010/7/13 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
>> 2010/7/13 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>>> so this is actualised patch:
>>> * concat_sql removed
>>> * left, right, reverse and concat are in core
>>> * printf and concat_ws are in contrib
>>> * format show "<NULL>" as NULL string
>>> * removed an using of wide chars
>>
>> I think function codes in the core (concat, format, left, right,
>> and reverse) are ready for committers. They also have docs, but
>> the names are not listed in Index page (bookindex.html).
>> Please add
>>   <indexterm>
>>    <primary>funcname</primary>
>>   </indexterm>
>> in func.sgml for each new function.
>>
>
> fixed
>> However, I have a couple of comments to stringfunc module. sprintf()
>> and concat_ws() are not installed by default, but provided by the module.
>>
>>> todo:
>>> NULL handling for printf function
>>
>> I like <NULL> for null arguments. It is just same as format() and RAISE.
>
> done
>
>>
>> === Questions ===
>> * concat_ws() transforms NULLs into empty strings.
>> Is it an intended behavior and compatible with MySQL?
>> Note that string_agg() doesn't add separators to NULLs.
>>
>
> no I was  wrong - original concat_ws just ignore NULL - fixed, now
> concat_ws has same behave like original.
>
>>  =# SELECT coalesce(concat_ws(',', 'A', NULL, 'B'), '(null)');
>>   coalesce
>>  ----------
>>   A,,B
>>  (1 row)
>>
>> * concat_ws() returns NULL when the separator is NULL.
>> Is it an intended behavior and compatible with MySQL?
>>
>>  =# SELECT coalesce(concat_ws(NULL, 'A', NULL, 'B'), '(null)');
>>   coalesce
>>  ----------
>>   (null)
>>  (1 row)
>>
>> === Trivial issues ===
>> * Some function prototypes are declared but not used.
>>  We can just remove them.
>>  - mb_string_info()
>>  - stringfunc_concat(PG_FUNCTION_ARGS);
>>  - stringfunc_left(PG_FUNCTION_ARGS);
>>  - stringfunc_right(PG_FUNCTION_ARGS);
>>  - stringfunc_reverse(PG_FUNCTION_ARGS);
>>
>> * Some error messages need to be improved.
>>  For example, "1th" is wrong.
>>    =# select sprintf('>>>%*s<<<', NULL, 'abcdef');
>>    ERROR:  null value not allowed
>>    HINT:  width (1th) arguments is NULL
>
> have you a some idea about it?
>
>>
>> * sprintf() has some typos in error messages
>>  For example, "sprinf".
>>
>
> fixed
>
>> --
>> Itagaki Takahiro
>>
>
> Regards
>
> Pavel
>

I reviewed the core changes of the patch. I don't think we need
mb_string_info() at all. Instead, we can just call pg_mbxxx() functions.

I rewrote the patch to use pg_mbstrlen_with_len() and pg_mbcharcliplen().
What do you think the changes? It requires re-counting lengths of multi-byte
strings in some cases, but the code will be much simpler and can avoid
allocating length buffers.

I'd like to apply contrib/stringinfo apart from the core changes,
because there seems to be still some idea to improve sprintf().

--
Itagaki Takahiro

2010/7/21 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> I reviewed the core changes of the patch. I don't think we need
> mb_string_info() at all. Instead, we can just call pg_mbxxx() functions.
>
> I rewrote the patch to use pg_mbstrlen_with_len() and pg_mbcharcliplen().
> What do you think the changes? It requires re-counting lengths of multi-byte
> strings in some cases, but the code will be much simpler and can avoid
> allocating length buffers.
>

It is a good idea. I see a problem only for "right" function, where
for most common use case a mblen will be called two times. I am not
able to say now, if this can be a performance issue or not. Highly
probably not - only for very large strings.

postgres=# create or replace function randomstr(int) returns text as
$$select string_agg(substring('abcdefghijklmnop' from
trunc(random()*13)::int+1 for 1),'') from generate_series(1,$1) $$
language sql;
CREATE FUNCTION
Time: 27,452 ms

postgres=# select count(*) from(select right(randomstr(1000),3) from
generate_series(1,10000))x;
count
-------
10000
(1 row)

Time: 5615,061 ms
postgres=# select count(*) from(select right(randomstr(1000),3) from
generate_series(1,10000))x;
count
-------
10000
(1 row)

Time: 5606,937 ms
postgres=# select count(*) from(select right(randomstr(1000),3) from
generate_series(1,10000))x;
count
-------
10000
(1 row)

Time: 5630,771 ms

postgres=# select count(*) from(select right(randomstr(1000),3) from
generate_series(1,10000))x;
count
-------
10000
(1 row)

Time: 5753,063 ms
postgres=# select count(*) from(select right(randomstr(1000),3) from
generate_series(1,10000))x;
count
-------
10000
(1 row)
Time: 5755,776 ms

It is about 2% slower for UTF8 encoding. So it isn't significant for me.

I agree with your changes. Thank You very much

Regards

Pavel Stehule

> I'd like to apply contrib/stringinfo apart from the core changes,
> because there seems to be still some idea to improve sprintf().
>
> --
> Itagaki Takahiro
>

2010/7/21 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> It is about 2% slower for UTF8 encoding. So it isn't significant for me.
> I agree with your changes. Thank You very much

Thanks. The core-part is almost ready to commit.
I'll continue to review the contrib part.

But I found there is a design issue in format() :
Appending a '%' is common use-case, but format() cannot append
% char without any spaces between placeholder and the raw % char.

itagaki=# SELECT format('%%%', 10), format('% %%', 10);
format | format
--------+--------
%10 | 10 %
(1 row)

It is a design issue, and RAISE in PL/pgSQL has the same issue, too.
Do we accept the restriction? Or should we add another escape
syntax and/or placeholder pattern?

--
Itagaki Takahiro

2010/7/23 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> 2010/7/21 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> It is about 2% slower for UTF8 encoding. So it isn't significant for me.
>> I agree with your changes. Thank You very much
>
> Thanks. The core-part is almost ready to commit.
> I'll continue to review the contrib part.
>
> But I found there is a design issue in format() :
> Appending a '%' is common use-case, but format() cannot append
> % char without any spaces between placeholder and the raw % char.
>
> itagaki=# SELECT format('%%%', 10), format('% %%', 10);
>  format | format
> --------+--------
>  %10    | 10 %
> (1 row)
>
> It is a design issue, and RAISE in PL/pgSQL has the same issue, too.
> Do we accept the restriction? Or should we add another escape
> syntax and/or placeholder pattern?
>

I prefer a current behave - RAISE statement uses same and it is not
reported as bug for ten years, what I read a mailing lists. I would to
have a FORMAT implementation simple as possible.

and there is simple workaround:

postgres=# create or replace function fx()
returns void as $$
begin
raise notice '>>>%<<<', '%';
end;
$$ language plpgsql;
CREATE FUNCTION
Time: 560.063 ms
postgres=# select fx();
NOTICE: >>>%<<<
fx
────

(1 row)

Regards

Pavel Stehule

> --
> Itagaki Takahiro
>

I'm reviewing contrib part of the string functions patch.

I found an issue in sprintf() to print integer values. In this case,
'l' (for long type) is used on *all* platforms. For example,
SELECT sprintf('%d', 10);
internally uses
appendStringInfo('%ld', (int64) 10)

But there are some platform that requires to use %lld for int64 format, probably
on Windows. That's why we have INT64_FORMAT macro. sprintf() needs to be
adjusted to use INT64_FORMAT or similar portable codes.

Other portion of the patch seems to be OK for me,
unless you have still some idea to extend the feature.

2010/7/17 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> I have a one idea nonstandard enhancing of sprintf - relatie often job
> is a quoting in PostgreSQL. So sprintf should have a special formats
> for quoted values. What do you think about
>
> %lq ... literal quoted
> %iq ... ident quoted

They save some keyboard types to write quote_literal() and quote_ident(), right?
They seem to be useful and reasonable for me. One comment is that you might
want to print NULL values as "NULL" instead of "<NULL>" in such cases.

--
Itagaki Takahiro

Hello

2010/7/23 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> I'm reviewing contrib part of the string functions patch.
>
> I found an issue in sprintf() to print integer values. In this case,
> 'l' (for long type) is used on *all* platforms. For example,
>  SELECT sprintf('%d', 10);
> internally uses
>  appendStringInfo('%ld', (int64) 10)
>
> But there are some platform that requires to use %lld for int64 format, probably
> on Windows. That's why we have INT64_FORMAT macro. sprintf() needs to be
> adjusted to use INT64_FORMAT or similar portable codes.

ok, I'll look on it

>
> Other portion of the patch seems to be OK for me,
> unless you have still some idea to extend the feature.
>
> 2010/7/17 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> I have a one idea nonstandard enhancing of sprintf - relatie often job
>> is a quoting in PostgreSQL. So sprintf should have a special formats
>> for quoted values. What do you think about
>>
>> %lq ... literal quoted
>> %iq ... ident quoted
>
> They save some keyboard types to write quote_literal() and quote_ident(), right?
> They seem to be useful and reasonable for me. One comment is that you might
> want to print NULL values as "NULL" instead of "<NULL>" in such cases.
>

yes, it is good note

Thank You very much

Regards

Pavel Stehule

> --
> Itagaki Takahiro
>

Hello

2010/7/23 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> I'm reviewing contrib part of the string functions patch.
>
> I found an issue in sprintf() to print integer values. In this case,
> 'l' (for long type) is used on *all* platforms. For example,
>  SELECT sprintf('%d', 10);
> internally uses
>  appendStringInfo('%ld', (int64) 10)
>

> But there are some platform that requires to use %lld for int64 format, probably
> on Windows. That's why we have INT64_FORMAT macro. sprintf() needs to be
> adjusted to use INT64_FORMAT or similar portable codes.
>

fixed - it depends on INT64_FORMAT now.

> Other portion of the patch seems to be OK for me,
> unless you have still some idea to extend the feature.
>
> 2010/7/17 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> I have a one idea nonstandard enhancing of sprintf - relatie often job
>> is a quoting in PostgreSQL. So sprintf should have a special formats
>> for quoted values. What do you think about
>>
>> %lq ... literal quoted
>> %iq ... ident quoted
>
> They save some keyboard types to write quote_literal() and quote_ident(), right?
> They seem to be useful and reasonable for me. One comment is that you might
> want to print NULL values as "NULL" instead of "<NULL>" in such cases.
>

NULL is showed as NULL for literal quoting and when ident quoting is
used, then exception is raised.

Maybe last rule is too hard, but it should be a protection before SQL
injection via mal formated SQL

Regards

Pavel

> --
> Itagaki Takahiro
>

I merged and enhanced some part of your patch:
- contrib/stringfunc are merged in the core patch
- Old format() is replaced with sprintf(), but the function name is
still format().
- Support %q as alias for %iq.

2010/7/25 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> fixed - it depends on INT64_FORMAT now.
I modified the code a bit not to expect 'll' or 'l'.

> %lq ... literal quoted
> %iq ... ident quoted
I also modified 'q' without specifier, i.e, %q is handled as same as %lq.

>> But I found there is a design issue in format() :
> I prefer a current behave - RAISE statement uses same and it is not
> reported as bug for ten years

I think RAISE is badly designed. Using % as a placeholder has a limitation
to format strings. For example, format() cannot work as concat():
SELECT format('%%', 123, 456) => ERROR

So, my proposal is renaming stringfunc//sprintf() to format(),
and moving it into the core. I think sprintf() is superior to format()
in every aspect; '%s%s' works as concat(), and '%s%%' can append
% without blanks.

Then, concat_ws() will be moved into core because contrib/stringfunc
only has the function now. In addition, I'd like to include the function for
the compatibility to MySQL. Also, concat() and concat_ws() can share
the implementation.

Comments?

--
Itagaki Takahiro

2010/7/26 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> I merged and enhanced some part of your patch:
>  - contrib/stringfunc are merged in the core patch
>  - Old format() is replaced with sprintf(), but the function name is
> still format().
>  - Support %q as alias for %iq.
>
> 2010/7/25 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> fixed - it depends on INT64_FORMAT now.
> I modified the code a bit not to expect 'll' or 'l'.
>
>> %lq ... literal quoted
>> %iq ... ident quoted
> I also modified 'q' without specifier, i.e, %q is handled as same as %lq.
>
>>> But I found there is a design issue in format() :
>> I prefer a current behave - RAISE statement uses same and it is not
>> reported as bug for ten years
>
> I think RAISE is badly designed. Using % as a placeholder has a limitation
> to format strings. For example, format() cannot work as concat():
>  SELECT format('%%', 123, 456) => ERROR
>
> So, my proposal is renaming stringfunc//sprintf() to format(),
> and moving it into the core. I think sprintf() is superior to format()
> in every aspect; '%s%s' works as concat(), and '%s%%' can append
> % without blanks.
>

Sorry, I am strong against. Using a format just for string string
concation is bad idea - there are a concat function, there are ||
operator. Look on complexity of format/RAISE and sprintf. More - it
can be strange, when we have a "format" function and it is almost
"sprintf". I still prefer simplicity of format - you have a true - it
has a issue, but there are simply workaround

format('%', 123||345)

format is very simple - but usually you don't need to specify a with,
a precision.

sprintf has some issue based on common sprintf implementation and
expecting too. For example a precision is used very dynamically - it
has a different sense for integers and for floats, so I wouldn't have
a sprintf in core.

> Then, concat_ws() will be moved into core because contrib/stringfunc
> only has the function now. In addition, I'd like to include the function for
> the compatibility to MySQL. Also, concat() and concat_ws() can share
> the implementation.
>
> Comments?

I disagree - please, return to prev variant

Regards

Pavel Stehule
>
> --
> Itagaki Takahiro
>

2010/7/26 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> sprintf has some issue based on common sprintf implementation and
> expecting too. For example a precision is used very dynamically - it
> has a different sense for integers and for floats, so I wouldn't have
> a sprintf in core.

Why do we need to have similar functions in core and contrib?
It will just confuse users. If you want to RAISE-version of format(),
I don't want to have stringfunc in contrib.

sprintf() is cool! So, I'd like to use sprintf() by default rather than
format() which has limited features. Almost all users don't know
well about contrib modules. Books about functions in inter-databases
don't consider about postgres' contrib modules. That's why I want to
move the useful features into core rather than contrib modules.

--
Itagaki Takahiro

2010/7/26 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> 2010/7/26 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> sprintf has some issue based on common sprintf implementation and
>> expecting too. For example a precision is used very dynamically - it
>> has a different sense for integers and for floats, so I wouldn't have
>> a sprintf in core.
>
> Why do we need to have similar functions in core and contrib?
> It will just confuse users. If you want to RAISE-version of format(),
> I don't want to have stringfunc in contrib.
>

:(

please, look back to discus about this module. There was desided, so
"format" will be in core and "sprintf" in contrib. One reason for this
decision was complexity of printf's implementation.

> sprintf() is cool! So, I'd like to use sprintf() by default rather than
> format() which has limited features. Almost all users don't know
> well about contrib modules. Books about functions in inter-databases
> don't consider about postgres' contrib modules. That's why I want to
> move the useful features into core rather than contrib modules.
>

I have a different opinion and I am not alone. sprintf is good for c
language, but it is problematic in scripting environments, where are
not pointers, where we have more info about variables - where we can
use a introspection - it is like dinosaurus in IT. My implementation
is little bit simple, bacause it is use a buildin functionality - but
still it has more then hundred rows. The full implementation has about
thousand rows. More sprintf is little bit slower than format - it have
to do little bit more work - and it can be confusing for people who
doesn't known it well.

for example - sprintf("%d", 10.2) ---> 10.

next - sprintf respect common standard - but this standard doesn't
calculate with PostgreSQL datatypes - there are not support for
"date", "timestemp" for example.

Function format is designed to work with builtin function to_char.
This is simple and full functional combination - I have not a plan to
replace it.

Regards

Pavel Stehule

> --
> Itagaki Takahiro
>

On Sun, Jul 25, 2010 at 11:29 PM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> I think RAISE is badly designed. Using % as a placeholder has a limitation
> to format strings. For example, format() cannot work as concat():
>  SELECT format('%%', 123, 456) => ERROR

It's hard to argue with this, as far as it goes.

> So, my proposal is renaming stringfunc//sprintf() to format(),
> and moving it into the core. I think sprintf() is superior to format()
> in every aspect; '%s%s' works as concat(), and '%s%%' can append
> % without blanks.

So forget about format() and put sprintf() in contrib/stringfunc.
That's not an argument for putting anything in core. Perhaps such an
argument can be made, but this isn't it.

> Then, concat_ws() will be moved into core because contrib/stringfunc
> only has the function now. In addition, I'd like to include the function for
> the compatibility to MySQL. Also, concat() and concat_ws() can share
> the implementation.

Regardless of where this function ends up, the concat_ws documentation
should contain some mention of the fact that "ws" is intended to mean
"with separator", and that the naming is chosen for compatibility with
MySQL. As for where to put it, I see no terribly compelling reason
why it needs to be in core. You can write array_to_string(array[txt1,
txt2, txt3], sep) and get the same effect as concat_ws(sep, txt1,
txt2, txt3). I don't really want to start maintaining duplicate
functionality for things like this, especially since MySQL users will
no doubt expect that our implementation will be bug-compatible. If
the output isn't identical to what MySQL does for every set of
arguments, it'll be reported as a bug.

Come to think of it, have we checked that the behavior of LEFT, RIGHT,
REVERSE, etc. is the same on other DBs, especially as far as nulls,
empty strings, too-large or negative subscripts, etc is concerned? Is
CONCAT('foo', NULL) => 'foo' really the behavior that everyone else
implements here? And why does CONCAT() take a variadic "ANY"
argument? Shouldn't that be variadic TEXT?

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

On Mon, Jul 26, 2010 at 8:02 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Regardless of where this function ends up, the concat_ws documentation
> should contain some mention of the fact that "ws" is intended to mean
> "with separator",

big +1 on that -- I've been loosely following the thread and I had
assumed 'ws' meant 'wide string' all this time :-).

> Come to think of it, have we checked that the behavior of LEFT, RIGHT,
> REVERSE, etc. is the same on other DBs, especially as far as nulls,
> empty strings, too-large or negative subscripts, etc is concerned?

Probably 'standard' behavior wrt null would be to be strict; return
null if any argument is null. The proposed behavior seems ok though.

> CONCAT('foo', NULL) => 'foo' really the behavior that everyone else
> implements here?  And why does CONCAT() take a variadic "ANY"
> argument?  Shouldn't that be variadic TEXT?

What does that accomplish, besides forcing you to sprinkle every
concat call with text casts (maybe that's not a bad thing?)?

merlin

>
>> CONCAT('foo', NULL) => 'foo' really the behavior that everyone else
>> implements here?  And why does CONCAT() take a variadic "ANY"
>> argument?  Shouldn't that be variadic TEXT?
>

CONCAT with variadic text parameter will be limited with existing
default casts to text - for example, you can't to cast date to text,
int to text.

postgres=# create or replace function concat(variadic text[]) returns
text as $$select string_agg(x,'') from unnest($1) x$$ language sql;
CREATE FUNCTION

postgres=# select concat('a','b');
concat
--------
ab
(1 row)

Time: 20,812 ms
postgres=# select concat('a',10);
ERROR: function concat(unknown, integer) does not exist
LINE 1: select concat('a',10);
^
HINT: No function matches the given name and argument types. You
might need to add explicit type casts.

so with variadic "any"[] concat doesn't need explicit cats.

Regards

Pavel Stehule

p.s. inside function is every value transformed to text.

> merlin
>

On Mon, Jul 26, 2010 at 9:10 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>> CONCAT('foo', NULL) => 'foo' really the behavior that everyone else
>> implements here?  And why does CONCAT() take a variadic "ANY"
>> argument?  Shouldn't that be variadic TEXT?
>
> What does that accomplish, besides forcing you to sprinkle every
> concat call with text casts (maybe that's not a bad thing?)?

You could ask the same thing about the existing || operator. And in
fact, we used to have that behavior. We changed it in 8.3. Perhaps
that was a good decision and perhaps it wasn't, but I don't think
using CONCAT() to make an end-run around that decision is the way to
go.

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

On Mon, Jul 26, 2010 at 9:26 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jul 26, 2010 at 9:10 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>>> CONCAT('foo', NULL) => 'foo' really the behavior that everyone else
>>> implements here?  And why does CONCAT() take a variadic "ANY"
>>> argument?  Shouldn't that be variadic TEXT?
>>
>> What does that accomplish, besides forcing you to sprinkle every
>> concat call with text casts (maybe that's not a bad thing?)?
>
> You could ask the same thing about the existing || operator.  And in
> fact, we used to have that behavior.  We changed it in 8.3.  Perhaps
> that was a good decision and perhaps it wasn't, but I don't think
> using CONCAT() to make an end-run around that decision is the way to
> go.

It was absolutely a good decision because it prevented type inference
in ways that were ambiguous or surprising (for a canonical case see:
http://www.mail-archive.com/pgsql-general(at)postgresql(dot)org/msg93224.html).

|| operator is still pretty tolerant in the 8.3+ world.
select interval_col || bool_col; -- error
select interval_col::text || bool_col; -- text concatenation
select text_col || interval_col || bool_col; -- text concatenation

variadic text would require text casts on EVERY non 'unknown' argument
which drops it below the threshold of usefulness IMO -- it would be
far stricter than vanilla || concatenation. Ok, pure bikeshed here
(shutting my trap now!), but concat() is one of those wonder functions
that you want to make as usable and terse as possible. I don't see
the value in making it overly strict.

merlin

On Mon, Jul 26, 2010 at 10:39 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
> On Mon, Jul 26, 2010 at 9:26 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Jul 26, 2010 at 9:10 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>>>> CONCAT('foo', NULL) => 'foo' really the behavior that everyone else
>>>> implements here?  And why does CONCAT() take a variadic "ANY"
>>>> argument?  Shouldn't that be variadic TEXT?
>>>
>>> What does that accomplish, besides forcing you to sprinkle every
>>> concat call with text casts (maybe that's not a bad thing?)?
>>
>> You could ask the same thing about the existing || operator.  And in
>> fact, we used to have that behavior.  We changed it in 8.3.  Perhaps
>> that was a good decision and perhaps it wasn't, but I don't think
>> using CONCAT() to make an end-run around that decision is the way to
>> go.
>
> It was absolutely a good decision because it prevented type inference
> in ways that were ambiguous or surprising (for a canonical case see:
> http://www.mail-archive.com/pgsql-general(at)postgresql(dot)org/msg93224.html).
>
> || operator is still pretty tolerant in the 8.3+ world.
> select interval_col || bool_col; -- error
> select interval_col::text || bool_col; -- text concatenation
> select text_col || interval_col || bool_col; -- text concatenation
>
> variadic text would require text casts on EVERY non 'unknown' argument
> which drops it below the threshold of usefulness IMO -- it would be
> far stricter than vanilla || concatenation.  Ok, pure bikeshed here
> (shutting my trap now!), but concat() is one of those wonder functions
> that you want to make as usable and terse as possible.  I don't see
> the value in making it overly strict.

I'm just very skeptical that we should give our functions argument
types that are essentially fantasy. CONCAT() doesn't concatenate
integers or intervals or boxes: it concatenates strings, and only
strings. Surely the right fix, if our casting rules are too
restrictive, is to fix the casting rules; not to start adding a bunch
of kludgery in every function we define.

The problem with your canonical example (and the other examples of
this type I've seen) is that they are underspecified. Given two
identically-named operators, one of which accepts types T1 and T2, and
the other of which accepts types T3 and T4, what is one to do with T1
OP T4? You can cast T1 to T3 and call the first operator or you can
cast T4 to T2 and call the second one, and it's really not clear which
is right, so you had better thrown an error. The same applies to
functions: given foo(text) and foo(date) and the call
foo('2010-04-15'), you had better complain, or you may end up with a
very sad user. But sometimes our current casting rules require casts
in situations where they don't seem necessary, such as
LPAD(integer_column, '0', 5). That's not ambiguous because there's
only one definition of LPAD, and the chances that the user will be
unpleasantly surprised if you call it seem quite low.

In other words, this problem is not unique to CONCAT().

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

On Mon, Jul 26, 2010 at 11:07 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jul 26, 2010 at 10:39 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>> It was absolutely a good decision because it prevented type inference
>> in ways that were ambiguous or surprising (for a canonical case see:
>> http://www.mail-archive.com/pgsql-general(at)postgresql(dot)org/msg93224.html).
>>
>> || operator is still pretty tolerant in the 8.3+ world.
>> select interval_col || bool_col; -- error
>> select interval_col::text || bool_col; -- text concatenation
>> select text_col || interval_col || bool_col; -- text concatenation
>>
>> variadic text would require text casts on EVERY non 'unknown' argument
>> which drops it below the threshold of usefulness IMO -- it would be
>> far stricter than vanilla || concatenation.  Ok, pure bikeshed here
>> (shutting my trap now!), but concat() is one of those wonder functions
>> that you want to make as usable and terse as possible.  I don't see
>> the value in making it overly strict.
>
> I'm just very skeptical that we should give our functions argument
> types that are essentially fantasy.  CONCAT() doesn't concatenate
> integers or intervals or boxes: it concatenates strings, and only
> strings.  Surely the right fix, if our casting rules are too
> restrictive, is to fix the casting rules; not to start adding a bunch
> of kludgery in every function we define.
>
> The problem with your canonical example (and the other examples of
> this type I've seen) is that they are underspecified.  Given two
> identically-named operators, one of which accepts types T1 and T2, and
> the other of which accepts types T3 and T4, what is one to do with T1
> OP T4?  You can cast T1 to T3 and call the first operator or you can
> cast T4 to T2 and call the second one, and it's really not clear which
> is right, so you had better thrown an error.  The same applies to
> functions: given foo(text) and foo(date) and the call
> foo('2010-04-15'), you had better complain, or you may end up with a
> very sad user.  But sometimes our current casting rules require casts
> in situations where they don't seem necessary, such as
> LPAD(integer_column, '0', 5).  That's not ambiguous because there's
> only one definition of LPAD, and the chances that the user will be
> unpleasantly surprised if you call it seem quite low.
>
> In other words, this problem is not unique to CONCAT().

shoot, can't resist :-).

Are the casting rules broken? If you want to do lpad w/o casts for
integers, you can define it explicitly -- feature, not bug. You can
basically do this for any function with fixed arguments -- either in
userland or core. lpad(int) actually introduces a problem case with
the minus sign possibly requiring application specific intervention,
so things are probably correct exactly as they are. Casting rules
need to be tight because if they are not they can introduce
independent behaviors when you underspecify as you noted above.

ISTM you are unhappy with the "any" variadic mechanism in general, not
the casting rules. "any" essentially means: "the types you pass to me
are irrelevant, because i'm going to look up behaviors on the server
and apply them as I see fit". You've defined a regular methodology
across ALL types and allow the user to pass anything -- I see nothing
at all wrong with this as long as it's only implemented in very
special cases. If you happen to not like the predefined 'box'
behavior, nothing is stopping you from massaging it before sending it
in. Also, there's no guarantee that the behavior hidden under the
"any" can be reproduced via manual cast...concat() is some thing of a
special case where you can.

merlin

2010/7/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Jul 26, 2010 at 10:39 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>> On Mon, Jul 26, 2010 at 9:26 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Mon, Jul 26, 2010 at 9:10 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>>>>> CONCAT('foo', NULL) => 'foo' really the behavior that everyone else
>>>>> implements here?  And why does CONCAT() take a variadic "ANY"
>>>>> argument?  Shouldn't that be variadic TEXT?
>>>>
>>>> What does that accomplish, besides forcing you to sprinkle every
>>>> concat call with text casts (maybe that's not a bad thing?)?
>>>
>>> You could ask the same thing about the existing || operator.  And in
>>> fact, we used to have that behavior.  We changed it in 8.3.  Perhaps
>>> that was a good decision and perhaps it wasn't, but I don't think
>>> using CONCAT() to make an end-run around that decision is the way to
>>> go.
>>
>> It was absolutely a good decision because it prevented type inference
>> in ways that were ambiguous or surprising (for a canonical case see:
>> http://www.mail-archive.com/pgsql-general(at)postgresql(dot)org/msg93224.html).
>>
>> || operator is still pretty tolerant in the 8.3+ world.
>> select interval_col || bool_col; -- error
>> select interval_col::text || bool_col; -- text concatenation
>> select text_col || interval_col || bool_col; -- text concatenation
>>
>> variadic text would require text casts on EVERY non 'unknown' argument
>> which drops it below the threshold of usefulness IMO -- it would be
>> far stricter than vanilla || concatenation.  Ok, pure bikeshed here
>> (shutting my trap now!), but concat() is one of those wonder functions
>> that you want to make as usable and terse as possible.  I don't see
>> the value in making it overly strict.
>
> I'm just very skeptical that we should give our functions argument
> types that are essentially fantasy.  CONCAT() doesn't concatenate
> integers or intervals or boxes: it concatenates strings, and only
> strings.  Surely the right fix, if our casting rules are too
> restrictive, is to fix the casting rules; not to start adding a bunch
> of kludgery in every function we define.
>

Rules are correct probably. The problem is in searching function
algorithm - it is too simple (and fast, just only one cycle). And some
exceptions - like COALESCE and similar are solved specifically on
parser level. We cannot enforce some casting on user level. PostgreSQL
is more strict with timestamps, dates than other databases. Sometimes
you have to do explicit casts, but it clean from context desired
datatype -

SELECT date_trunc('day', current_date) - result is timestamp, but it
is clean, so result have to be date ... When I proposed a parser hook
I though about these functions. With this hook, you can enforce any
necessary casting like some buildin functions does.

so "any"

> The problem with your canonical example (and the other examples of
> this type I've seen) is that they are underspecified.  Given two
> identically-named operators, one of which accepts types T1 and T2, and
> the other of which accepts types T3 and T4, what is one to do with T1
> OP T4?  You can cast T1 to T3 and call the first operator or you can
> cast T4 to T2 and call the second one, and it's really not clear which
> is right, so you had better thrown an error.  The same applies to
> functions: given foo(text) and foo(date) and the call
> foo('2010-04-15'), you had better complain, or you may end up with a
> very sad user.  But sometimes our current casting rules require casts
> in situations where they don't seem necessary, such as
> LPAD(integer_column, '0', 5).  That's not ambiguous because there's
> only one definition of LPAD, and the chances that the user will be
> unpleasantly surprised if you call it seem quite low.
>
> In other words, this problem is not unique to CONCAT().
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

>
> so "any" datatype is last possibility - is workaroud for custom functions.

Probably the most correct implementation of CONCAT have to contains a
parser changes - and then you can use a some internal concat function
with text only parameters. VARIADIC with "any" is just workaround that
is enouhg

Regards

Pavel

>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>> The problem with your canonical example (and the other examples of
>> this type I've seen) is that they are underspecified.  Given two
>> identically-named operators, one of which accepts types T1 and T2, and
>> the other of which accepts types T3 and T4, what is one to do with T1
>> OP T4?  You can cast T1 to T3 and call the first operator or you can
>> cast T4 to T2 and call the second one, and it's really not clear which
>> is right, so you had better thrown an error.  The same applies to
>> functions: given foo(text) and foo(date) and the call
>> foo('2010-04-15'), you had better complain, or you may end up with a
>> very sad user.  But sometimes our current casting rules require casts
>> in situations where they don't seem necessary, such as
>> LPAD(integer_column, '0', 5).  That's not ambiguous because there's
>> only one definition of LPAD, and the chances that the user will be
>> unpleasantly surprised if you call it seem quite low.
>>
>> In other words, this problem is not unique to CONCAT().
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise Postgres Company
>>
>

On Mon, Jul 26, 2010 at 11:39 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>> I'm just very skeptical that we should give our functions argument
>> types that are essentially fantasy.  CONCAT() doesn't concatenate
>> integers or intervals or boxes: it concatenates strings, and only
>> strings.  Surely the right fix, if our casting rules are too
>> restrictive, is to fix the casting rules; not to start adding a bunch
>> of kludgery in every function we define.
>>
>> The problem with your canonical example (and the other examples of
>> this type I've seen) is that they are underspecified.  Given two
>> identically-named operators, one of which accepts types T1 and T2, and
>> the other of which accepts types T3 and T4, what is one to do with T1
>> OP T4?  You can cast T1 to T3 and call the first operator or you can
>> cast T4 to T2 and call the second one, and it's really not clear which
>> is right, so you had better thrown an error.  The same applies to
>> functions: given foo(text) and foo(date) and the call
>> foo('2010-04-15'), you had better complain, or you may end up with a
>> very sad user.  But sometimes our current casting rules require casts
>> in situations where they don't seem necessary, such as
>> LPAD(integer_column, '0', 5).  That's not ambiguous because there's
>> only one definition of LPAD, and the chances that the user will be
>> unpleasantly surprised if you call it seem quite low.
>>
>> In other words, this problem is not unique to CONCAT().
>
> shoot, can't resist :-).
>
> Are the casting rules broken? If you want to do lpad w/o casts for
> integers, you can define it explicitly -- feature, not bug.  You can
> basically do this for any function with fixed arguments -- either in
> userland or core.  lpad(int) actually introduces a problem case with
> the minus sign possibly requiring application specific intervention,
> so things are probably correct exactly as they are.

Huh? If you're arguing that LPAD should require a cast on an integer
argument because the defined behavior of the function might not be
what someone wants, then apparently explicit casts should be required
in all cases. If you're arguing that I can eliminate the need for an
explicit cast by overloading LPAD(), I agree with you, but that's not
a feature.

> ISTM you are unhappy with the "any" variadic mechanism in general, not
> the casting rules.

No, I'm unhappy with the use of "any" to make an end-run around the
casting rules. If you're writing a function that operates on an
argument of any type, like pg_sizeof() - or if you're writing a
function that does something like append two arrays of unspecified but
identical type or, perhaps, search an array of unspecified type for an
element of that same type - or if you're writing a function where the
types of the arguments can't be known in advance, like printf(), well,
then any is what you need. But the only argument anyone has put
forward for making CONCAT() accept ANY instead of TEXT is that it
might require casting otherwise. My response to that is "well then
you have to cast it, or fix the casting rules".

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

2010/7/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Jul 26, 2010 at 11:39 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>>> I'm just very skeptical that we should give our functions argument
>>> types that are essentially fantasy.  CONCAT() doesn't concatenate
>>> integers or intervals or boxes: it concatenates strings, and only
>>> strings.  Surely the right fix, if our casting rules are too
>>> restrictive, is to fix the casting rules; not to start adding a bunch
>>> of kludgery in every function we define.
>>>
>>> The problem with your canonical example (and the other examples of
>>> this type I've seen) is that they are underspecified.  Given two
>>> identically-named operators, one of which accepts types T1 and T2, and
>>> the other of which accepts types T3 and T4, what is one to do with T1
>>> OP T4?  You can cast T1 to T3 and call the first operator or you can
>>> cast T4 to T2 and call the second one, and it's really not clear which
>>> is right, so you had better thrown an error.  The same applies to
>>> functions: given foo(text) and foo(date) and the call
>>> foo('2010-04-15'), you had better complain, or you may end up with a
>>> very sad user.  But sometimes our current casting rules require casts
>>> in situations where they don't seem necessary, such as
>>> LPAD(integer_column, '0', 5).  That's not ambiguous because there's
>>> only one definition of LPAD, and the chances that the user will be
>>> unpleasantly surprised if you call it seem quite low.
>>>
>>> In other words, this problem is not unique to CONCAT().
>>
>> shoot, can't resist :-).
>>
>> Are the casting rules broken? If you want to do lpad w/o casts for
>> integers, you can define it explicitly -- feature, not bug.  You can
>> basically do this for any function with fixed arguments -- either in
>> userland or core.  lpad(int) actually introduces a problem case with
>> the minus sign possibly requiring application specific intervention,
>> so things are probably correct exactly as they are.
>
> Huh?  If you're arguing that LPAD should require a cast on an integer
> argument because the defined behavior of the function might not be
> what someone wants, then apparently explicit casts should be required
> in all cases.  If you're arguing that I can eliminate the need for an
> explicit cast by overloading LPAD(), I agree with you, but that's not
> a feature.
>
>> ISTM you are unhappy with the "any" variadic mechanism in general, not
>> the casting rules.
>
> No, I'm unhappy with the use of "any" to make an end-run around the
> casting rules.  If you're writing a function that operates on an
> argument of any type, like pg_sizeof() - or if you're writing a
> function that does something like append two arrays of unspecified but
> identical type or, perhaps, search an array of unspecified type for an
> element of that same type - or if you're writing a function where the
> types of the arguments can't be known in advance, like printf(), well,
> then any is what you need.  But the only argument anyone has put
> forward for making CONCAT() accept ANY instead of TEXT is that it
> might require casting otherwise.  My response to that is "well then
> you have to cast it, or fix the casting rules".

I understand, but with only text accepting, then CONCAT will has much
less benefit - you can't do a numeric list, for example

see concat(c1::text, ',', c2::text, ',' ...)

with this is much simpler use a pipes '' || c1 || ',' || c2 ... and
this operator does necessary cast self.

This function is probably one use case of exception from our rules.

Regards

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

On Mon, Jul 26, 2010 at 2:09 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> I understand, but with only text accepting, then CONCAT will has much
> less benefit - you can't do a numeric list, for example
>
> see concat(c1::text, ',', c2::text, ',' ...)
>
> with this is much simpler use a pipes '' || c1 || ',' || c2 ... and
> this operator does necessary cast self.
>
> This function is probably one use case of exception from our rules.

At least two, right? Because for that use case you'd probably want
concat_ws(). In fact, it's hard for me to think of a variadic text
function where you wouldn't want the "no casts" behavior you're
getting via ANY.

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

On Mon, Jul 26, 2010 at 3:07 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jul 26, 2010 at 2:09 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> I understand, but with only text accepting, then CONCAT will has much
>> less benefit - you can't do a numeric list, for example
>>
>> see concat(c1::text, ',', c2::text, ',' ...)
>>
>> with this is much simpler use a pipes '' || c1 || ',' || c2 ... and
>> this operator does necessary cast self.
>>
>> This function is probably one use case of exception from our rules.
>
> At least two, right?

correct: there would be at least two.

> Because for that use case you'd probably want
> concat_ws().  In fact, it's hard for me to think of a variadic text
> function where you wouldn't want the "no casts" behavior you're
> getting via ANY.

concat() is not a variadic text function. it is variadic "any" that
happens to do text coercion (not casting) inside the function. The
the assumption that concat is casting internally is probably wrong.
Suppose I had hacked the int->text cast to call a custom function -- I
would very much expect concat() not to produce output from that
function, just the vanilla output text (I could always force the cast
if I wanted to).

concat is just a function that does something highly similar to
casting. suppose I had a function count_memory(variadic "any") that
summed memory usage of input args -- forcing casts would make no sense
in that context (I'm not suggesting that you think so -- just bringing
up a case that illustrates how forcing cast into the function can
change behavior in subtle ways).

merlin

On Mon, Jul 26, 2010 at 3:49 PM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
> concat() is not a variadic text function. it is variadic "any" that
> happens to do text coercion (not casting) inside the function. The
> the assumption that concat is casting internally is probably wrong.
> Suppose I had hacked the int->text cast to call a custom function -- I
> would very much expect concat() not to produce output from that
> function, just the vanilla output text (I could always force the cast
> if I wanted to).
>
> concat is just a function that does something highly similar to
> casting.  suppose I had a function count_memory(variadic "any") that
> summed memory usage of input args -- forcing casts would make no sense
> in that context (I'm not suggesting that you think so -- just bringing
> up a case that illustrates how forcing cast into the function can
> change behavior in subtle ways).

Right, but I already said I wasn't objecting to the use of variadic
ANY in cases like that - only in cases where, as here, you were
basically taking any old arguments and forcing them all to text.

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

2010/7/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Jul 26, 2010 at 2:09 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> I understand, but with only text accepting, then CONCAT will has much
>> less benefit - you can't do a numeric list, for example
>>
>> see concat(c1::text, ',', c2::text, ',' ...)
>>
>> with this is much simpler use a pipes '' || c1 || ',' || c2 ... and
>> this operator does necessary cast self.
>>
>> This function is probably one use case of exception from our rules.
>
> At least two, right?  Because for that use case you'd probably want
> concat_ws().

sorry, yes

Pavel

 In fact, it's hard for me to think of a variadic text
> function where you wouldn't want the "no casts" behavior you're
> getting via ANY.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Hi Pavel,

In xfunc.sgml, I came across a function example (for use of VARIADIC in polymorphic functions),
where the function name is concat(): (in the manual: 35.4.10. Polymorphic SQL Functions).
Although that is not strictly wrong, it seems better to change that name when concat goes into
core, as seems to be the plan.

If you agree, it seems best to include this change in your patch and change that example
function's name when the stringfunc patch gets applied.

Erik Rijkers

On Thu, July 29, 2010 22:43, Erik Rijkers wrote:
> Hi Pavel,
>
> In xfunc.sgml, I came across a function example (for use of VARIADIC in polymorphic functions),
> where the function name is concat(): (in the manual: 35.4.10. Polymorphic SQL Functions).
> Although that is not strictly wrong, it seems better to change that name when concat goes into
> core, as seems to be the plan.
>
> If you agree, it seems best to include this change in your patch and change that example
> function's name when the stringfunc patch gets applied.
>

My apologies, the previous email had the wrong doc-patch attached.

Here is the correct one.

Erik Rijkers

On Mon, Jul 26, 2010 at 8:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jul 26, 2010 at 3:49 PM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>> concat() is not a variadic text function. it is variadic "any" that
>> happens to do text coercion (not casting) inside the function. The
>> the assumption that concat is casting internally is probably wrong.
>> Suppose I had hacked the int->text cast to call a custom function -- I
>> would very much expect concat() not to produce output from that
>> function, just the vanilla output text (I could always force the cast
>> if I wanted to).
>>
>> concat is just a function that does something highly similar to
>> casting.  suppose I had a function count_memory(variadic "any") that
>> summed memory usage of input args -- forcing casts would make no sense
>> in that context (I'm not suggesting that you think so -- just bringing
>> up a case that illustrates how forcing cast into the function can
>> change behavior in subtle ways).
>
> Right, but I already said I wasn't objecting to the use of variadic
> ANY in cases like that - only in cases where, as here, you were
> basically taking any old arguments and forcing them all to text.

I believe that another unpleasant side effect of this is that CONCAT()
will have to be declared stable rather than immutable.

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

Hello

2010/7/29 Erik Rijkers <er(at)xs4all(dot)nl>:
> On Thu, July 29, 2010 22:43, Erik Rijkers wrote:
>> Hi Pavel,
>>
>> In xfunc.sgml, I came across a function example (for use of VARIADIC in polymorphic functions),
>> where the function name is concat():  (in the manual: 35.4.10. Polymorphic SQL Functions).
>> Although that is not strictly wrong, it seems better to change that name when concat goes into
>> core, as seems to be the plan.
>>
>> If you agree, it seems best to include this change in your patch and change that example
>> function's name when the stringfunc patch gets applied.
>>
>
> My apologies, the previous email had the wrong doc-patch attached.
>
> Here is the correct one.
>

it is good idea, thank you

Pavel

>
> Erik Rijkers
>

2010/7/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> Come to think of it, have we checked that the behavior of LEFT, RIGHT,
> REVERSE, etc. is the same on other DBs, especially as far as nulls,
> empty strings, too-large or negative subscripts, etc is concerned?  Is
> CONCAT('foo', NULL) => 'foo' really the behavior that everyone else
> implements here?

I made a discussion page in wiki for the compatibility issue.
http://wiki.postgresql.org/wiki/String_Functions_and_Operators_Compatibility

Please fill empty cells and fix wrong descriptions.
* concat() is not compatible between MySQL and Oracle/DB2. Which do we buy?
* How do other databases behave in left() and right() with negative lengths?
* Are there any databases that has similar features with format() or
sprintf() ?

> And why does CONCAT() take a variadic "ANY"
> argument?  Shouldn't that be variadic TEXT?

I think we have no other choice but to use VARIADIC "any" for variadic
functions.
We have all combinations of argument types for || operator, (text, text),
(text, any), (any, text), but we cannot use such codes for variadic functions
-- they have no limits of argument numbers. And in the case, the functions
should be STABLE because they convert arguments to text in it with typout
functions that might be STABLE.

IMHO, I'd repeat, syntax for format() is a bad choice because it cannot
concatenate multiple arguments without separator, though RAISE also uses it.
%s format in sprintf() or {n} syntax in C#'s String.Format() seems to be
a better design.

--
Itagaki Takahiro

Hello

2010/8/7 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> 2010/7/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> Come to think of it, have we checked that the behavior of LEFT, RIGHT,
>> REVERSE, etc. is the same on other DBs, especially as far as nulls,
>> empty strings, too-large or negative subscripts, etc is concerned?  Is
>> CONCAT('foo', NULL) => 'foo' really the behavior that everyone else
>> implements here?
>
> I made a discussion page in wiki for the compatibility issue.
> http://wiki.postgresql.org/wiki/String_Functions_and_Operators_Compatibility
>

nice, thank you

> Please fill empty cells and fix wrong descriptions.
>  * concat() is not compatible between MySQL and Oracle/DB2. Which do we buy?

I prefer a our implementation - it skip a NULL values and it has a
variadic arguments. MySQL's concat isn't too consistent - I don't know
why it has different NULL handlidg than concat_ws.

>  * How do other databases behave in left() and right() with negative lengths?

I don't know about one with left() and right() functions. What I know,
only MS Access has these functions. The design of these functions is
inspirited by wide used a Oracle library PLvision - this library is
freeware now - but my code is original. See plvstr.left() and
plvstr.right() - and little bit by python substring operations. The
sense of negative arguments is elimination of necessary detoast
operations and utf8 related calculations. For right() it means skip
first n chars, for left() skip last n chars. These functions was
originally designed for contrib - and I still thinking so contrib is
better - My opinion isn't strong here - I prefer a fully functional
function in contrib before minimalistic version in core. Minimalistic
functions are trivial via substring.

>  * Are there any databases that has similar features with format() or
> sprintf() ?

I know only about package from PLvision library -

select plvsubst.string('My name is %s %s', ARRAY['Pavel','Stěhule']);

but you can find a lot of custom implementations. I found a some
similar - not exactly this in T-SQL see FORMATMESSAGE() function. But
the using of this function is very limited and it is C API function
(available from T-SQL). It doesn't return a string, just write to log.

>
>
>> And why does CONCAT() take a variadic "ANY"
>> argument?  Shouldn't that be variadic TEXT?
>
> I think we have no other choice but to use VARIADIC "any" for variadic
> functions.
> We have all combinations of argument types for || operator, (text, text),
> (text, any), (any, text), but we cannot use such codes for variadic functions
> -- they have no limits of argument numbers. And in the case, the functions
> should be STABLE because they convert arguments to text in it with typout
> functions that might be STABLE.
>
>
> IMHO, I'd repeat, syntax for format() is a bad choice because it cannot
> concatenate multiple arguments without separator, though RAISE also uses it.
> %s format in sprintf() or {n} syntax in C#'s String.Format() seems to be
> a better design.

I don't agree. This function isn't designed to replace string
concation. It is designed to build a SQL string (for dynamic SQL) or
format messages. It isn't designed to replace to_char function. It is
designed to work mainly inside PLpgSQL functions and then is
consistent with RAISE statement.

Thank you

Regards

Pavel Stehule

>
> --
> Itagaki Takahiro
>

On Sat, Aug 7, 2010 at 8:39 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> I made a discussion page in wiki for the compatibility issue.
>> http://wiki.postgresql.org/wiki/String_Functions_and_Operators_Compatibility
> nice, thank you

I filled cells for SQL Server and DB2.

>>  * concat() is not compatible between MySQL and Oracle/DB2. Which do we buy?
> I prefer a our implementation - it skip a NULL values and it has a
> variadic arguments.

OK. I'm going to put both concat() and concat_ws() into core.

>>  * How do other databases behave in left() and right() with negative lengths?
> little bit by python substring operations.

I'll respect your proposal. The behaviors for negative lengths will
be our specific feature, but I don't see any problems there.
Since other databases raises errors, user should have negative-protections
in their existing codes.

> I don't agree. This function isn't designed to replace string
> concation. It is designed to build a SQL string (for dynamic SQL) or
> format messages. It isn't designed to replace to_char function. It is
> designed to work mainly inside PLpgSQL functions and then is
> consistent with RAISE statement.

OK. I'll revert my changes to your original format().

But please wait a moment to include sprintf() and contrib/stringfunc.
I think the function is useful, but don't want to have two versions
of formatting functions. So, the extended features will be merged
into format() with additional syntax something like {10s}. Then,
we could simplify the code because some of complex format syntax
are not so useful in SQL, especially length+string formatter (*s).

--
Itagaki Takahiro

2010/8/23 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> On Sat, Aug 7, 2010 at 8:39 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> I made a discussion page in wiki for the compatibility issue.
>>> http://wiki.postgresql.org/wiki/String_Functions_and_Operators_Compatibility
>> nice, thank you
>
> I filled cells for SQL Server and DB2.
>
>>>  * concat() is not compatible between MySQL and Oracle/DB2. Which do we buy?
>> I prefer a our implementation - it skip a NULL values and it has a
>> variadic arguments.
>
> OK. I'm going to put both concat() and concat_ws() into core.
>
>>>  * How do other databases behave in left() and right() with negative lengths?
>> little bit by python substring operations.
>
> I'll respect your proposal. The behaviors for negative lengths will
> be our specific feature, but I don't see any problems there.
> Since other databases raises errors, user should have negative-protections
> in their existing codes.
>
>> I don't agree. This function isn't designed to replace string
>> concation. It is designed to build a SQL string (for dynamic SQL) or
>> format messages. It isn't designed to replace to_char function. It is
>> designed to work mainly inside PLpgSQL functions and then is
>> consistent with RAISE statement.
>
> OK. I'll revert my changes to your original format().
>
> But please wait a moment to include sprintf() and contrib/stringfunc.
> I think the function is useful, but don't want to have two versions
> of formatting functions. So, the extended features will be merged
> into format() with additional syntax something like {10s}. Then,
> we could simplify the code because some of complex format syntax
> are not so useful in SQL, especially length+string formatter (*s).

It's reason, why I moved sprintf to contrib. When you build a SQL
statement or error message, you don't need (usually) complex
formating. And when you need it, then you can use a contrib sprintf
function. I am not against to additional simply formating - but I
afraid so we will create a second "printf" function. The formating
enhancing should be shared with RAISE NOTICE command. Probably we can
share code better now, when a PLpgSQL is in core.

Regards

Pavel

>
> --
> Itagaki Takahiro
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> ... The formating
> enhancing should be shared with RAISE NOTICE command.

I remain of the opinion that RAISE's approach to formatting is
completely broken and inextensible, and that any attempt to be somehow
compatible with it is going to lead to an unusably broken design.
You should leave RAISE alone and just think about printf.

regards, tom lane

2010/8/23 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> ... The formating
>> enhancing should be shared with RAISE NOTICE command.
>
> I remain of the opinion that RAISE's approach to formatting is
> completely broken and inextensible, and that any attempt to be somehow
> compatible with it is going to lead to an unusably broken design.
> You should leave RAISE alone and just think about printf.
>

ok - then we don't need modify proposed patch. "Format" function is
enough for PL/pgSQL and other PL languages has own mutation of this
functions. There are not barrier for implementation as custom
function, so we can hold this function most simple.

Regards

Pavel

>                        regards, tom lane
>

I applied the attached patch to HEAD. concat(), concat_ws(), left(),
right(), and reverse() are in it, but format() and sprintf() are not.
It's my understanding that we don't have consensus about the best syntax
for the formatting function. We can forget about RAISE. C-like printf
syntax is the next candidate, but we should consider about other ones
restarting with a clean slate.

Anyway, the newly added functions are useful for developers especially
migrated from other database products. Thank you.

On Mon, Aug 23, 2010 at 11:41 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2010/8/23 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> You should leave RAISE alone and just think about printf.
>
> ok - then we don't need modify proposed patch. "Format" function is
> enough for PL/pgSQL and other PL languages has own mutation of this
> functions. There are not barrier for implementation as custom
> function, so we can hold this function most simple.

--
Itagaki Takahiro

2010/8/24 Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>:
> I applied the attached patch to HEAD. concat(), concat_ws(), left(),
> right(), and reverse() are in it, but format() and sprintf() are not.
> It's my understanding that we don't have consensus about the best syntax
> for the formatting function. We can forget about RAISE. C-like printf
> syntax is the next candidate, but we should consider about other ones
> restarting with a clean slate.
>

Thank you very much

C-like printf function is the most worse candidate - I don't like to
repeat discussion again - this function is designed for totally
different environment than SQL. I am sure, so we don't need a function
with complex formatting - maintaining "to_char" is good example.

Regards

Pavel Stehule

> Anyway, the newly added functions are useful for developers especially
> migrated from other database products. Thank you.
>
>
> On Mon, Aug 23, 2010 at 11:41 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> 2010/8/23 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> You should leave RAISE alone and just think about printf.
>>
>> ok - then we don't need modify proposed patch. "Format" function is
>> enough for PL/pgSQL and other PL languages has own mutation of this
>> functions. There are not barrier for implementation as custom
>> function, so we can hold this function most simple.
>
> --
> Itagaki Takahiro
>

On Tue, August 24, 2010 08:32, Itagaki Takahiro wrote:
> I applied the attached patch to HEAD. concat(), concat_ws(), left(),
> right(), and reverse() are in it, but format() and sprintf() are not.

+1 to add also sprintf

Erik Rijkers

Erik Rijkers wrote:
> On Thu, July 29, 2010 22:43, Erik Rijkers wrote:
> > Hi Pavel,
> >
> > In xfunc.sgml, I came across a function example (for use of VARIADIC in polymorphic functions),
> > where the function name is concat(): (in the manual: 35.4.10. Polymorphic SQL Functions).
> > Although that is not strictly wrong, it seems better to change that name when concat goes into
> > core, as seems to be the plan.
> >
> > If you agree, it seems best to include this change in your patch and change that example
> > function's name when the stringfunc patch gets applied.
> >
>
> My apologies, the previous email had the wrong doc-patch attached.
>
> Here is the correct one.

Applied for 9.1. Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +