SEO

#1 (**permalink**) 04-18-2008, 12:01 AM

Good analysis. I guess the question is what patch would we put into a
subrelease? If you go for a new state code, rather than a separate
boolean, does it reduce the size of the patch?

---------------------------------------------------------------------------

Tom Lane wrote:
> I wrote:
> > OK, I think I see it. The problem is that the code in slru.c is careful
> > about not modifying state when it doesn't hold the proper lock, but not
> > so careful about not *inspecting* state without the proper lock.
> > ...
> > I'm still thinking about how to make a real fix without introducing
> > another lock/unlock cycle --- we can do that if we have to, but I think
> > maybe it's possible to fix it without.
>
> Attached is a proposed patch to fix up slru.c so that it's not playing
> any games by either reading or writing shared state without holding
> the ControlLock for the SLRU set.
>
> The main problem with the existing code, as I now see it, was a poor
> choice of representation of page state: we had states CLEAN, DIRTY, and
> WRITE_IN_PROGRESS, which meant that re-dirtying a page while a write was
> in progress required setting the state back to DIRTY, which hid the fact
> that indeed a write was in progress. So the I/O code was designed to
> cope with not knowing whether another write was already in progress. We
> can make it a whole lot cleaner by changing the state representation so
> that we can tell the difference --- then, we can know before releasing
> the ControlLock whether we need to write the page or just wait for
> someone else to finish writing it. And that means we can do all the
> state-manipulation before releasing the lock.
>
> I could have just added an additional state WRITE_IN_PROGRESS_REDIRTIED,
> or some such, but it seemed notationally cleaner to create a separate
> "page_dirty" boolean, and reduce the number of states to four (empty,
> read-in-progress, valid, write-in-progress). This lets outside code
> such as clog.c just set "page_dirty = true" rather than doing a complex
> bit of logic to change the state value properly.
>
> The patch is a bit bulky because of the representation change, but the
> key changes are localized in SimpleLruReadPage and SimpleLruWritePage.
>
> I think this code is a whole lot cleaner than it was before, but it's
> a bit of a large change to be making so late in the 8.1 cycle (not to
> mention that we really ought to back-patch similar changes all the way
> back, because the bug exists as far back as 7.2). I am going to take
> another look to see if there is a less invasive change that will fix
> the problem at some performance cost; if so, we might want to do it that
> way in 8.1 and back branches.
>
> Any comments on the patch, or thoughts on how to proceed?
>
> regards, tom lane
>
>

Content-Description: slru-race-1.patch

[ Type application/octet-stream treated as attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

#2 (**permalink**) 04-18-2008, 12:01 AM

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> If you go for a new state code, rather than a separate
> boolean, does it reduce the size of the patch?

No, and it certainly wouldn't improve my level of confidence in it ...

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

#3 (**permalink**) 04-18-2008, 12:01 AM

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > If you go for a new state code, rather than a separate
> > boolean, does it reduce the size of the patch?
>
> No, and it certainly wouldn't improve my level of confidence in it ...

Well, then what real options do we have? It seems the patch is just
required for all branches.

--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

#4 (**permalink**) 04-18-2008, 12:01 AM

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Well, then what real options do we have? It seems the patch is just
> required for all branches.

I think it would be possible to fix it in a less invasive way by taking
and releasing the ControlLock an extra time in SimpleLruReadPage and
SimpleLruWritePage. What's indeterminate about that is the performance
cost. In situations where there's not a lot of SLRU I/O traffic it's
presumably negligible, but in a case like Jim's where there's evidently
a *whole* lot of traffic, it might be a killer.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

#5 (**permalink**) 04-18-2008, 12:01 AM

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Well, then what real options do we have? It seems the patch is just
> > required for all branches.
>
> I think it would be possible to fix it in a less invasive way by taking
> and releasing the ControlLock an extra time in SimpleLruReadPage and
> SimpleLruWritePage. What's indeterminate about that is the performance
> cost. In situations where there's not a lot of SLRU I/O traffic it's
> presumably negligible, but in a case like Jim's where there's evidently
> a *whole* lot of traffic, it might be a killer.

To me a performance problem is much harder get reports on and to locate
than a real fix to the problem. I think if a few people eyeball the
patch, it is OK for application. Are backpatches significantly
different?

--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

#6 (**permalink**) 04-18-2008, 12:01 AM

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> To me a performance problem is much harder get reports on and to locate
> than a real fix to the problem. I think if a few people eyeball the
> patch, it is OK for application. Are backpatches significantly
> different?

Well, the logic is the same all the way back, but the code has changed
textually quite a bit since 7.4 and even more since 7.3. I think the
patch would apply reasonably cleanly to 8.0, but adjusting it for 7.x
will take a bit of work, which would mean those versions would probably
need to be reviewed separately.

One possible compromise is to use this patch in 8.x and a simpler patch
in 7.x --- people who are very concerned about performance ought to be
running 8.x anyway ;-)

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Thread Tools
Show Printable Version Email this Page
Display Modes
Linear Mode Switch to Hybrid Mode Switch to Threaded Mode

SEO

Re: slru.c race condition (was Re: [HACKERS] TRAP: FailedAssertion("!((itemid)->lp_flags