Re: Why is pq_begintypsend so slow?

Hi,

On 2024-02-18 17:38:09 +0900, Sutou Kouhei wrote:
> In <20240218015955(dot)rmw5mcmobt5hbene(at)awork3(dot)anarazel(dot)de>
> "Re: Why is pq_begintypsend so slow?" on Sat, 17 Feb 2024 17:59:55 -0800,
> Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > v3-0008-wip-make-in-out-send-recv-calls-in-copy.c-cheaper.patch
>
> It seems that this is an alternative approach of [1].

Note that what I posted was a very lightly polished rebase of a ~4 year old
patchset.

> [1] https://www.postgresql.org/message-id/flat/20240215.153421.96888103784986803.kou%40clear-code.com#34df359e6d255795d16814ce138cc995
>
>
> +typedef struct CopyInAttributeInfo
> +{
> + AttrNumber num;
> + const char *name;
> +
> + Oid typioparam;
> + int32 typmod;
> +
> + FmgrInfo in_finfo;
> + union
> + {
> + FunctionCallInfoBaseData fcinfo;
> + char fcinfo_data[SizeForFunctionCallInfo(3)];
> + } in_fcinfo;
>
> Do we need one FunctionCallInfoBaseData for each attribute?
> How about sharing one FunctionCallInfoBaseData by all
> attributes like [1]?

That makes no sense to me. You're throwing away most of the possible gains by
having to update the FunctionCallInfo fields on every call. You're saving
neglegible amounts of memory at a substantial runtime cost.

> @@ -956,20 +956,47 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
>
> values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
> }
> -
> - /*
> - * If ON_ERROR is specified with IGNORE, skip rows with soft
> - * errors
> - */
> - else if (!InputFunctionCallSafe(&in_functions[m],
> - string,
> - typioparams[m],
> - att->atttypmod,
> - (Node *) cstate->escontext,
> - &values[m]))
>
> Inlining InputFuncallCallSafe() here to use pre-initialized
> fcinfo will decrease maintainability. Because we abstract
> InputFunctionCall family in fmgr.c. If we define a
> InputFunctionCall variant here, we need to change both of
> fmgr.c and here when InputFunctionCall family is changed.
> How about defining details in fmgr.c and call it here
> instead like [1]?

I'm not sure I buy that that is a problem. It's not like my approach was
actually bypassing fmgr abstractions alltogether - instead it just used the
lower level APIs, because it's a performance sensitive area.

> @@ -1966,7 +1992,7 @@ CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
> if (fld_size == -1)
> {
> *isnull = true;
> - return ReceiveFunctionCall(flinfo, NULL, typioparam, typmod);
> + return ReceiveFunctionCall(fcinfo->flinfo, NULL, attr->typioparam, attr->typmod);
>
> Why pre-initialized fcinfo isn't used here?

Because it's a prototype and because I don't think it's a common path.

Greetings,

Andres Freund