On Thu, Sep 9, 2010 at 8:57 PM, Pavel Stehule <pavel.steh...@gmail.com> wrote:
> I am sending a updated version.
>
> changes:
> * tag %v removed from format function,
> * proprietary tags %lq a iq removed from sprintf
> * code cleaned
>
> patch divided to two parts - format function and stringfunc (contains
> sprintf function and substitute function)
=== Discussions about the spec ===
Two patches add format() into the core, and substitute() and sprintf() into
stringfunc contrib module. But will we have 3 versions of string formatters?
IMHO, substitute() is the best choice that we will have in the core because
functionalities in format() and sprintf() can be achieved by combination of
substitute() and quote_nullable(), quote_ident(), or to_char(). I think the
core will provide only simple and non-overlapped features. Users can write
wrapper functions by themselves if they think the description is redundant.
=== format.diff ===
* It has a reject in doc, but the hunk can be fixed easily.
1 out of 2 hunks FAILED -- saving rejects to file doc/src/sgml/func.sgml.rej
COMMENT: We have the function list in alphabetical order,
so format() should be inserted after encode().
* It can be built without compile warnings.
* Enough documentation and regression tests are included.
=== stringfunc.diff ===
* It can be applied cleanly and built without compile warnings.
* Documentation is included, but not enough.
COMMENT: According to existing docs, function list are described with
<variablelist> or <table>.
* Enough regression tests are included.
* COMMENT: stringfunc directory should be added to contrib/Makefile.
* BUG: stringfunc_substitute_nv() calls text_format().
I think we don't need stringfunc_substitute_nv at all.
It can be replaced by stringfunc_substitute(). _nv version is only
required if it is in the core because of sanity regression test.
* BUG?: The doc says sprintf() doesn't support length modifiers,
but it is actually supported in broken state:
postgres=# SELECT sprintf('%*s', 2, 'ABC');
sprintf
---------
ABC <= should be ERROR if unsupported, or AB if supported.
(1 row)
* BUG?: ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("unsupported tag \"%%%c\"", tag)));
Is the code ok if the tag (a char) is a partial byte of multi-byte character?
My machine prints ? in the case, but it might be platform-dependent.
=== Both patches ===
* Performance: I don't think those functions are not performance-critical,
but we could cache typoutput functions in fn_extra if needed.
record_out would be a reference.
* Coding: Whitespace and tabs are mixed in some places. They are not so
important because we will run pgindent, but careful choice will be
preferred even of a patch.
--
Itagaki Takahiro
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
