Thread: Comment patch for bgworker.c
The comment for the BackgroundWorkerSlot structure tripped me up reviewing Robert's background worker patch; it made it clearthat you need to use a memory barrier before setting in_use, but normally you'd never need to worry about that becauseRegisterDynamicBackgroundWorker() 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
Attachment
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
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