Date: Mon, 19 Feb 2024 10:02:52 +0900 (JST)
Message-Id: <20240219.100252.1610549091537921652.kou@clear-code.com>
To: andres@anarazel.de
Cc: robertmhaas@gmail.com, laurenz.albe@cybertec.at, tgl@sss.pgh.pa.us,
 jack@jncsoftware.com, david@fetter.org, jeff.janes@gmail.com,
 pgsql-hackers@postgresql.org
Subject: Re: Why is pq_begintypsend so slow?
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: <20240218200906.zvihkrs46yl2juzf@awork3.anarazel.de>
References: <20240218015955.rmw5mcmobt5hbene@awork3.anarazel.de>
	<20240218.173809.707692226756628288.kou@clear-code.com>
	<20240218200906.zvihkrs46yl2juzf@awork3.anarazel.de>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20240219.100252.1610549091537921652.kou%40clear-code.com>
Precedence: bulk

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