Re: string function - "format" function proposal - Mailing list pgsql-hackers
From | Itagaki Takahiro |
---|---|
Subject | Re: string function - "format" function proposal |
Date | |
Msg-id | AANLkTi=GvM5q9qquFQSJnAo7r1mn4DP=T7ECp+yZMVAT@mail.gmail.com Whole thread Raw |
In response to | Re: string function - "format" function proposal (Pavel Stehule <pavel.stehule@gmail.com>) |
Responses |
Re: string function - "format" function proposal
|
List | pgsql-hackers |
On Thu, Sep 9, 2010 at 8:57 PM, Pavel Stehule <pavel.stehule@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.rejCOMMENT: We have the function list in alphabetical order, so format() should be inserted afterencode(). * 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 replacedby 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)));Isthe code ok if the tag (a char) is a partial byte of multi-byte character?My machine prints ? in the case, but itmight be platform-dependent. === Both patches === * Performance: I don't think those functions are not performance-critical, but we could cache typoutput functions in fn_extraif 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 carefulchoice will be preferred even of a patch. -- Itagaki Takahiro
pgsql-hackers by date: