Date: Tue, 13 Feb 2024 17:33:40 +0900 (JST)
Message-Id: <20240213.173340.1518143507526518973.kou@clear-code.com>
To: andres@anarazel.de
Cc: michael@paquier.xyz, sawada.mshk@gmail.com, zhjwpku@gmail.com,
 andrew@dunslane.net, nathandbossart@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Make COPY format extendable: Extract COPY TO format
 implementations
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: <20240209192705.5qdilvviq3py2voq@awork3.anarazel.de>
References: <ZcMIDgkdSrz5ibvf@paquier.xyz>
	<ZcWoTr1N0GELFA9E@paquier.xyz>
	<20240209192705.5qdilvviq3py2voq@awork3.anarazel.de>
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Tue_Feb_13_17_33_40_2024_912)--"
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20240213.173340.1518143507526518973.kou%40clear-code.com>
Precedence: bulk

----Next_Part(Tue_Feb_13_17_33_40_2024_912)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

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.

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


I'll reply other comments later...


Thanks,
-- 
kou

----Next_Part(Tue_Feb_13_17_33_40_2024_912)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="prepare-callinfo.diff"

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();
 	cstate->defmap = defmap;
 	cstate->defexprs = defexprs;
 	cstate->volatile_defexprs = volatile_defexprs;
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 906756362e..e372e5efb8 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -853,6 +853,7 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 				num_defaults = cstate->num_defaults;
 	FmgrInfo   *in_functions = cstate->in_functions;
 	Oid		   *typioparams = cstate->typioparams;
+	FunctionCallInfoBaseData *fcinfo = cstate->fcinfo;
 	int			i;
 	int		   *defmap = cstate->defmap;
 	ExprState **defexprs = cstate->defexprs;
@@ -953,12 +954,13 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 			 * 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]))
+			else if (!PreparedInputFunctionCallSafe(fcinfo,
+													&in_functions[m],
+													string,
+													typioparams[m],
+													att->atttypmod,
+													(Node *) cstate->escontext,
+													&values[m]))
 			{
 				cstate->num_errors++;
 				return true;
@@ -1958,7 +1960,7 @@ CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
 	if (fld_size == -1)
 	{
 		*isnull = true;
-		return ReceiveFunctionCall(flinfo, NULL, typioparam, typmod);
+		return PreparedReceiveFunctionCall(cstate->fcinfo, flinfo, NULL, typioparam, typmod);
 	}
 	if (fld_size < 0)
 		ereport(ERROR,
@@ -1979,8 +1981,8 @@ CopyReadBinaryAttribute(CopyFromState cstate, FmgrInfo *flinfo,
 	cstate->attribute_buf.data[fld_size] = '\0';
 
 	/* Call the column type's binary input converter */
-	result = ReceiveFunctionCall(flinfo, &cstate->attribute_buf,
-								 typioparam, typmod);
+	result = PreparedReceiveFunctionCall(cstate->fcinfo, flinfo, &cstate->attribute_buf,
+										 typioparam, typmod);
 
 	/* Trouble if it didn't eat the whole buffer */
 	if (cstate->attribute_buf.cursor != cstate->attribute_buf.len)
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index e48a86be54..b0b5310219 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -1672,6 +1672,73 @@ DirectInputFunctionCallSafe(PGFunction func, char *str,
 	return true;
 }
 
+/*
+ * Prepare callinfo for PreparedInputFunctionCallSafe to reuse one callinfo
+ * instead of initializing it for each call. This is for performance.
+ */
+FunctionCallInfoBaseData *
+PrepareInputFunctionCallInfo(void)
+{
+	FunctionCallInfoBaseData *fcinfo;
+
+	fcinfo = (FunctionCallInfoBaseData *) palloc(SizeForFunctionCallInfo(3));
+	InitFunctionCallInfoData(*fcinfo, NULL, 3, InvalidOid, NULL, NULL);
+	fcinfo->args[0].isnull = false;
+	fcinfo->args[1].isnull = false;
+	fcinfo->args[2].isnull = false;
+	return fcinfo;
+}
+
+/*
+ * Call a previously-looked-up datatype input function, with prepared callinfo
+ * and non-exception handling of "soft" errors.
+ *
+ * This is basically like InputFunctionCallSafe, but it reuses prepared
+ * callinfo.
+ */
+bool
+PreparedInputFunctionCallSafe(FunctionCallInfoBaseData *fcinfo,
+							  FmgrInfo *flinfo, char *str,
+							  Oid typioparam, int32 typmod,
+							  fmNodePtr escontext,
+							  Datum *result)
+{
+	if (str == NULL && flinfo->fn_strict)
+	{
+		*result = (Datum) 0;	/* just return null result */
+		return true;
+	}
+
+	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);
+
+	*result = FunctionCallInvoke(fcinfo);
+
+	/* Result value is garbage, and could be null, if an error was reported */
+	if (SOFT_ERROR_OCCURRED(escontext))
+		return false;
+
+	/* Otherwise, should get null result if and only if str is NULL */
+	if (str == NULL)
+	{
+		if (!fcinfo->isnull)
+			elog(ERROR, "input function %u returned non-NULL",
+				 flinfo->fn_oid);
+	}
+	else
+	{
+		if (fcinfo->isnull)
+			elog(ERROR, "input function %u returned NULL",
+				 flinfo->fn_oid);
+	}
+
+	return true;
+}
+
 /*
  * Call a previously-looked-up datatype output function.
  *
@@ -1731,6 +1798,65 @@ ReceiveFunctionCall(FmgrInfo *flinfo, StringInfo buf,
 	return result;
 }
 
+/*
+ * Prepare callinfo for PreparedReceiveFunctionCall to reuse one callinfo
+ * instead of initializing it for each call. This is for performance.
+ */
+FunctionCallInfoBaseData *
+PrepareReceiveFunctionCallInfo(void)
+{
+	FunctionCallInfoBaseData *fcinfo;
+
+	fcinfo = (FunctionCallInfoBaseData *) palloc(SizeForFunctionCallInfo(3));
+	InitFunctionCallInfoData(*fcinfo, NULL, 3, InvalidOid, NULL, NULL);
+	fcinfo->args[0].isnull = false;
+	fcinfo->args[1].isnull = false;
+	fcinfo->args[2].isnull = false;
+	return fcinfo;
+}
+
+/*
+ * Call a previously-looked-up datatype binary-input function, with prepared
+ * callinfo.
+ *
+ * This is basically like ReceiveFunctionCall, but it reuses prepared
+ * callinfo.
+ */
+Datum
+PreparedReceiveFunctionCall(FunctionCallInfoBaseData *fcinfo,
+							FmgrInfo *flinfo, StringInfo buf,
+							Oid typioparam, int32 typmod)
+{
+	Datum		result;
+
+	if (buf == NULL && flinfo->fn_strict)
+		return (Datum) 0;		/* just return null result */
+
+	fcinfo->flinfo = flinfo;
+	fcinfo->isnull = false;
+	fcinfo->args[0].value = PointerGetDatum(buf);
+	fcinfo->args[1].value = ObjectIdGetDatum(typioparam);
+	fcinfo->args[2].value = Int32GetDatum(typmod);
+
+	result = FunctionCallInvoke(fcinfo);
+
+	/* Should get null result if and only if buf is NULL */
+	if (buf == NULL)
+	{
+		if (!fcinfo->isnull)
+			elog(ERROR, "receive function %u returned non-NULL",
+				 flinfo->fn_oid);
+	}
+	else
+	{
+		if (fcinfo->isnull)
+			elog(ERROR, "receive function %u returned NULL",
+				 flinfo->fn_oid);
+	}
+
+	return result;
+}
+
 /*
  * Call a previously-looked-up datatype binary-output function.
  *
diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index 759f8e3d09..4d7928b3ac 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -104,6 +104,7 @@ typedef struct CopyFromStateData
 	Oid		   *typioparams;	/* array of element types for in_functions */
 	ErrorSaveContext *escontext;	/* soft error trapper during in_functions
 									 * execution */
+	FunctionCallInfoBaseData *fcinfo;	/* reusable callinfo for in_functions */
 	uint64		num_errors;		/* total number of rows which contained soft
 								 * errors */
 	int		   *defmap;			/* array of default att numbers related to
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index ccb4070a25..994d8ce487 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -708,12 +708,24 @@ extern bool DirectInputFunctionCallSafe(PGFunction func, char *str,
 										Oid typioparam, int32 typmod,
 										fmNodePtr escontext,
 										Datum *result);
+extern FunctionCallInfoBaseData *PrepareInputFunctionCallInfo(void);
+extern bool
+			PreparedInputFunctionCallSafe(FunctionCallInfoBaseData *fcinfo,
+										  FmgrInfo *flinfo, char *str,
+										  Oid typioparam, int32 typmod,
+										  fmNodePtr escontext,
+										  Datum *result);
 extern Datum OidInputFunctionCall(Oid functionId, char *str,
 								  Oid typioparam, int32 typmod);
 extern char *OutputFunctionCall(FmgrInfo *flinfo, Datum val);
 extern char *OidOutputFunctionCall(Oid functionId, Datum val);
 extern Datum ReceiveFunctionCall(FmgrInfo *flinfo, fmStringInfo buf,
 								 Oid typioparam, int32 typmod);
+extern FunctionCallInfoBaseData *PrepareReceiveFunctionCallInfo(void);
+extern Datum
+			PreparedReceiveFunctionCall(FunctionCallInfoBaseData *fcinfo,
+										FmgrInfo *flinfo, fmStringInfo buf,
+										Oid typioparam, int32 typmod);
 extern Datum OidReceiveFunctionCall(Oid functionId, fmStringInfo buf,
 									Oid typioparam, int32 typmod);
 extern bytea *SendFunctionCall(FmgrInfo *flinfo, Datum val);

----Next_Part(Tue_Feb_13_17_33_40_2024_912)----