patch (for 9.1) string functions
patch (for 9.1) string functions
От:
Pavel Stehule <pavel.stehule@gmail.com>
Дата:
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
Re: patch (for 9.1) string functions
От:
Pavel Stehule <pavel.stehule@gmail.com>
Дата:
updated version, concat function doesn't use separator
Pavel
2010/3/9 Pavel Stehule :
> 2010/3/9 Merlin Moncure :
>> On Tue, Mar 9, 2010 at 9:30 AM, Pavel Stehule 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
>>
>
Re: patch (for 9.1) string functions ( correct patch attached )
От:
"Erik Rijkers" <er@xs4all.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. Erik Rijkers
Re: patch (for 9.1) string functions ( correct patch attached )
От:
Bruce Momjian <bruce@momjian.us>
Дата:
Re: patch (for 9.1) string functions
От:
"Erik Rijkers" <er@xs4all.nl>
Дата:
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
Re: patch (for 9.1) string functions
От:
"Erik Rijkers" <er@xs4all.nl>
Дата:
contrib/stringfunc was missing this small change in contrib/Makefile, I think. With it, it installs and runs make check cleanly. Erik Rijkers
Re: patch (for 9.1) string functions
От:
Pavel Stehule <pavel.stehule@gmail.com>
Дата:
Hello
2010/7/23 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.
>
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 :
>> 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 "" 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
>
Re: patch (for 9.1) string functions
От:
Pavel Stehule <pavel.stehule@gmail.com>
Дата:
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 :
> hello
>
> 2010/7/9 Takahiro Itagaki :
>> 2010/7/8 Pavel Stehule :
>>> 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 "".
>>> I prefer just NULL.
>>> maybe some GUC variable
>>> stringfunc.null_string = '' in future??
>>
>> We have some choices for NULL representation. For example, empty string,
>> 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
>
Re: patch (for 9.1) string functions
От:
Pavel Stehule <pavel.stehule@gmail.com>
Дата:
Hello
2010/7/8 Takahiro Itagaki :
>
> Pavel Stehule 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 "". 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, ''));
sprintf
---------
(1 row)
maybe some GUC variable
stringfunc.null_string = '' 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
> 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
>
>
>
Re: patch (for 9.1) string functions
От:
Pavel Stehule <pavel.stehule@gmail.com>
Дата:
Hello
2010/7/13 Itagaki Takahiro :
> 2010/7/13 Pavel Stehule :
>> 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 "" 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
>
> funcname
>
> 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 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
Re: patch (for 9.1) string functions ( correct patch attached )
От:
Pavel Stehule <pavel.stehule@gmail.com>
Дата:
Re: patch (for 9.1) string functions
От:
Itagaki Takahiro <itagaki.takahiro@gmail.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. 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
Re: patch (for 9.1) string functions
От:
Itagaki Takahiro <itagaki.takahiro@gmail.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 :
> 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
Re: patch (for 9.1) string functions
От:
Pavel Stehule <pavel.stehule@gmail.com>
Дата:
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 "" 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 :
> 2010/7/12 Robert Haas :
>> 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
>
Re: patch (for 9.1) string functions
От:
Itagaki Takahiro <itagaki.takahiro@gmail.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. 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 wrote: > 2010/8/23 Tom Lane : >> 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
