Re: patch (for 9.1) string functions

From: Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: Merlin Moncure <mmoncure(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: patch (for 9.1) string functions
Date: 2010-07-08 07:15:38
Message-ID: 20100708161538.FFF5.52131E4D@oss.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Fujii Masao 2010-07-08 07:51:22 Re: Proposal for 9.1: WAL streaming from WAL buffers
Previous Message Fujii Masao 2010-07-08 06:55:24 Re: keepalive in libpq using