Re: Add ENCODING option to COPY

On Tue, Feb 1, 2011 at 13:08, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)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