Shared buffer access rule violations?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Shared buffer access rule violations?

Asim Praveen
Hi,

In order to make changes to a shared buffer, one must hold a pin on it
and the content lock in exclusive mode.  This rule seems to be
followed in most of the places but there are a few exceptions.

One can find several PageInit() calls with no content lock held.  See,
for example:

fill_seq_with_data()
vm_readbuf()
fsm_readbuf()

Moreover, fsm_vacuum_page() performs
"PageGetContents(page))->fp_next_slot = 0;" without content lock.

There may be more but I want to know if these can be treated as
violations before moving ahead.

Asim

Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Tom Lane-2
Asim R P <[hidden email]> writes:
> In order to make changes to a shared buffer, one must hold a pin on it
> and the content lock in exclusive mode.  This rule seems to be
> followed in most of the places but there are a few exceptions.

> One can find several PageInit() calls with no content lock held.  See,
> for example:

> fill_seq_with_data()

That would be for a relation that no one else can even see yet, no?

> vm_readbuf()
> fsm_readbuf()

In these cases I'd imagine that the I/O completion interlock is what
is preventing other backends from accessing the buffer.

> Moreover, fsm_vacuum_page() performs
> "PageGetContents(page))->fp_next_slot = 0;" without content lock.

That field is just a hint, IIRC, and the possibility of a torn read
is explicitly not worried about.

> There may be more but I want to know if these can be treated as
> violations before moving ahead.

These specific things don't sound like bugs, though possibly I'm
missing something.  Perhaps more comments would be in order.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Asim Praveen
On Tue, Jul 10, 2018 at 8:33 PM, Tom Lane <[hidden email]> wrote:
> Asim R P <[hidden email]> writes:
>
>> One can find several PageInit() calls with no content lock held.  See,
>> for example:
>
>> fill_seq_with_data()
>
> That would be for a relation that no one else can even see yet, no?

Yes, when the sequence is being created.  No, when the sequence is
being reset, in ResetSequence().

>
>> vm_readbuf()
>> fsm_readbuf()
>
> In these cases I'd imagine that the I/O completion interlock is what
> is preventing other backends from accessing the buffer.
>

What is I/O completion interlock?  I see no difference in initializing
a visimap/fsm page and initializing a standard heap page.  For
standard heap pages, the code currently acquires the buffer pin as
well as content lock for initialization.


>> Moreover, fsm_vacuum_page() performs
>> "PageGetContents(page))->fp_next_slot = 0;" without content lock.
>
> That field is just a hint, IIRC, and the possibility of a torn read
> is explicitly not worried about.

Yes, that's a hint.  And ignoring torn page possibility doesn't result
in checksum failures because fsm_read() passes RMB_ZERO_ON_ERROR to
buffer manager.  The page will be zeroed out in the event of checksum
failure.

Asim

Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Tom Lane-2
Asim R P <[hidden email]> writes:
> On Tue, Jul 10, 2018 at 8:33 PM, Tom Lane <[hidden email]> wrote:
>> Asim R P <[hidden email]> writes:
>>> One can find several PageInit() calls with no content lock held.  See,
>>> for example:
>>> fill_seq_with_data()

>> That would be for a relation that no one else can even see yet, no?

> Yes, when the sequence is being created.  No, when the sequence is
> being reset, in ResetSequence().

ResetSequence creates a new relfilenode, which no one else will be able
to see until it commits, so the case is effectively the same as for
creation.

>>> vm_readbuf()
>>> fsm_readbuf()

>> In these cases I'd imagine that the I/O completion interlock is what
>> is preventing other backends from accessing the buffer.

> What is I/O completion interlock?

Oh ... the RBM_ZERO_ON_ERROR action should be done under the I/O lock,
but the ReadBuffer caller isn't holding that lock anymore, so I see your
point here.  Probably, nobody's noticed because it's a corner case that
shouldn't happen under normal use, but it's not safe.  I think what we
want is more like

        if (PageIsNew(BufferGetPage(buf)))
        {
                LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
                if (PageIsNew(BufferGetPage(buf)))
                        PageInit(BufferGetPage(buf), BLCKSZ, 0);
                UnlockReleaseBuffer(buf);
        }

to ensure that the page is initialized once and only once, even if
several backends do this concurrently.

                        regards, tom lane

Previous Thread Next Thread