[pgpool-general: 9265] Re: Segmentation fault during shutdown
Tatsuo Ishii
ishii at postgresql.org
Sat Nov 9 14:45:49 JST 2024
Hi Emond,
> Hi,
>
> Your explanation makes sense. In the original backtrace, you can see that
> the signal handler is started from memset, which is called from
> pool_init_cp. The thing I'm worried about with your patch, is that this
> will cause pgpool to no longer inform postgresql that it's about to close
> the connection. I don't know how postgresql handles such termination
> messages, but it would make sense that it frees resources on receiving a
> termination message. These resources will most likely also be freed when
> the tcp connection is closed, but what if for some reason that fails? That
> would cause the tcp connection to become stale and keep the resources
> allocated for a very long time until the tcp connection times out (probably
> 2 hours).
PostgreSQL handles both the terminate message and broken socket (EOF
upon recv(2)) in same way. From src/backend/tcop/postgres.c around
line 4993:
/*
* 'X' means that the frontend is closing down the socket. EOF
* means unexpected loss of frontend connection. Either way,
* perform normal shutdown.
*/
case EOF:
/* for the cumulative statistics system */
pgStatSessionEndCause = DISCONNECT_CLIENT_EOF;
/* FALLTHROUGH */
case PqMsg_Terminate:
/*
Note that "PqMsg_Terminate" is actually 'X' (terminate message). So
there's no fear that some resouces remain upon broken connection
scenario (EOF). However, problem is, it may take long time before
PostgreSQL notices EOF if tcp_keepalives_idle is not set (as you said
if it's not set, most system default is likely 2 hours).
> What if pool_init_cp would set a flag to indicates a successful
> init. close_all_backend_connections could then check this flag and only
> close the connections if this flag is set. I don't know if is actually
> possible, because I'm a Java developer and things work quite different
> there. But IMHO, it makes sense to at least try to close the connections
> from the side of pgpool if this is possible.
Sounds like a good idea. I will revert the previous commit and try to
implement another patch for this.
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
> Best regards,
> Emond
>
> Op vr 8 nov 2024 om 12:23 schreef Tatsuo Ishii <ishii at postgresql.org>:
>
>> > Hi Emond,
>> > Thank you for the report. I will look into this.
>> > Best reagards,
>> > --
>> > Tatsuo Ishii
>> > SRA OSS K.K.
>> > English: http://www.sraoss.co.jp/index_en/
>> > Japanese:http://www.sraoss.co.jp
>> >> Hi,
>> >> Unfortunately, it seems the patch did not fix this issue. Yesterday we
>> had
>> >> a segmentation fault at this same point again. The top of the backtrace
>> now
>> >> is:
>> >> #0 close_all_backend_connections () at
>> protocol/pool_connection_pool.c:1082
>> >> #1 0x0000563a51f9280f in proc_exit_prepare (code=-1) at
>> >> ../../src/utils/error/elog.c:2707
>> >> #2 0x00007f0926782da7 in __funcs_on_exit () at src/exit/atexit.c:34
>> >> #3 0x00007f092677a08f in exit (code=code at entry=0) at
>> src/exit/exit.c:29
>> >> #4 0x0000563a51f4e4e2 in child_exit (code=0) at protocol/child.c:1378
>> >> #5 die (sig=3) at protocol/child.c:1174
>> >> #6 <signal handler called>
>> >> As you can see, it now crashes at line 1082 in pool_connection_pool.c,
>> >> which looks like this in our patched version:
>> >> 1074 for (i = 0; i < pool_config->max_pool; i++, p++)
>> >> 1075 {
>> >> 1076 int backend_id = in_use_backend_id(p);
>> >> 1077
>> >> 1078 if (backend_id < 0)
>> >> 1079 continue;
>> >> 1080 if (CONNECTION_SLOT(p, backend_id) == NULL)
>> >> 1081 continue;
>> >> 1082 if (CONNECTION_SLOT(p, backend_id)->sp == NULL)
>> >> 1083 continue;
>> >> 1084 if (CONNECTION_SLOT(p, backend_id)->sp->user == NULL)
>> >> 1085 continue;
>> >> 1086 pool_send_frontend_exits(p);
>> >> 1087 }
>> >> At the moment of the crash, a lot is happening at the same time. We are
>> >> reducing a cluster back to a single node. The crash happens at the very
>> >> last moment, when only the final remaining node is still up and running,
>> >> but it still is running with cluster configuration (with a watchdog and
>> 2
>> >> backends, the local one up, the remote one down). Our configuration
>> >> management then restarts the database (to force a configuration change
>> on
>> >> postgresql). Looking at the logs, this shutdown is noticed by pgpool,
>> but
>> >> the watchdog does not hold a quorum, so it cannot initiate a failover
>> >> (also, there's no backend to failover to). Then, within a second, pgpool
>> >> itself is also shutdown. This is when the process segfaults. Something
>> that
>> >> does seem interesting is that the pid (183) that segfaults, seems to be
>> >> started during the failover process. pgpool is simultaneously killing
>> all
>> >> connection pids and starting this one. Also, this pid is killed within a
>> >> single ms of being started (see timestamp 2024-11-07T00:48:06.906935
>> >> and 2024-11-07T00:48:06.907304 in the logs). I hope this helps in
>> tracking
>> >> this issue down.
>> The crash is inside close_all_backend_connections(), which is called
>> when pgpool child process is about to exit. The function should never
>> crash if the connection pool has been already initialized. The
>> initialization is done in pool_init_cp(). The function is called at
>> the very early stage of pgpool child process starts up. It allocates
>> memory by using palloc then clear the memory using memset().
>> >> connection pids and starting this one. Also, this pid is killed within a
>> >> single ms of being started (see timestamp 2024-11-07T00:48:06.906935
>> >> and 2024-11-07T00:48:06.907304 in the logs). I hope this helps in
>> tracking
>> So my guess is, the process (pid 183) was killed in the middle of
>> memset() and the connection pool object was left with uninitialized
>> data.
>> The purpose of close_all_backend_connections() is sending an 'X'
>> (terminate) message to backend telling that pgpool is about to
>> disconnect. This is a polite way from frontend/backend protocol's
>> point of view, but it's hard to make it work in the situation you
>> reported because we need to establish a way to not call
>> close_all_backend_connections() if the initialization is not
>> done. Alternative way is, to not call close_all_backend_connections()
>> at all at the process exit. This will solve the problem in simple and
>> reliable way. Disconnecting connection to backend without sending 'X'
>> message is not prohibited by the protocol, I believe. Also as far as I
>> know PostgreSQL can handle the situation (disconnected without a prior
>> 'X' message from frontend).
>> Patch attached.
>> Best reagards,
>> --
>> Tatsuo Ishii
>> SRA OSS K.K.
>> English: http://www.sraoss.co.jp/index_en/
>> Japanese:http://www.sraoss.co.jp
>> Mime-Version: 1.0
>> Content-Type: Text/X-Patch; charset=us-ascii
>> Content-Transfer-Encoding: 7bit
>> Content-Disposition: inline; filename="fix_segfault.patch"
>>
>> diff --git a/src/protocol/child.c b/src/protocol/child.c
>> index c12a5a2c1..4e2a3443b 100644
>> --- a/src/protocol/child.c
>> +++ b/src/protocol/child.c
>> @@ -1350,9 +1350,12 @@ child_will_go_down(int code, Datum arg)
>> memcached_disconnect();
>> }
>>
>> - /* let backend know now we are exiting */
>> - if (pool_connection_pool)
>> - close_all_backend_connections();
>> + /*
>> + * We used to call close_all_backend_connections() here so that we
>> send
>> + * 'X' (terminate) message to backend. However it was possible
>> that the
>> + * function is called while initializing the connection pool
>> object, which
>> + * leads to crash. So we stopped to call
>> close_all_backend_connections().
>> + */
>> }
>>
>> /*
>>
More information about the pgpool-general
mailing list