Thread: Correction to comment regarding atomicity of an operation
<div dir="ltr">This comment in UpdateFullPageWrites() seems to be inaccurate:<br /><br /> * It's safe to check the sharedfull_page_writes without the lock,<br /> * because we assume that there is no concurrently running process which<br/> * can update it.<br /><br />That assumption does not hold on any sane SMP system.<br /><br />I think thereal reason is that we assume that read/write to an integer is atomic, like we do for TransactionId variables:<br /><br/> heapam.c: "TransactionId read/write is assumed atomic anyway."<br /><br />Best regards,<br /><br />PS: As usual,I hope I am not missing something very obvious.<br />-- <br /><div dir="ltr">Gurjeet Singh<br /><br /><a href="http://gurjeet.singh.im/"target="_blank">http://gurjeet.singh.im/</a><br /></div><br /></div>
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.
On Tue, Sep 11, 2012 at 11:19 PM, Amit Kapila <amit.kapila@huawei.com> wrote:
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.
On Wednesday, September 12, 2012 5:33 AM Gurjeet Singh wrote:
> This comment in UpdateFullPageWrites() seems to be inaccurate:Do you able to see any case where it can be updated when being accessed here.
> * 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.
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,
--
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.
<div dir="ltr"><div class="gmail_quote">On Wed, Sep 12, 2012 at 4:08 PM, Noah Misch <span dir="ltr"><<a href="mailto:noah@leadboat.com"target="_blank">noah@leadboat.com</a>></span> wrote:<br /><blockquote class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Wed, Sep 12,2012 at 06:44:37AM -0400, Gurjeet Singh wrote:<br /> > Thinking a bit more about the need for locks, I guess even theshared<br /> > variables whose read/write ops are considered atomic need to be protected<br /> > by locks so thatthe effects of NUMA architectures can be mitigated.<br /><br /></div>src/backend/storage/lmgr/README.barrier has nicecoverage of such issues.<br /><br /> NUMA does not change the picture. CPU architecture specifications define<br />ordering constraints for instructions that touch memory. NUMA is a property<br /> of specific system implementations thatchanges performance characteristics,<br /> but not functional guarantees, of those instructions.<br /></blockquote></div><br/>I read-up a bit more on the topic, and it seems that the pure NUMA based machines have never beensold in the market, quite possibly because of the difficulty to write programs for them. The NUMA machines in use areeffectively ccNUMA (cc for cache-coherent).<br /><br />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 guaranteesprovided by ccNUMA and those provided by non-ccNUMA (regular NUMA). I may be totally off here, so please correctme if needed.<br /><br /><a href="http://en.wikipedia.org/wiki/Non-Uniform_Memory_Access#Cache_coherent_NUMA_.28ccNUMA.29">http://en.wikipedia.org/wiki/Non-Uniform_Memory_Access#Cache_coherent_NUMA_.28ccNUMA.29</a><br />--<br /><div dir="ltr">Gurjeet Singh<br /><br /><a href="http://gurjeet.singh.im/" target="_blank">http://gurjeet.singh.im/</a><br/></div><br /></div>