From: Tom Lane <tgl@sss.pgh.pa.us>
To: Sergey Shinderuk <s.shinderuk@postgrespro.ru>
cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: gcc -Wclobbered in PostgresMain
In-reply-to: <2eda015b-7dff-47fd-d5e2-f1a9899b90a6@postgrespro.ru>
References: <2eda015b-7dff-47fd-d5e2-f1a9899b90a6@postgrespro.ru>
Comments: In-reply-to Sergey Shinderuk <s.shinderuk@postgrespro.ru>
	message dated "Fri, 07 Jul 2023 15:13:14 +0300"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1631917.1688828909.0@sss.pgh.pa.us>
Date: Sat, 08 Jul 2023 11:11:22 -0400
Message-ID: <1632266.1688829082@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1632266.1688829082%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1631917.1688828909.1@sss.pgh.pa.us>

Sergey Shinderuk <s.shinderuk@postgrespro.ru> writes:
> While analyzing -Wclobbered warnings from gcc we found a true one in 
> PostgresMain():
> ...
> These variables must be declared volatile, because they are read after 
> longjmp(). send_ready_for_query declared there is volatile.

Yeah, you're on to something there.

> Without volatile, these variables are kept in registers and restored by 
> longjump(). I think, this is harmless because the error handling code 
> calls disable_all_timeouts() anyway.

Hmm.  So what could happen (if these *aren't* in registers) is that we
might later uselessly call disable_timeout to get rid of timeouts that
are long gone anyway.  While that's not terribly expensive, it's not
great either.  What we ought to be doing is resetting these two flags
after the disable_all_timeouts call.

Having done that, it wouldn't really be necessary to mark these
as volatile.  I kept that marking anyway for consistency with 
send_ready_for_query, but perhaps we shouldn't?

> I also moved firstchar's declaration inside the loop where it's used, to 
> make it clear that this variable needn't be volatile and is not 
> preserved after longjmp().

Good idea, but then why not the same for input_message?  It's fully
reinitialized each time through the loop, too.

In short, something like the attached, except I'm not totally sold
on changing the volatility of the timeout flags.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="v2-fix-PostgresMain-local-vars.patch";
	charset="us-ascii"
Content-ID: <1631917.1688828909.2@sss.pgh.pa.us>
Content-Description: v2-fix-PostgresMain-local-vars.patch

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 01b6cc1f7d..d18018671d 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4111,12 +4111,12 @@ PostgresSingleUserMain(int argc, char *argv[],
 void
 PostgresMain(const char *dbname, const char *username)
 {
-	int			firstchar;
-	StringInfoData input_message;
 	sigjmp_buf	local_sigjmp_buf;
+
+	/* these must be volatile to ensure state is preserved across longjmp: */
 	volatile bool send_ready_for_query = true;
-	bool		idle_in_transaction_timeout_enabled = false;
-	bool		idle_session_timeout_enabled = false;
+	volatile bool idle_in_transaction_timeout_enabled = false;
+	volatile bool idle_session_timeout_enabled = false;
 
 	Assert(dbname != NULL);
 	Assert(username != NULL);
@@ -4324,6 +4324,8 @@ PostgresMain(const char *dbname, const char *username)
 		 */
 		disable_all_timeouts(false);
 		QueryCancelPending = false; /* second to avoid race condition */
+		idle_in_transaction_timeout_enabled = false;
+		idle_session_timeout_enabled = false;
 
 		/* Not reading from the client anymore. */
 		DoingCommandRead = false;
@@ -4418,6 +4420,9 @@ PostgresMain(const char *dbname, const char *username)
 
 	for (;;)
 	{
+		int			firstchar;
+		StringInfoData input_message;
+
 		/*
 		 * At top of loop, reset extended-query-message flag, so that any
 		 * errors encountered in "idle" state don't provoke skip.

------- =_aaaaaaaaaa0--