Re: Make COPY format extendable: Extract COPY TO format implementations - Mailing list pgsql-hackers

On Tue, Feb 13, 2024 at 05:33:40PM +0900, Sutou Kouhei wrote:
> Hi,
>
> In <20240209192705.5qdilvviq3py2voq@awork3.anarazel.de>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 9 Feb 2024 11:27:05 -0800,
>   Andres Freund <andres@anarazel.de> wrote:
>
>>> +static void
>>> +CopyFromTextInFunc(CopyFromState cstate, Oid atttypid,
>>> +                   FmgrInfo *finfo, Oid *typioparam)
>>> +{
>>> +    Oid            func_oid;
>>> +
>>> +    getTypeInputInfo(atttypid, &func_oid, typioparam);
>>> +    fmgr_info(func_oid, finfo);
>>> +}
>>
>> FWIW, we should really change the copy code to initialize FunctionCallInfoData
>> instead of re-initializing that on every call, realy makes a difference
>> performance wise.
>
> 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.

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

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).

>>> +    cstate->raw_fields = (char **) palloc(attr_count * sizeof(char *));
>>> +    /* Set read attribute callback */
>>> +    if (cstate->opts.csv_mode)
>>> +        cstate->copy_read_attributes = CopyReadAttributesCSV;
>>> +    else
>>> +        cstate->copy_read_attributes = CopyReadAttributesText;
>>> +}
>>
>> Isn't this precisely repeating the mistake of 2889fd23be56?
>
> What do you think about the approach in my previous mail's
> attachments?
>
https://www.postgresql.org/message-id/flat/20240209.163205.704848659612151781.kou%40clear-code.com#dbb1f8d7f2f0e8fe3c7e37a757fcfc54
>
> If it's a desired approach, I can prepare a v15 patch set
> based on the v14 patch set and the approach.

Yes, this one looks like it's using the right angle: we don't rely
anymore in cstate to decide which CopyReadAttributes to use, the
routines do that instead.  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.

> 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.
--
Michael