Thread: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Alvaro Herrera
Date:
I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifying whetherit is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs tobe fixed. I didn't look into past releases to see if there's a live released bug here or not. I think the fix is simply to ignore the Xmax if the HEAP_XMAX_IS_MULTI bit is set. Additionally I think it should check HEAP_XMAX_INVALID before reading the Xmax at all. -- Álvaro Herrera (from some crappy webmail)
Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Tom Lane
Date:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifying
whetherit is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs
tobe fixed. I didn't look into past releases to see if there's a live released bug here or not.
> I think the fix is simply to ignore the Xmax if the HEAP_XMAX_IS_MULTI bit is set.
> Additionally I think it should check HEAP_XMAX_INVALID before reading the Xmax at all.
If it's failing to even check XMAX_INVALID, surely it's completely
broken? Perhaps it assumes its caller has checked all this?
regards, tom lane
Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Simon Riggs
Date:
On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifying whetherit is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs tobe fixed. I didn't look into past releases to see if there's a live released bug here or not. > >> I think the fix is simply to ignore the Xmax if the HEAP_XMAX_IS_MULTI bit is set. > >> Additionally I think it should check HEAP_XMAX_INVALID before reading the Xmax at all. > > If it's failing to even check XMAX_INVALID, surely it's completely > broken? Perhaps it assumes its caller has checked all this? Looking at it now. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Simon Riggs
Date:
On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifying whetherit is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs tobe fixed. I didn't look into past releases to see if there's a live released bug here or not. > >> I think the fix is simply to ignore the Xmax if the HEAP_XMAX_IS_MULTI bit is set. > >> Additionally I think it should check HEAP_XMAX_INVALID before reading the Xmax at all. > > If it's failing to even check XMAX_INVALID, surely it's completely > broken? Perhaps it assumes its caller has checked all this? HeapTupleHeaderAdvanceLatestRemovedXid() is only ever called when HeapTupleSatisfiesVacuum() returns HEAPTUPLE_DEAD, which only happens when HEAP_XMAX_IS_MULTI is not set. I'll add an assert to check this and a comment to explain. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Alvaro Herrera
Date:
At Monday, 10/17/2011 on 4:38 pm Simon Riggs wrote: > On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > >> I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifying whetherit is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs tobe fixed. I didn't look into past releases to see if there's a live released bug here or not. > > > >> I think the fix is simply to ignore the Xmax if the HEAP_XMAX_IS_MULTI bit is set. > > > >> Additionally I think it should check HEAP_XMAX_INVALID before reading the Xmax at all. > > > > If it's failing to even check XMAX_INVALID, surely it's completely > > broken? Perhaps it assumes its caller has checked all this? > > HeapTupleHeaderAdvanceLatestRemovedXid() is only ever called when > HeapTupleSatisfiesVacuum() returns HEAPTUPLE_DEAD, which only happens > when HEAP_XMAX_IS_MULTI is not set. Hmkay. > I'll add an assert to check this and a comment to explain. This means I'll have to hack it up further in my FK locks patch. No problem with that.
Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Simon Riggs
Date:
On Tue, Oct 18, 2011 at 3:14 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> I'll add an assert to check this and a comment to explain. > > This means I'll have to hack it up further in my FK locks patch. No problem with that. OK, I'll hold back to avoid interfering with your patch. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Bruce Momjian
Date:
On Mon, Oct 17, 2011 at 08:38:18PM +0100, Simon Riggs wrote: > On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > >> I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifying whetherit is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs tobe fixed. I didn't look into past releases to see if there's a live released bug here or not. > > > >> I think the fix is simply to ignore the Xmax if the HEAP_XMAX_IS_MULTI bit is set. > > > >> Additionally I think it should check HEAP_XMAX_INVALID before reading the Xmax at all. > > > > If it's failing to even check XMAX_INVALID, surely it's completely > > broken? Perhaps it assumes its caller has checked all this? > > HeapTupleHeaderAdvanceLatestRemovedXid() is only ever called when > HeapTupleSatisfiesVacuum() returns HEAPTUPLE_DEAD, which only happens > when HEAP_XMAX_IS_MULTI is not set. > > I'll add an assert to check this and a comment to explain. Was this completed? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Alvaro Herrera
Date:
Excerpts from Bruce Momjian's message of jue ago 16 11:24:55 -0400 2012: > On Mon, Oct 17, 2011 at 08:38:18PM +0100, Simon Riggs wrote: > > On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > >> I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifyingwhether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probablyneeds to be fixed. I didn't look into past releases to see if there's a live released bug here or not. > > > > > >> I think the fix is simply to ignore the Xmax if the HEAP_XMAX_IS_MULTI bit is set. > > > > > >> Additionally I think it should check HEAP_XMAX_INVALID before reading the Xmax at all. > > > > > > If it's failing to even check XMAX_INVALID, surely it's completely > > > broken? Perhaps it assumes its caller has checked all this? > > > > HeapTupleHeaderAdvanceLatestRemovedXid() is only ever called when > > HeapTupleSatisfiesVacuum() returns HEAPTUPLE_DEAD, which only happens > > when HEAP_XMAX_IS_MULTI is not set. > > > > I'll add an assert to check this and a comment to explain. > > Was this completed? As far as I recall, there are changes related to this in my fklocks patch. I am hoping to have some review happen on it during the upcoming commitfest (which presumably means I need to do a merge to newer sources.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Bruce Momjian
Date:
On Thu, Aug 16, 2012 at 11:38:14AM -0400, Alvaro Herrera wrote: > Excerpts from Bruce Momjian's message of jue ago 16 11:24:55 -0400 2012: > > On Mon, Oct 17, 2011 at 08:38:18PM +0100, Simon Riggs wrote: > > > On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > >> I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifyingwhether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probablyneeds to be fixed. I didn't look into past releases to see if there's a live released bug here or not. > > > > > > > >> I think the fix is simply to ignore the Xmax if the HEAP_XMAX_IS_MULTI bit is set. > > > > > > > >> Additionally I think it should check HEAP_XMAX_INVALID before reading the Xmax at all. > > > > > > > > If it's failing to even check XMAX_INVALID, surely it's completely > > > > broken? Perhaps it assumes its caller has checked all this? > > > > > > HeapTupleHeaderAdvanceLatestRemovedXid() is only ever called when > > > HeapTupleSatisfiesVacuum() returns HEAPTUPLE_DEAD, which only happens > > > when HEAP_XMAX_IS_MULTI is not set. > > > > > > I'll add an assert to check this and a comment to explain. > > > > Was this completed? > > As far as I recall, there are changes related to this in my fklocks > patch. I am hoping to have some review happen on it during the upcoming > commitfest (which presumably means I need to do a merge to newer > sources.) I was asking about adding the assert check --- does that need to wait too? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
From
Alvaro Herrera
Date:
Excerpts from Bruce Momjian's message of jue ago 16 11:44:48 -0400 2012: > On Thu, Aug 16, 2012 at 11:38:14AM -0400, Alvaro Herrera wrote: > > Excerpts from Bruce Momjian's message of jue ago 16 11:24:55 -0400 2012: > > > On Mon, Oct 17, 2011 at 08:38:18PM +0100, Simon Riggs wrote: > > > > On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > > >> I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifyingwhether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probablyneeds to be fixed. I didn't look into past releases to see if there's a live released bug here or not. > > > > > > > > > >> I think the fix is simply to ignore the Xmax if the HEAP_XMAX_IS_MULTI bit is set. > > > > > > > > > >> Additionally I think it should check HEAP_XMAX_INVALID before reading the Xmax at all. > > > > > > > > > > If it's failing to even check XMAX_INVALID, surely it's completely > > > > > broken? Perhaps it assumes its caller has checked all this? > > > > > > > > HeapTupleHeaderAdvanceLatestRemovedXid() is only ever called when > > > > HeapTupleSatisfiesVacuum() returns HEAPTUPLE_DEAD, which only happens > > > > when HEAP_XMAX_IS_MULTI is not set. > > > > > > > > I'll add an assert to check this and a comment to explain. > > > > > > Was this completed? > > > > As far as I recall, there are changes related to this in my fklocks > > patch. I am hoping to have some review happen on it during the upcoming > > commitfest (which presumably means I need to do a merge to newer > > sources.) > > I was asking about adding the assert check --- does that need to wait > too? I don't think it's worth fussing about. Also I don't need any more merge conflicts than I already have. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services