Prev: ALTER TABLE SET STATISTICS requiresAccessExclusiveLock
Next: [HACKERS] leaky views, yet again
From: Takahiro Itagaki on 8 Jul 2010 03:15

Pavel Stehule <pavel.stehule(a)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



--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Takahiro Itagaki on 8 Jul 2010 22:36
2010/7/8 Pavel Stehule <pavel.stehule(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Pavel Stehule on 9 Jul 2010 03:40
hello

2010/7/9 Takahiro Itagaki <itagaki.takahiro(a)gmail.com>:
> 2010/7/8 Pavel Stehule <pavel.stehule(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Pavel Stehule on 9 Jul 2010 05:12
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(a)gmail.com>:
> hello
>
> 2010/7/9 Takahiro Itagaki <itagaki.takahiro(a)gmail.com>:
>> 2010/7/8 Pavel Stehule <pavel.stehule(a)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

>>
>
>
>> 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
>
From: "Erik Rijkers" on 10 Jul 2010 18:34
contrib/stringfunc was missing this small change in contrib/Makefile, I think. With it, it
installs and runs make check cleanly.


Erik Rijkers

 |  Next  |  Last
Pages: 1 2 3 4 5
Prev: ALTER TABLE SET STATISTICS requiresAccessExclusiveLock
Next: [HACKERS] leaky views, yet again