Date: Wed, 14 Feb 2024 14:08:51 +0900 (JST)
Message-Id: <20240214.140851.211967754365096862.kou@clear-code.com>
To: michael@paquier.xyz
Cc: andres@anarazel.de, sawada.mshk@gmail.com, zhjwpku@gmail.com,
 andrew@dunslane.net, nathandbossart@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Make COPY format extendable: Extract COPY TO format
 implementations
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: <ZcwzZrrsTEJ7oJyq@paquier.xyz>
References: <20240209192705.5qdilvviq3py2voq@awork3.anarazel.de>
	<20240213.173340.1518143507526518973.kou@clear-code.com>
	<ZcwzZrrsTEJ7oJyq@paquier.xyz>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20240214.140851.211967754365096862.kou%40clear-code.com>
Precedence: bulk

Hi,

In <ZcwzZrrsTEJ7oJyq@paquier.xyz>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 14 Feb 2024 12:28:38 +0900,
  Michael Paquier <michael@paquier.xyz> wrote:

>> How about the attached patch approach? If it's a desired
>> approach, I can also write a separated patch for COPY TO.
> 
> Hmm, I have not studied that much, but my first impression was that we
> would not require any new facility in fmgr.c, but perhaps you're right
> and it's more elegant to pass a InitFunctionCallInfoData this way.

I'm not familiar with the fmgr.c related code base but it
seems that we abstract {,binary-}input function call by
fmgr.c. So I think that it's better that we follow the
design. (If there is a person who knows the fmgr.c related
code base, please help us.)

> PrepareInputFunctionCallInfo() looks OK as a name, but I'm less a fan
> of PreparedInputFunctionCallSafe() and its "Prepared" part.  How about
> something like ExecuteInputFunctionCallSafe()?

I understand the feeling. SQL uses "prepared" for "prepared
statement". There are similar function names such as
InputFunctionCall()/InputFunctionCallSafe()/DirectInputFunctionCallSafe(). They
execute (call) an input function but they use "call" not
"execute" for it... So "Execute...Call..." may be
redundant...

How about InputFunctionCallSafeWithInfo(),
InputFunctionCallSafeInfo() or
InputFunctionCallInfoCallSafe()?

> I may be able to look more at that next week, and I would surely check
> the impact of that with a simple COPY query throttled by CPU (more
> rows and more attributes the better).

Thanks!

>                            Note that I've reverted 06bd311bce24 for
> the moment, as this is just getting in the way of the main patch, and
> that was non-optimal once there is a per-row callback.

Thanks for sharing the information. I'll rebase on master
when I create the v15 patch.


>> diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
>> index 41f6bc43e4..a43c853e99 100644
>> --- a/src/backend/commands/copyfrom.c
>> +++ b/src/backend/commands/copyfrom.c
>> @@ -1691,6 +1691,10 @@ BeginCopyFrom(ParseState *pstate,
>>  	/* We keep those variables in cstate. */
>>  	cstate->in_functions = in_functions;
>>  	cstate->typioparams = typioparams;
>> +	if (cstate->opts.binary)
>> +		cstate->fcinfo = PrepareInputFunctionCallInfo();
>> +	else
>> +		cstate->fcinfo = PrepareReceiveFunctionCallInfo();
> 
> Perhaps we'd better avoid more callbacks like that, for now.

I'll not use a callback for this. I'll not change this part
after we introduce Copy{To,From}Routine. cstate->fcinfo
isn't used some custom COPY format handlers such as Apache
Arrow handler like cstate->in_functions and
cstate->typioparams. But they will be always allocated. It's
a bit wasteful for those handlers but we may not care about
it. So we can always use "if (state->opts.binary)" condition
here.

BTW... This part was wrong... Sorry... It should be:


	if (cstate->opts.binary)
		cstate->fcinfo = PrepareReceiveFunctionCallInfo();
	else
		cstate->fcinfo = PrepareInputFunctionCallInfo();


Thanks,
-- 
kou