Lists: | pgsql-patches |
---|
From: | ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | pgsql-patches(at)postgresql(dot)org |
Subject: | pgstattuple locking fix |
Date: | 2007-10-22 02:58:12 |
Message-ID: | 20071022112558.648E.ITAGAKI.TAKAHIRO@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Here is a trivial fix of locking issue in pgstattuple().
It was locking buffers around PageGetHeapFreeSpace()
in the heap scan loop, but not in the scanning of the tail.
I think we need locks in the tail, too.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
pgstattuple_lock.patch | application/octet-stream | 785 bytes |
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
Cc: | pgsql-patches(at)postgresql(dot)org |
Subject: | Re: pgstattuple locking fix |
Date: | 2007-10-22 03:46:43 |
Message-ID: | 20464.1193024803@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> Here is a trivial fix of locking issue in pgstattuple().
Hmm, is this really a bug, and if so how far back does it go?
I'm thinking that having a pin on the buffer should be enough to
call PageGetHeapFreeSpace.
regards, tom lane
From: | ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | pgsql-patches(at)postgresql(dot)org, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: pgstattuple locking fix |
Date: | 2007-10-22 09:42:44 |
Message-ID: | 20071022183548.6064.ITAGAKI.TAKAHIRO@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> > Here is a trivial fix of locking issue in pgstattuple().
>
> Hmm, is this really a bug, and if so how far back does it go?
> I'm thinking that having a pin on the buffer should be enough to
> call PageGetHeapFreeSpace.
Hmm... we might use pd_upper and pd_lower at different times,
but the error is ok for pgstattuple purpose.
(It might be dangerous for tuple insertion, though.)
Inconsistent usage of locks is confusable -- remove them, please.
Index: contrib/pgstattuple/pgstattuple.c
===================================================================
--- contrib/pgstattuple/pgstattuple.c (HEAD)
+++ contrib/pgstattuple/pgstattuple.c (working copy)
@@ -289,9 +289,7 @@
while (block <= tupblock)
{
buffer = ReadBuffer(rel, block);
- LockBuffer(buffer, BUFFER_LOCK_SHARE);
stat.free_space += PageGetHeapFreeSpace((Page) BufferGetPage(buffer));
- LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
ReleaseBuffer(buffer);
block++;
}
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
From: | "Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> |
---|---|
To: | "ITAGAKI Takahiro" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
Cc: | <pgsql-patches(at)postgresql(dot)org>, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: pgstattuple locking fix |
Date: | 2007-10-22 09:54:10 |
Message-ID: | 471C7342.2040102@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
ITAGAKI Takahiro wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
>>> Here is a trivial fix of locking issue in pgstattuple().
>> Hmm, is this really a bug, and if so how far back does it go?
>> I'm thinking that having a pin on the buffer should be enough to
>> call PageGetHeapFreeSpace.
>
> Hmm... we might use pd_upper and pd_lower at different times,
> but the error is ok for pgstattuple purpose.
> (It might be dangerous for tuple insertion, though.)
> Inconsistent usage of locks is confusable -- remove them, please.
No I think the original patch was right. You can indeed read
inconsistent values for pd_upper and pd_lower, though the window is very
small. But a bigger issue is that in 8.3 PageGetHeapFreeSpace counts the
used line pointers to see if MaxHeapTuplesPerPage has been reached, and
I'm not convinced you can do that safely without holding a lock.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
From: | "Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> |
---|---|
To: | <pgsql-patches(at)postgresql(dot)org> |
Cc: | "ITAGAKI Takahiro" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: pgstattuple locking fix |
Date: | 2007-10-22 10:07:53 |
Message-ID: | 471C7679.2000705@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
Heikki Linnakangas wrote:
> ITAGAKI Takahiro wrote:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>>> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
>>>> Here is a trivial fix of locking issue in pgstattuple().
>>> Hmm, is this really a bug, and if so how far back does it go?
>>> I'm thinking that having a pin on the buffer should be enough to
>>> call PageGetHeapFreeSpace.
>> Hmm... we might use pd_upper and pd_lower at different times,
>> but the error is ok for pgstattuple purpose.
>> (It might be dangerous for tuple insertion, though.)
>> Inconsistent usage of locks is confusable -- remove them, please.
>
> No I think the original patch was right. You can indeed read
> inconsistent values for pd_upper and pd_lower, though the window is very
> small. But a bigger issue is that in 8.3 PageGetHeapFreeSpace counts the
> used line pointers to see if MaxHeapTuplesPerPage has been reached, and
> I'm not convinced you can do that safely without holding a lock.
On second thought, we do call PageGetHeapFreeSpace without holding a
lock in heap_page_prune_opt as well, so it better be safe. Looking
closer at PageGetHeapFreeSpace, I think it is. The
return value can be bogus, of course.
That's worth noting in the comments:
*** src/backend/storage/page/bufpage.c 21 Sep 2007 21:25:42 -0000 1.75
--- src/backend/storage/page/bufpage.c 22 Oct 2007 10:06:02 -0000
***************
*** 506,511 ****
--- 506,514 ----
* or dead line pointers it'd be possible to have too many line pointers.
* To avoid breaking code that assumes MaxHeapTuplesPerPage is a hard
limit
* on the number of line pointers, we make this extra check.)
+ *
+ * You don't need to hold a lock on the page to call this function,
but if
+ * you don't, the return value should be considered a hint only.
*/
Size
PageGetHeapFreeSpace(Page page)
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | "Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> |
Cc: | "ITAGAKI Takahiro" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>, pgsql-patches(at)postgresql(dot)org |
Subject: | Re: pgstattuple locking fix |
Date: | 2007-10-22 17:21:04 |
Message-ID: | 9494.1193073664@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:
> ITAGAKI Takahiro wrote:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I'm thinking that having a pin on the buffer should be enough to
>>> call PageGetHeapFreeSpace.
>>
>> Hmm... we might use pd_upper and pd_lower at different times,
> No I think the original patch was right. You can indeed read
> inconsistent values for pd_upper and pd_lower, though the window is very
> small.
Good point. I'll apply the original patch and also Heikki's suggested
comment.
regards, tom lane
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | "Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> |
Cc: | pgsql-patches(at)postgresql(dot)org, "ITAGAKI Takahiro" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
Subject: | Re: pgstattuple locking fix |
Date: | 2007-10-22 17:32:06 |
Message-ID: | 16712.1193074326@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-patches |
"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:
> On second thought, we do call PageGetHeapFreeSpace without holding a
> lock in heap_page_prune_opt as well, so it better be safe. Looking
> closer at PageGetHeapFreeSpace, I think it is. The
> return value can be bogus, of course.
> That's worth noting in the comments:
On closer look, the comments in heap_page_prune_opt seem to address
the issue already, and I'm not sure why we should comment
PageGetHeapFreeSpace this way but not all the other variants in
bufpage.c.
regards, tom lane