Re: Correction to comment regarding atomicity of an operation

On Tue, Sep 11, 2012 at 11:19 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>wrote:

> 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/