patch (for 9.1) string functions

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

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.stehule@gmail.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@gmail.com>:

On Tue, Mar 9, 2010 at 9:30 AM, Pavel Stehule <pavel.stehule@gmail.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

updated version, concat function doesn't use separator

Pavel

2010/3/9 Pavel Stehule <pavel.stehule@gmail.com>:

On Tue, Mar 9, 2010 at 1:45 PM, Pavel Stehule <pavel.stehule@gmail.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@gmail.com>:

On Tue, Mar 9, 2010 at 1:45 PM, Pavel Stehule <pavel.stehule@gmail.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

Pavel Stehule <pavel.stehule@gmail.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.takahiro@oss.ntt.co.jp>:

Pavel Stehule <pavel.stehule@gmail.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

2010/7/8 Pavel Stehule <pavel.stehule@gmail.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.takahiro@gmail.com>:

2010/7/8 Pavel Stehule <pavel.stehule@gmail.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.stehule@gmail.com>:

hello

2010/7/9 Takahiro Itagaki <itagaki.takahiro@gmail.com>:

2010/7/8 Pavel Stehule <pavel.stehule@gmail.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

contrib/stringfunc was missing this small change in contrib/Makefile, I think. With it, it
installs and runs make check cleanly.

Erik Rijkers

2010/7/9 Pavel Stehule <pavel.stehule@gmail.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.takahiro@gmail.com> wrote:

2010/7/9 Pavel Stehule <pavel.stehule@gmail.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@gmail.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@gmail.com>:

On Sun, Jul 11, 2010 at 10:30 PM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:

2010/7/9 Pavel Stehule <pavel.stehule@gmail.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

On Jul 11, 2010, at 10:32 PM, Itagaki Takahiro <itagaki.takahiro@gmail.com> wrote:

2010/7/12 Robert Haas <robertmhaas@gmail.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