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
Attachments:
fix-pgstat-proc-null.difftext/x-diff; name=fix-pgstat-proc-null.diffDownload
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 64c4cc4..355e58c 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -783,13 +783,23 @@ 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);
+ if (proc != NULL)
+ {
+ wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info);
+ wait_event = pgstat_get_wait_event(proc->wait_event_info);
+
+ }
+ else
+ {
+ wait_event_type = NULL;
+ wait_event = NULL;
+ }
+
if (wait_event_type)
values[6] = CStringGetTextDatum(wait_event_type);
else
nulls[6] = true;
- wait_event = pgstat_get_wait_event(proc->wait_event_info);
if (wait_event)
values[7] = CStringGetTextDatum(wait_event);
else
@@ -990,11 +1000,8 @@ pg_stat_get_backend_wait_event_type(PG_FUNCTION_ARGS)
wait_event_type = "<backend information not available>";
else if (!has_privs_of_role(GetUserId(), beentry->st_userid))
wait_event_type = "<insufficient privilege>";
- else
- {
- proc = BackendPidGetProc(beentry->st_procpid);
+ else if ((proc = BackendPidGetProc(beentry->st_procpid)) != NULL)
wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info);
- }
if (!wait_event_type)
PG_RETURN_NULL();
@@ -1014,11 +1021,8 @@ pg_stat_get_backend_wait_event(PG_FUNCTION_ARGS)
wait_event = "<backend information not available>";
else if (!has_privs_of_role(GetUserId(), beentry->st_userid))
wait_event = "<insufficient privilege>";
- else
- {
- proc = BackendPidGetProc(beentry->st_procpid);
+ else if ((proc = BackendPidGetProc(beentry->st_procpid)) != NULL)
wait_event = pgstat_get_wait_event(proc->wait_event_info);
- }
if (!wait_event)
PG_RETURN_NULL();
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
Attachments:
fix-pgstat-proc-null-v2.difftext/x-patch; charset=us-asciiDownload
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, backend is
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_t filesize);
extern void pgstat_report_appname(const char *appname);
extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
+extern uint32 pgstat_get_wait_event_info(int procpid);
extern const char *pgstat_get_wait_event(uint32 wait_event_info);
extern const char *pgstat_get_wait_event_type(uint32 wait_event_info);
extern const char *pgstat_get_backend_current_activity(int pid, 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers