Thread: pg_stat_activity crashes
Hi, I noticed sporadic segfaults when selecting from pg_stat_activity on current HEAD. The culprit is the 53be0b1add7064ca5db3cd884302dfc3268d884e commit which added more wait info into the pg_stat_get_activity(). More specifically, the following code is broken: + proc = BackendPidGetProc(beentry->st_procpid); + wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info); This needs to check if proc is NULL. When reading the code I noticed that the new functions pg_stat_get_backend_wait_event_type() and pg_stat_get_backend_wait_event() suffer from the same problem. Here is PoC patch which fixes the problem. I am wondering if we should raise warning in the pg_stat_get_backend_wait_event_type() and pg_stat_get_backend_wait_event() like the pg_signal_backend() does when proc is NULL instead of just returning NULL which is what this patch does though. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Hello,
At Wed, 20 Apr 2016 15:14:16 +0200, Petr Jelinek <petr@2ndquadrant.com> wrote in <571780A8.4070902@2ndquadrant.com>
> I noticed sporadic segfaults when selecting from pg_stat_activity on
> current HEAD.
>
> The culprit is the 53be0b1add7064ca5db3cd884302dfc3268d884e commit
> which added more wait info into the pg_stat_get_activity(). More
> specifically, the following code is broken:
>
> + proc = BackendPidGetProc(beentry->st_procpid);
> + wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info);
>
> This needs to check if proc is NULL. When reading the code I noticed
> that the new functions pg_stat_get_backend_wait_event_type() and
> pg_stat_get_backend_wait_event() suffer from the same problem.
Good catch.
> Here is PoC patch which fixes the problem. I am wondering if we should
> raise warning in the pg_stat_get_backend_wait_event_type() and
> pg_stat_get_backend_wait_event() like the pg_signal_backend() does
> when proc is NULL instead of just returning NULL which is what this
> patch does though.
It still makes the two relevant columns in pg_stat_activity
inconsistent each other since it reads the procarray entry twice
without a lock on procarray.
The attached patch adds pgstat_get_wait_event_info to read
wait_event_info in more appropriate way. Then change
pg_stat_get_wait_event(_type) to take the wait_event_info.
Does this work for you?
We still may have an inconsistency between weit_event and query,
or beentry itself but preventing it would need to have local
copies of wait_event_info of all corresponding entries in
procarray, which will be overdone.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 41f4374..999b7e7 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -55,6 +55,7 @@#include "storage/latch.h"#include "storage/lmgr.h"#include "storage/pg_shmem.h"
+#include "storage/procarray.h"#include "storage/procsignal.h"#include "storage/sinvaladt.h"#include "utils/ascii.h"
@@ -3118,6 +3119,27 @@ pgstat_read_current_status(void)}/* ----------
+ * pgstat_get_wait_event_info() -
+ *
+ * Return the wait_event_info for a procid. 0 if the proc is no longer
+ * living or has no waiting event.
+ */
+uint32
+pgstat_get_wait_event_info(int procpid)
+{
+ uint32 ret = 0;
+ PGPROC *proc;
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+ proc = BackendPidGetProcWithLock(procpid);
+ if (proc)
+ ret = proc->wait_event_info;
+ LWLockRelease(ProcArrayLock);
+
+ return ret;
+}
+
+/* ---------- * pgstat_get_wait_event_type() - * * Return a string representing the current wait event type,
backendis
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 64c4cc4..72776ab 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -679,7 +679,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS) bool nulls[PG_STAT_GET_ACTIVITY_COLS];
LocalPgBackendStatus*local_beentry; PgBackendStatus *beentry;
- PGPROC *proc;
+ uint32 wait_event_info; const char *wait_event_type; const char *wait_event;
@@ -716,6 +716,14 @@ pg_stat_get_activity(PG_FUNCTION_ARGS) continue; }
+ /*
+ * Read wait event. This may be inconsistent with the beentry when the
+ * corresponding procarray entry has been removed or modified after
+ * the beentry was copied, but we don't need such strict consistency
+ * for this information.
+ */
+ wait_event_info = pgstat_get_wait_event_info(beentry->st_procpid);
+ /* Values available to all callers */ values[0] = ObjectIdGetDatum(beentry->st_databaseid);
values[1]= Int32GetDatum(beentry->st_procpid);
@@ -782,14 +790,13 @@ pg_stat_get_activity(PG_FUNCTION_ARGS) values[5] =
CStringGetTextDatum(beentry->st_activity);
- proc = BackendPidGetProc(beentry->st_procpid);
- wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info);
+ wait_event_type = pgstat_get_wait_event_type(wait_event_info); if (wait_event_type)
values[6] = CStringGetTextDatum(wait_event_type); else nulls[6] = true;
- wait_event = pgstat_get_wait_event(proc->wait_event_info);
+ wait_event = pgstat_get_wait_event(wait_event_info); if (wait_event) values[7] =
CStringGetTextDatum(wait_event); else
@@ -983,7 +990,6 @@ pg_stat_get_backend_wait_event_type(PG_FUNCTION_ARGS){ int32 beid = PG_GETARG_INT32(0);
PgBackendStatus *beentry;
- PGPROC *proc; const char *wait_event_type; if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
@@ -992,8 +998,8 @@ pg_stat_get_backend_wait_event_type(PG_FUNCTION_ARGS) wait_event_type = "<insufficient
privilege>"; else {
- proc = BackendPidGetProc(beentry->st_procpid);
- wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info);
+ wait_event_type = pgstat_get_wait_event_type(
+ pgstat_get_wait_event_info(beentry->st_procpid)); } if (!wait_event_type)
@@ -1007,7 +1013,6 @@ pg_stat_get_backend_wait_event(PG_FUNCTION_ARGS){ int32 beid = PG_GETARG_INT32(0);
PgBackendStatus*beentry;
- PGPROC *proc; const char *wait_event; if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
@@ -1016,8 +1021,8 @@ pg_stat_get_backend_wait_event(PG_FUNCTION_ARGS) wait_event = "<insufficient privilege>";
else {
- proc = BackendPidGetProc(beentry->st_procpid);
- wait_event = pgstat_get_wait_event(proc->wait_event_info);
+ wait_event = pgstat_get_wait_event(
+ pgstat_get_wait_event_info(beentry->st_procpid)); } if (!wait_event)
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 4be09fe..228e865 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -969,6 +969,7 @@ extern void pgstat_report_activity(BackendState state, const char *cmd_str);extern void
pgstat_report_tempfile(size_tfilesize);extern void pgstat_report_appname(const char *appname);extern void
pgstat_report_xact_timestamp(TimestampTztstamp);
+extern uint32 pgstat_get_wait_event_info(int procpid);extern const char *pgstat_get_wait_event(uint32
wait_event_info);externconst char *pgstat_get_wait_event_type(uint32 wait_event_info);extern const char
*pgstat_get_backend_current_activity(intpid, bool checkUser);
On Wed, Apr 20, 2016 at 10:56 PM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > At Wed, 20 Apr 2016 15:14:16 +0200, Petr Jelinek <petr@2ndquadrant.com> wrote in <571780A8.4070902@2ndquadrant.com> >> I noticed sporadic segfaults when selecting from pg_stat_activity on >> current HEAD. >> >> The culprit is the 53be0b1add7064ca5db3cd884302dfc3268d884e commit >> which added more wait info into the pg_stat_get_activity(). More >> specifically, the following code is broken: >> >> + proc = BackendPidGetProc(beentry->st_procpid); >> + wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info); >> >> This needs to check if proc is NULL. When reading the code I noticed >> that the new functions pg_stat_get_backend_wait_event_type() and >> pg_stat_get_backend_wait_event() suffer from the same problem. > > Good catch. Agreed. >> Here is PoC patch which fixes the problem. I am wondering if we should >> raise warning in the pg_stat_get_backend_wait_event_type() and >> pg_stat_get_backend_wait_event() like the pg_signal_backend() does >> when proc is NULL instead of just returning NULL which is what this >> patch does though. > > It still makes the two relevant columns in pg_stat_activity > inconsistent each other since it reads the procarray entry twice > without a lock on procarray. > > The attached patch adds pgstat_get_wait_event_info to read > wait_event_info in more appropriate way. Then change > pg_stat_get_wait_event(_type) to take the wait_event_info. > > Does this work for you? This is a hideously way of fixing this problem. The whole point of storing the wait event in a 4-byte integer is that we already assume reads of 4 byte integers are atomic and thus no lock is needed. The only thing we need to do is to prevent the value from being read twice, and we already have precedent for how to prevent that in freelist.c. I've committed a modified version of Petr's patch which I think should be sufficient. It survived this test on my machine: pgbench -n -f f -c 64 -j 64 -T 120 Where f contained: SELECT * FROM pg_stat_activity; -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Apr 21, 2016 at 2:04 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Apr 20, 2016 at 10:56 PM, Kyotaro HORIGUCHI > <horiguchi.kyotaro@lab.ntt.co.jp> wrote: >> At Wed, 20 Apr 2016 15:14:16 +0200, Petr Jelinek <petr@2ndquadrant.com> wrote in <571780A8.4070902@2ndquadrant.com> >>> I noticed sporadic segfaults when selecting from pg_stat_activity on >>> current HEAD. >>> >>> The culprit is the 53be0b1add7064ca5db3cd884302dfc3268d884e commit >>> which added more wait info into the pg_stat_get_activity(). More >>> specifically, the following code is broken: >>> >>> + proc = BackendPidGetProc(beentry->st_procpid); >>> + wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info); >>> >>> This needs to check if proc is NULL. When reading the code I noticed >>> that the new functions pg_stat_get_backend_wait_event_type() and >>> pg_stat_get_backend_wait_event() suffer from the same problem. >> >> Good catch. > > Agreed. > >>> Here is PoC patch which fixes the problem. I am wondering if we should >>> raise warning in the pg_stat_get_backend_wait_event_type() and >>> pg_stat_get_backend_wait_event() like the pg_signal_backend() does >>> when proc is NULL instead of just returning NULL which is what this >>> patch does though. >> >> It still makes the two relevant columns in pg_stat_activity >> inconsistent each other since it reads the procarray entry twice >> without a lock on procarray. >> >> The attached patch adds pgstat_get_wait_event_info to read >> wait_event_info in more appropriate way. Then change >> pg_stat_get_wait_event(_type) to take the wait_event_info. >> >> Does this work for you? > > This is a hideously way of fixing this problem. The whole point of > storing the wait event in a 4-byte integer is that we already assume > reads of 4 byte integers are atomic and thus no lock is needed. The > only thing we need to do is to prevent the value from being read > twice, and we already have precedent for how to prevent that in > freelist.c. That was intended to say "a hideously expensive way". -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
At Thu, 21 Apr 2016 14:05:28 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmobYX6+d695SE3-UZ1HL326at-9sr4JzQxBW3GAW4exjEA@mail.gmail.com> > >>> Here is PoC patch which fixes the problem. I am wondering if we should > >>> raise warning in the pg_stat_get_backend_wait_event_type() and > >>> pg_stat_get_backend_wait_event() like the pg_signal_backend() does > >>> when proc is NULL instead of just returning NULL which is what this > >>> patch does though. > >> > >> It still makes the two relevant columns in pg_stat_activity > >> inconsistent each other since it reads the procarray entry twice > >> without a lock on procarray. > >> > >> The attached patch adds pgstat_get_wait_event_info to read > >> wait_event_info in more appropriate way. Then change > >> pg_stat_get_wait_event(_type) to take the wait_event_info. > >> > >> Does this work for you? > > > > This is a hideously way of fixing this problem. The whole point of > > storing the wait event in a 4-byte integer is that we already assume > > reads of 4 byte integers are atomic and thus no lock is needed. A lock was already held in BackendPidGetProc(). Is it also needless? If so, we should use BackendPidGetProcWithLock() instad (the name seems a bit confusing, though). What I did in the patch was just extending the lock duration until reading the pointer proc. I didn't added any additional lock. > > The > > only thing we need to do is to prevent the value from being read > > twice, and we already have precedent for how to prevent that in > > freelist.c. However, I don't have objections for the patch applied. > That was intended to say "a hideously expensive way". Thanks for the kind suppliment.. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Apr 21, 2016 at 9:47 PM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > A lock was already held in BackendPidGetProc(). Is it also > needless? If so, we should use BackendPidGetProcWithLock() instad > (the name seems a bit confusing, though). Oh, that's really sad. No, that lock is definitely needed. We should probably try to figure out some day if there is a way to make this completely lockless, but that'll have to be 9.7 material or later. :-( > What I did in the patch was just extending the lock duration > until reading the pointer proc. I didn't added any additional > lock. Sorry, I didn't realize that. Good point. >> > The >> > only thing we need to do is to prevent the value from being read >> > twice, and we already have precedent for how to prevent that in >> > freelist.c. > > However, I don't have objections for the patch applied. OK, let's leave it like that for now, then. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hello, thank you for understanding. At Mon, 25 Apr 2016 10:26:49 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmoZDcaGCF0n9PaF5kzwj0CNRa-E+tDgzW80GVxg77gPdSA@mail.gmail.com> > On Thu, Apr 21, 2016 at 9:47 PM, Kyotaro HORIGUCHI > <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > > A lock was already held in BackendPidGetProc(). Is it also > > needless? If so, we should use BackendPidGetProcWithLock() instad > > (the name seems a bit confusing, though). > > Oh, that's really sad. No, that lock is definitely needed. We should > probably try to figure out some day if there is a way to make this > completely lockless, but that'll have to be 9.7 material or later. > :-( Agreed. > > What I did in the patch was just extending the lock duration > > until reading the pointer proc. I didn't added any additional > > lock. > > Sorry, I didn't realize that. Good point. I'm happy that you understand me:) > >> > The > >> > only thing we need to do is to prevent the value from being read > >> > twice, and we already have precedent for how to prevent that in > >> > freelist.c. > > > > However, I don't have objections for the patch applied. > > OK, let's leave it like that for now, then. regards, -- Kyotaro Horiguchi NTT Open Source Software Center