Date: Sun, 18 Feb 2024 17:38:09 +0900 (JST)
Message-Id: <20240218.173809.707692226756628288.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: <20240218015955.rmw5mcmobt5hbene@awork3.anarazel.de>
References: <20200731170008.dp2q5467zlskh6ra@alap3.anarazel.de>
	<CA+Tgmoa0NkJhTpo8k--joaT=L1kVovK5YozzRjf9eEJdnu5etA@mail.gmail.com>
	<20240218015955.rmw5mcmobt5hbene@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/20240218.173809.707692226756628288.kou%40clear-code.com>
Precedence: bulk

Hi,

In <20240218015955.rmw5mcmobt5hbene@awork3.anarazel.de>
  "Re: Why is pq_begintypsend so slow?" on Sat, 17 Feb 2024 17:59:55 -0800,
  Andres Freund <andres@anarazel.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].

[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]?


@@ -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]?

+				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;

I think that "fcinfo->isnull = false;" is also needed like
[1].

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


Thanks,
-- 
kou