Resetting PGPROC atomics in ProcessInit()
Hi,
I just noticed, while working on a patch adding things to PGPROC, that
the group clearning patches for the proc array and clog reset atomics in
InitProcess().
I'm not a big fan of that, because it means that it's not safe to look
at the atomics of backends that aren't currently in use. Is there any
reason to not instead initialize them in InitProcGlobal() and just
assert in InitProcess() that they're 0? If they're not, we'd be in deep
trouble anyway, no?
Greetings,
Andres Freund
On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
I just noticed, while working on a patch adding things to PGPROC, that
the group clearning patches for the proc array and clog reset atomics in
InitProcess().I'm not a big fan of that, because it means that it's not safe to look
at the atomics of backends that aren't currently in use. Is there any
reason to not instead initialize them in InitProcGlobal() and just
assert in InitProcess() that they're 0?
It seems the code written has followed a natural practice i.e PGPROC
members are initialized in InitProcess and ProcGlobal members (like
procArrayGroupFirst) are initialized in InitProcGlobal. For your use
case, can't you look at procArrayGroupFirst? If not, then I think we
can do what you are saying as I don't see a problem in initializing
them in InitProcGlobal.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On October 27, 2018 3:36:45 PM GMT+01:00, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <andres@anarazel.de>
wrote:Hi,
I just noticed, while working on a patch adding things to PGPROC,
that
the group clearning patches for the proc array and clog reset atomics
in
InitProcess().
I'm not a big fan of that, because it means that it's not safe to
look
at the atomics of backends that aren't currently in use. Is there
any
reason to not instead initialize them in InitProcGlobal() and just
assert in InitProcess() that they're 0?It seems the code written has followed a natural practice i.e PGPROC
members are initialized in InitProcess and ProcGlobal members (like
procArrayGroupFirst) are initialized in InitProcGlobal. For your use
case, can't you look at procArrayGroupFirst? If not, then I think we
can do what you are saying as I don't see a problem in initializing
them in InitProcGlobal.
In my opinion that's an argument for resetting the contents with pg_atomic_write, but not reinitializing the atomic (which could reset the spinlock inside while somebody else holds it).
It's not really a problem for me, but I think the code is pretty much wrong like this...
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Sat, Oct 27, 2018 at 8:52 PM Andres Freund <andres@anarazel.de> wrote:
On October 27, 2018 3:36:45 PM GMT+01:00, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <andres@anarazel.de>
wrote:Hi,
I just noticed, while working on a patch adding things to PGPROC,
that
the group clearning patches for the proc array and clog reset atomics
in
InitProcess().
I'm not a big fan of that, because it means that it's not safe to
look
at the atomics of backends that aren't currently in use. Is there
any
reason to not instead initialize them in InitProcGlobal() and just
assert in InitProcess() that they're 0?It seems the code written has followed a natural practice i.e PGPROC
members are initialized in InitProcess and ProcGlobal members (like
procArrayGroupFirst) are initialized in InitProcGlobal. For your use
case, can't you look at procArrayGroupFirst? If not, then I think we
can do what you are saying as I don't see a problem in initializing
them in InitProcGlobal.In my opinion that's an argument for resetting the contents with pg_atomic_write, but not reinitializing the atomic
Okay, makes sense.
(which could reset the spinlock inside while somebody else holds it).
This part is not clear to me, how can this happen? I think we only
access these variable for active procs which means no-one can hold it
till it's reinitialized.
It's not really a problem for me, but I think the code is pretty much wrong like this...
I think I understand why it is better to write the way you are
suggesting, but not clear how the current code can lead to a problem,
can you please explain in more detail?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Sat, Oct 27, 2018 at 9:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Oct 27, 2018 at 8:52 PM Andres Freund <andres@anarazel.de> wrote:
On October 27, 2018 3:36:45 PM GMT+01:00, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <andres@anarazel.de>
wrote:Hi,
I just noticed, while working on a patch adding things to PGPROC,
that
the group clearning patches for the proc array and clog reset atomics
in
InitProcess().
I'm not a big fan of that, because it means that it's not safe to
look
at the atomics of backends that aren't currently in use. Is there
any
reason to not instead initialize them in InitProcGlobal() and just
assert in InitProcess() that they're 0?It seems the code written has followed a natural practice i.e PGPROC
members are initialized in InitProcess and ProcGlobal members (like
procArrayGroupFirst) are initialized in InitProcGlobal. For your use
case, can't you look at procArrayGroupFirst? If not, then I think we
can do what you are saying as I don't see a problem in initializing
them in InitProcGlobal.In my opinion that's an argument for resetting the contents with pg_atomic_write, but not reinitializing the atomic
Okay, makes sense.
(which could reset the spinlock inside while somebody else holds it).
This part is not clear to me, how can this happen? I think we only
access these variable for active procs which means no-one can hold it
till it's reinitialized.It's not really a problem for me, but I think the code is pretty much wrong like this...
I think I understand why it is better to write the way you are
suggesting, but not clear how the current code can lead to a problem,
can you please explain in more detail?
You haven't confirmed on this part.
Do you want to see this change? I think if we make this change, we
should backport this as well and I am not sure if we should make such
a change without a strong reason in back-branches.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Sat, Oct 27, 2018 at 6:41 AM Andres Freund <andres@anarazel.de> wrote:
I just noticed, while working on a patch adding things to PGPROC, that
the group clearning patches for the proc array and clog reset atomics in
InitProcess().I'm not a big fan of that, because it means that it's not safe to look
at the atomics of backends that aren't currently in use. Is there any
reason to not instead initialize them in InitProcGlobal() and just
assert in InitProcess() that they're 0? If they're not, we'd be in deep
trouble anyway, no?
I think you are correct. I think it would be better in general for
InitProcess() to Assert() rather than reinitializing. Apart from this
issue, it's not free.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Sun, Nov 4, 2018 at 6:30 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Oct 27, 2018 at 6:41 AM Andres Freund <andres@anarazel.de> wrote:
I just noticed, while working on a patch adding things to PGPROC, that
the group clearning patches for the proc array and clog reset atomics in
InitProcess().I'm not a big fan of that, because it means that it's not safe to look
at the atomics of backends that aren't currently in use. Is there any
reason to not instead initialize them in InitProcGlobal() and just
assert in InitProcess() that they're 0? If they're not, we'd be in deep
trouble anyway, no?I think you are correct. I think it would be better in general for
InitProcess() to Assert() rather than reinitializing.
Okay, changed the code as per Andres's and your suggestion. Do you
think the attached change makes sense? I think we should backpatch
this.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
fix_atomic_init_group_clear_v1.patchapplication/octet-stream; name=fix_atomic_init_group_clear_v1.patchDownload
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 6f9aaa52fa..6ad504453b 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -267,6 +267,13 @@ InitProcGlobal(void)
/* Initialize lockGroupMembers list. */
dlist_init(&procs[i].lockGroupMembers);
+
+ /*
+ * Initialize the atomic variables, otherwise, it won't be safe to
+ * access them for backends that aren't currently in use.
+ */
+ pg_atomic_init_u32(&(procs[i].procArrayGroupNext), INVALID_PGPROCNO);
+ pg_atomic_init_u32(&(procs[i].clogGroupNext), INVALID_PGPROCNO);
}
/*
@@ -401,7 +408,7 @@ InitProcess(void)
/* Initialize fields for group XID clearing. */
MyProc->procArrayGroupMember = false;
MyProc->procArrayGroupMemberXid = InvalidTransactionId;
- pg_atomic_init_u32(&MyProc->procArrayGroupNext, INVALID_PGPROCNO);
+ Assert(pg_atomic_read_u32(&MyProc->procArrayGroupNext) == INVALID_PGPROCNO);
/* Check that group locking fields are in a proper initial state. */
Assert(MyProc->lockGroupLeader == NULL);
@@ -416,7 +423,7 @@ InitProcess(void)
MyProc->clogGroupMemberXidStatus = TRANSACTION_STATUS_IN_PROGRESS;
MyProc->clogGroupMemberPage = -1;
MyProc->clogGroupMemberLsn = InvalidXLogRecPtr;
- pg_atomic_init_u32(&MyProc->clogGroupNext, INVALID_PGPROCNO);
+ Assert(pg_atomic_read_u32(&MyProc->clogGroupNext) == INVALID_PGPROCNO);
/*
* Acquire ownership of the PGPROC's latch, so that we can use WaitLatch
On Thu, Nov 8, 2018 at 4:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sun, Nov 4, 2018 at 6:30 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Oct 27, 2018 at 6:41 AM Andres Freund <andres@anarazel.de> wrote:
I just noticed, while working on a patch adding things to PGPROC, that
the group clearning patches for the proc array and clog reset atomics in
InitProcess().I'm not a big fan of that, because it means that it's not safe to look
at the atomics of backends that aren't currently in use. Is there any
reason to not instead initialize them in InitProcGlobal() and just
assert in InitProcess() that they're 0? If they're not, we'd be in deep
trouble anyway, no?I think you are correct. I think it would be better in general for
InitProcess() to Assert() rather than reinitializing.Okay, changed the code as per Andres's and your suggestion. Do you
think the attached change makes sense? I think we should backpatch
this.
For 10 and 9.6, we need a slightly different patch as the change of
group clog update went in 11. Attached are the patches for the same,
note that there is a slight change in the commit message for the patch
written for 10 and 9.6. I will wait for a few days (till Tuesday@8:00
AM IST) to see if somebody has any comments or want to review before I
push.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
0001-Fix-the-initialization-of-atomic-variables-introduce.patchapplication/octet-stream; name=0001-Fix-the-initialization-of-atomic-variables-introduce.patchDownload
From 8686ae4d71a9d3aab9acc7aa515880e9aa1b75b1 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 9 Nov 2018 09:30:59 +0530
Subject: [PATCH] Fix the initialization of atomic variables introduced by the
group clearing mechanism.
Commits 0e141c0fbb and baaf272ac9 introduced initialization of atomic
variables in InitProcess which means that it's not safe to look at those
for backends that aren't currently in use. Fix that by initializing them
during postmaster startup.
Reported-by: Andres Freund
Author: Amit Kapila
Backpatch-through: 9.6
Discussion: https://postgr.es/m/20181027104138.qmbbelopvy7cw2qv@alap3.anarazel.de
---
src/backend/storage/lmgr/proc.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 6f9aaa5..6ad5044 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -267,6 +267,13 @@ InitProcGlobal(void)
/* Initialize lockGroupMembers list. */
dlist_init(&procs[i].lockGroupMembers);
+
+ /*
+ * Initialize the atomic variables, otherwise, it won't be safe to
+ * access them for backends that aren't currently in use.
+ */
+ pg_atomic_init_u32(&(procs[i].procArrayGroupNext), INVALID_PGPROCNO);
+ pg_atomic_init_u32(&(procs[i].clogGroupNext), INVALID_PGPROCNO);
}
/*
@@ -401,7 +408,7 @@ InitProcess(void)
/* Initialize fields for group XID clearing. */
MyProc->procArrayGroupMember = false;
MyProc->procArrayGroupMemberXid = InvalidTransactionId;
- pg_atomic_init_u32(&MyProc->procArrayGroupNext, INVALID_PGPROCNO);
+ Assert(pg_atomic_read_u32(&MyProc->procArrayGroupNext) == INVALID_PGPROCNO);
/* Check that group locking fields are in a proper initial state. */
Assert(MyProc->lockGroupLeader == NULL);
@@ -416,7 +423,7 @@ InitProcess(void)
MyProc->clogGroupMemberXidStatus = TRANSACTION_STATUS_IN_PROGRESS;
MyProc->clogGroupMemberPage = -1;
MyProc->clogGroupMemberLsn = InvalidXLogRecPtr;
- pg_atomic_init_u32(&MyProc->clogGroupNext, INVALID_PGPROCNO);
+ Assert(pg_atomic_read_u32(&MyProc->clogGroupNext) == INVALID_PGPROCNO);
/*
* Acquire ownership of the PGPROC's latch, so that we can use WaitLatch
--
1.8.3.1
0001-Fix-the-initialization-of-atomic-variable-introduced-10.patchapplication/octet-stream; name=0001-Fix-the-initialization-of-atomic-variable-introduced-10.patchDownload
From c76734f1b8ee9b1a4496eda1f6bb3c66499f6217 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 9 Nov 2018 09:58:27 +0530
Subject: [PATCH] Fix the initialization of atomic variable introduced by the
group clearing mechanism.
Commit 0e141c0fbb introduced initialization of atomic variable in
InitProcess which means that it's not safe to look at it for backends that
aren't currently in use. Fix that by initializing them during postmaster
startup.
Reported-by: Andres Freund
Author: Amit Kapila
Backpatch-through: 9.6
Discussion: https://postgr.es/m/20181027104138.qmbbelopvy7cw2qv@alap3.anarazel.de
---
src/backend/storage/lmgr/proc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index bfa8499..5188264 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -266,6 +266,12 @@ InitProcGlobal(void)
/* Initialize lockGroupMembers list. */
dlist_init(&procs[i].lockGroupMembers);
+
+ /*
+ * Initialize the atomic variable, otherwise, it won't be safe to
+ * access it for backends that aren't currently in use.
+ */
+ pg_atomic_init_u32(&(procs[i].procArrayGroupNext), INVALID_PGPROCNO);
}
/*
@@ -399,7 +405,7 @@ InitProcess(void)
/* Initialize fields for group XID clearing. */
MyProc->procArrayGroupMember = false;
MyProc->procArrayGroupMemberXid = InvalidTransactionId;
- pg_atomic_init_u32(&MyProc->procArrayGroupNext, INVALID_PGPROCNO);
+ Assert(pg_atomic_read_u32(&MyProc->procArrayGroupNext) == INVALID_PGPROCNO);
/* Check that group locking fields are in a proper initial state. */
Assert(MyProc->lockGroupLeader == NULL);
--
1.8.3.1
On Fri, Nov 9, 2018 at 10:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Nov 8, 2018 at 4:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sun, Nov 4, 2018 at 6:30 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Oct 27, 2018 at 6:41 AM Andres Freund <andres@anarazel.de> wrote:
Okay, changed the code as per Andres's and your suggestion. Do you
think the attached change makes sense? I think we should backpatch
this.For 10 and 9.6, we need a slightly different patch as the change of
group clog update went in 11. Attached are the patches for the same,
note that there is a slight change in the commit message for the patch
written for 10 and 9.6. I will wait for a few days (till Tuesday@8:00
AM IST) to see if somebody has any comments or want to review before I
push.
Pushed.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com