Comment patch for bgworker.c
The comment for the BackgroundWorkerSlot structure tripped me up reviewing Robert's background worker patch; it made it clear that you need to use a memory barrier before setting in_use, but normally you'd never need to worry about that because RegisterDynamicBackgroundWorker() handles it for you. Patch adds a comment to that effect.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
Attachments:
patchtext/plain; charset=UTF-8; name=patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 85a3b3a..e81dbc1 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -55,7 +55,8 @@ slist_head BackgroundWorkerList = SLIST_STATIC_INIT(BackgroundWorkerList);
* responsibility of the postmaster. Regular backends may no longer modify it,
* but the postmaster may examine it. Thus, a backend initializing a slot
* must fully initialize the slot - and insert a write memory barrier - before
- * marking it as in use.
+ * marking it as in use. Note that RegisterDynamicBackgroundWorker() handles
+ * in_use correctly for you.
*
* As an exception, however, even when the slot is in use, regular backends
* may set the 'terminate' flag for a slot, telling the postmaster not
On Fri, Oct 24, 2014 at 8:51 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
The comment for the BackgroundWorkerSlot structure tripped me up reviewing
Robert's background worker patch; it made it clear that you need to use a
memory barrier before setting in_use, but normally you'd never need to worry
about that because RegisterDynamicBackgroundWorker() handles it for you.
Patch adds a comment to that effect.
I vote to reject this patch. I think it's explaining something that
doesn't really need to be explained, and shouldn't be explained like
this even if it does. It adds a comment that reads "Note that
RegisterDynamicBackgroundWorker() handles in_use correctly for you".
But the long block comment of which it is a part is entirely devoted
to explaining concerns internal to bgworker.c, from which I think it
should be inferred that all of the public APIs in that file handle all
of the things in that paragraph correctly (or are intended to,
anyway).
--
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 2/2/15 7:49 AM, Robert Haas wrote:
On Fri, Oct 24, 2014 at 8:51 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
The comment for the BackgroundWorkerSlot structure tripped me up reviewing
Robert's background worker patch; it made it clear that you need to use a
memory barrier before setting in_use, but normally you'd never need to worry
about that because RegisterDynamicBackgroundWorker() handles it for you.
Patch adds a comment to that effect.I vote to reject this patch. I think it's explaining something that
doesn't really need to be explained, and shouldn't be explained like
this even if it does. It adds a comment that reads "Note that
RegisterDynamicBackgroundWorker() handles in_use correctly for you".
But the long block comment of which it is a part is entirely devoted
to explaining concerns internal to bgworker.c, from which I think it
should be inferred that all of the public APIs in that file handle all
of the things in that paragraph correctly (or are intended to,
anyway).
At this point I don't remember what it was in your patch that tripped me
up on this, so I'm marking the patch rejected.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers