Re: [HACKERS] Correction to comment regarding atomicity of an operation
On Tue, Sep 11, 2012 at 11:19 PM, Amit Kapila amit.kap...@huawei.comwrote: On Wednesday, September 12, 2012 5:33 AM Gurjeet Singh wrote: ** ** ** ** This comment in UpdateFullPageWrites() seems to be inaccurate: * It's safe to check the shared full_page_writes without the lock, * because we assume that there is no concurrently running process which * can update it. That assumption does not hold on any sane SMP system. Do you able to see any case where it can be updated when being accessed here. Now that I looked again, I don't see this being called by anyone other than Checkpointer or the Startup process (StartupXLOG()). This stack seemed like it could be called by multiple backend processes at the same time: UpdateFullPageWrites() UpdateSharedMemoryConfig() CheckpointWriteDelay() But looking closely, CheckpointWriteDelay() has this check at the beginning: if (!AmCheckpointerProcess()) return; which stops normal backends from calling UpdateFullPageWrites(). All this is not obvious from the comments in UpdateFullPageWrites(), but the comments for XLogCtlInsert.fullPageWrites make it clear that this shared variable is changed only by the Checkpointer. Thinking a bit more about the need for locks, I guess even the shared variables whose read/write ops are considered atomic need to be protected by locks so that the effects of NUMA architectures can be mitigated. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/
Re: [HACKERS] Correction to comment regarding atomicity of an operation
On Wed, Sep 12, 2012 at 06:44:37AM -0400, Gurjeet Singh wrote: Thinking a bit more about the need for locks, I guess even the shared variables whose read/write ops are considered atomic need to be protected by locks so that the effects of NUMA architectures can be mitigated. src/backend/storage/lmgr/README.barrier has nice coverage of such issues. NUMA does not change the picture. CPU architecture specifications define ordering constraints for instructions that touch memory. NUMA is a property of specific system implementations that changes performance characteristics, but not functional guarantees, of those instructions. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Correction to comment regarding atomicity of an operation
On Wed, Sep 12, 2012 at 4:08 PM, Noah Misch n...@leadboat.com wrote: On Wed, Sep 12, 2012 at 06:44:37AM -0400, Gurjeet Singh wrote: Thinking a bit more about the need for locks, I guess even the shared variables whose read/write ops are considered atomic need to be protected by locks so that the effects of NUMA architectures can be mitigated. src/backend/storage/lmgr/README.barrier has nice coverage of such issues. NUMA does not change the picture. CPU architecture specifications define ordering constraints for instructions that touch memory. NUMA is a property of specific system implementations that changes performance characteristics, but not functional guarantees, of those instructions. I read-up a bit more on the topic, and it seems that the pure NUMA based machines have never been sold in the market, quite possibly because of the difficulty to write programs for them. The NUMA machines in use are effectively ccNUMA (cc for cache-coherent). So when people talk about NUMA (like, I think you are doing above), they mean the ccNUMA. So, based on what little I know about it, I think there are differences between functional guarantees provided by ccNUMA and those provided by non-ccNUMA (regular NUMA). I may be totally off here, so please correct me if needed. http://en.wikipedia.org/wiki/Non-Uniform_Memory_Access#Cache_coherent_NUMA_.28ccNUMA.29 -- Gurjeet Singh http://gurjeet.singh.im/
[HACKERS] Correction to comment regarding atomicity of an operation
This comment in UpdateFullPageWrites() seems to be inaccurate: * It's safe to check the shared full_page_writes without the lock, * because we assume that there is no concurrently running process which * can update it. That assumption does not hold on any sane SMP system. I think the real reason is that we assume that read/write to an integer is atomic, like we do for TransactionId variables: heapam.c: TransactionId read/write is assumed atomic anyway. Best regards, PS: As usual, I hope I am not missing something very obvious. -- Gurjeet Singh http://gurjeet.singh.im/
Re: [HACKERS] Correction to comment regarding atomicity of an operation
On Wednesday, September 12, 2012 5:33 AM Gurjeet Singh wrote: This comment in UpdateFullPageWrites() seems to be inaccurate: * It's safe to check the shared full_page_writes without the lock, * because we assume that there is no concurrently running process which * can update it. That assumption does not hold on any sane SMP system. Do you able to see any case where it can be updated when being accessed here. With Regards, Amit Kapila.
