Re: Why is pq_begintypsend so slow? - Mailing list pgsql-hackers

Hi,

In <20240218200906.zvihkrs46yl2juzf@awork3.anarazel.de>
  "Re: Why is pq_begintypsend so slow?" on Sun, 18 Feb 2024 12:09:06 -0800,
  Andres Freund <andres@anarazel.de> wrote:

>> [1]
https://www.postgresql.org/message-id/flat/20240215.153421.96888103784986803.kou%40clear-code.com#34df359e6d255795d16814ce138cc995

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

The number of updated fields of your approach and [1] are
same:

Your approach: 6 (I think that "fcinfo->isnull = false" is
needed though.)

+                fcinfo->args[0].value = CStringGetDatum(string);
+                fcinfo->args[0].isnull = false;
+                fcinfo->args[1].value = ObjectIdGetDatum(attr->typioparam);
+                fcinfo->args[1].isnull = false;
+                fcinfo->args[2].value = Int32GetDatum(attr->typmod);
+                fcinfo->args[2].isnull = false;

[1]: 6 (including "fcinfo->isnull = false")

+    fcinfo->flinfo = flinfo;
+    fcinfo->context = escontext;
+    fcinfo->isnull = false;
+    fcinfo->args[0].value = CStringGetDatum(str);
+    fcinfo->args[1].value = ObjectIdGetDatum(typioparam);
+    fcinfo->args[2].value = Int32GetDatum(typmod);


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

[1] provides some optimized abstractions, which are
implemented with lower level APIs, without breaking the
abstractions.

Note that I don't have a strong opinion how to implement
this optimization. If other developers think this approach
makes sense for this optimization, I don't object it.

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

How about adding a comment why we don't need to optimize
this case?


I don't have a strong opinion how to implement this
optimization as I said above. It seems that you like your
approach. So I withdraw [1]. Could you complete this
optimization? Can we proceed making COPY format extensible
without this optimization? It seems that it'll take a little
time to complete this optimization because your patch is
still WIP. And it seems that you can work on it after making
COPY format extensible.


Thanks,
-- 
kou