On Tue, Feb 1, 2011 at 13:08, Hitoshi Harada <umi.tan...@gmail.com> wrote: >> The third patch is attached, modifying mb routines so that they can >> receive conversion procedures as FmgrInof * and save the function >> pointer in CopyState. >> I tested it with encoding option and could not see performance slowdown. > Hmm, sorry, the patch was wrong. Correct version is attached.
Here is a brief review for the patch. * Syntax: ENCODING encoding vs. ENCODING 'encoding' We don't have to quote encoding names in the patch, but we always need quotes for CREATE DATABASE WITH ENCODING. I think we should modify CREATE DATABASE to accept unquoted encoding names aside from the patch. Changes in pg_wchar.h are the most debatable parts of the patch. The patch adds pg_cached_encoding_conversion(). We normally use pg_do_encoding_conversion(), but it is slower than the proposed function because it lookups conversion proc from catalog every call. * Can we use #ifndef FRONTEND in the header? Usage of fmgr.h members will broke client applications without the #ifdef, but I guess client apps don't always have definitions of FRONTEND. If we don't want to change pg_wchar.h, pg_conversion_fn.h might be a better header for the new API because FindDefaultConversion() is in it. Changes in copy.c looks good except a few trivial cosmetic issues: * encoding_option could be a local variable instead of cstate's member. * cstate->client_encoding is renamed to target_encoding, but I prefer file_encoding or remote_encoding. * CopyState can have conv_proc entity as a member instead of the pointer. * need_transcoding checks could be replaced with conv_proc IS NULL check. -- 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
