Re: [HACKERS] Outdated comments around HandleFunctionRequest

2017-04-06 Thread Heikki Linnakangas 

On 04/05/2017 10:58 AM, Heikki Linnakangas wrote:

On 04/05/2017 04:05 AM, Andres Freund wrote:

PostgresMain() has the following blurb for fastpath functions:

/*
 * Note: we may at this point be inside an 
aborted
 * transaction.  We can't throw error for that 
until we've
 * finished reading the function-call message, 
so
 * HandleFunctionRequest() must check for it 
after doing so.
 * Be careful not to do anything that assumes 
we're inside a
 * valid transaction here.
 */
and in HandleFunctionRequest() there's:

 * INPUT:
 *  In protocol version 3, postgres.c has already read the message 
body
 *  and will pass it in msgBuf.
 *  In old protocol, the passed msgBuf is empty and we must read the
 *  message here.

which is not true anymore.  Followed by:

/*
 * Now that we've eaten the input message, check to see if we actually
 * want to do the function call or not.  It's now safe to ereport(); we
 * won't lose sync with the frontend.
 */

which is also not really meaningful, because there's no previous code in
the function.


You're right, I missed those comments in commit 2b3a8b20c2.

In fact, HandleFunctionRequest() now always return 0, so we can also
remove the dead code to check the return value from the caller. Barring
objections, I'll commit the attached to do that and fix the comments.


Committed, thanks!

- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Outdated comments around HandleFunctionRequest

2017-04-05 Thread Heikki Linnakangas 

On 04/05/2017 04:05 AM, Andres Freund wrote:

PostgresMain() has the following blurb for fastpath functions:

/*
 * Note: we may at this point be inside an 
aborted
 * transaction.  We can't throw error for that 
until we've
 * finished reading the function-call message, 
so
 * HandleFunctionRequest() must check for it 
after doing so.
 * Be careful not to do anything that assumes 
we're inside a
 * valid transaction here.
 */
and in HandleFunctionRequest() there's:

 * INPUT:
 *  In protocol version 3, postgres.c has already read the message 
body
 *  and will pass it in msgBuf.
 *  In old protocol, the passed msgBuf is empty and we must read the
 *  message here.

which is not true anymore.  Followed by:

/*
 * Now that we've eaten the input message, check to see if we actually
 * want to do the function call or not.  It's now safe to ereport(); we
 * won't lose sync with the frontend.
 */

which is also not really meaningful, because there's no previous code in
the function.


You're right, I missed those comments in commit 2b3a8b20c2.

In fact, HandleFunctionRequest() now always return 0, so we can also 
remove the dead code to check the return value from the caller. Barring 
objections, I'll commit the attached to do that and fix the comments.


- Heikki

diff --git a/src/backend/tcop/fastpath.c b/src/backend/tcop/fastpath.c
index 2efed95132..d29c9d81ad 100644
--- a/src/backend/tcop/fastpath.c
+++ b/src/backend/tcop/fastpath.c
@@ -249,24 +249,15 @@ fetch_fp_info(Oid func_id, struct fp_info * fip)
  * This corresponds to the libpq protocol symbol "F".
  *
  * INPUT:
- *		In protocol version 3, postgres.c has already read the message body
- *		and will pass it in msgBuf.
- *		In old protocol, the passed msgBuf is empty and we must read the
- *		message here.
- *
- * RETURNS:
- *		0 if successful completion, EOF if frontend connection lost.
- *
- * Note: All ordinary errors result in ereport(ERROR,...).  However,
- * if we lose the frontend connection there is no one to ereport to,
- * and no use in proceeding...
+ *		postgres.c has already read the message body and will pass it in
+ *		msgBuf.
  *
  * Note: palloc()s done here and in the called function do not need to be
  * cleaned up explicitly.  We are called from PostgresMain() in the
  * MessageContext memory context, which will be automatically reset when
  * control returns to PostgresMain.
  */
-int
+void
 HandleFunctionRequest(StringInfo msgBuf)
 {
 	Oid			fid;
@@ -281,9 +272,8 @@ HandleFunctionRequest(StringInfo msgBuf)
 	char		msec_str[32];
 
 	/*
-	 * Now that we've eaten the input message, check to see if we actually
-	 * want to do the function call or not.  It's now safe to ereport(); we
-	 * won't lose sync with the frontend.
+	 * We can only accept COMMIT/ABORT if we are in an aborted transaction,
+	 * and COMMIT/ABORT cannot be executed through the fastpath interface.
 	 */
 	if (IsAbortedTransactionBlockState())
 		ereport(ERROR,
@@ -406,8 +396,6 @@ HandleFunctionRequest(StringInfo msgBuf)
 			msec_str, fip->fname, fid)));
 			break;
 	}
-
-	return 0;
 }
 
 /*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 6258a14c39..92beb316c5 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4160,19 +4160,7 @@ PostgresMain(int argc, char *argv[],
 /* switch back to message context */
 MemoryContextSwitchTo(MessageContext);
 
-if (HandleFunctionRequest(_message) == EOF)
-{
-	/* lost frontend connection during F message input */
-
-	/*
-	 * Reset whereToSendOutput to prevent ereport from
-	 * attempting to send any more messages to client.
-	 */
-	if (whereToSendOutput == DestRemote)
-		whereToSendOutput = DestNone;
-
-	proc_exit(0);
-}
+HandleFunctionRequest(_message);
 
 /* commit the function-invocation transaction */
 finish_xact_command();
diff --git a/src/include/tcop/fastpath.h b/src/include/tcop/fastpath.h
index af1e330f18..81d94913db 100644
--- a/src/include/tcop/fastpath.h
+++ b/src/include/tcop/fastpath.h
@@ -16,6 +16,6 @@
 #include "lib/stringinfo.h"
 
 extern int	GetOldFunctionMessage(StringInfo buf);
-extern int	HandleFunctionRequest(StringInfo msgBuf);
+extern void HandleFunctionRequest(StringInfo msgBuf);
 
 #endif   /* FASTPATH_H */

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Outdated comments around HandleFunctionRequest

2017-04-04 Thread Andres Freund 
Hi,

PostgresMain() has the following blurb for fastpath functions:

/*
 * Note: we may at this point be inside an 
aborted
 * transaction.  We can't throw error for that 
until we've
 * finished reading the function-call message, 
so
 * HandleFunctionRequest() must check for it 
after doing so.
 * Be careful not to do anything that assumes 
we're inside a
 * valid transaction here.
 */
and in HandleFunctionRequest() there's:

 * INPUT:
 *  In protocol version 3, postgres.c has already read the message 
body
 *  and will pass it in msgBuf.
 *  In old protocol, the passed msgBuf is empty and we must read the
 *  message here.

which is not true anymore.  Followed by:

/*
 * Now that we've eaten the input message, check to see if we actually
 * want to do the function call or not.  It's now safe to ereport(); we
 * won't lose sync with the frontend.
 */

which is also not really meaningful, because there's no previous code in
the function.

This largely seems to be damage from


commit 2b3a8b20c2da9f39ffecae25ab7c66974fbc0d3b
Author: Heikki Linnakangas 
Date:   2015-02-02 17:08:45 +0200

Be more careful to not lose sync in the FE/BE protocol.

Heikki?


- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers