Horton

Thread

Commits

  1. Remove dead code and fix comments in fast-path function handling.

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

  1. Outdated comments around HandleFunctionRequest

    Andres Freund <andres@anarazel.de> — 2017-04-05T01:05:25Z

    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 <heikki.linnakangas@iki.fi>
Date:   2015-02-02 17:08:45 +0200

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

Heikki?


- Andres

  2. Re: Outdated comments around HandleFunctionRequest

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-05T07:58:55Z

    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

  3. Re: Outdated comments around HandleFunctionRequest

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-06T06:12:25Z

    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