HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifying whether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs 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.
--
Álvaro Herrera (from some crappy webmail)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
I just noticed that HeapTupleHeaderAdvanceLatestRemovedXid is comparing Xmax as a TransactionId without verifying whether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs 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?
regards, tom lane
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 whether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs 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?
Looking at it now.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
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 whether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs 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.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
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 whether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs 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.
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.
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
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 whether it is a multixact or not. �Since they advance separately, this could lead to bogus answers. �This probably needs 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?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
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 verifying whether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs 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
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 verifying whether it is a multixact or not. �Since they advance separately, this could lead to bogus answers. �This probably needs 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. +
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 verifying whether it is a multixact or not. Since they advance separately, this could lead to bogus answers. This probably needs 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