Lists: | pgsql-hackers |
---|
From: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
---|---|
To: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2011-10-17 18:33:36 |
Message-ID: | 1318876416.4e9c75001bfc7@webmail.no-ip.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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)
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2011-10-17 19:03:50 |
Message-ID: | 17713.1318878230@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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
From: | Simon Riggs <simon(at)2ndQuadrant(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2011-10-17 19:07:37 |
Message-ID: | CA+U5nMKFJTKybNo+LJ+1HdpHmpzubja_vfzWUGFf9AQE=DBmTg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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
From: | Simon Riggs <simon(at)2ndQuadrant(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2011-10-17 19:38:18 |
Message-ID: | CA+U5nMJJ4s-CysTBiqXK0YaaPavHn_2McL-ETPXejMf7QdfmRQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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
From: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
---|---|
To: | Simon Riggs <simon(at)2ndQuadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2011-10-18 02:14:53 |
Message-ID: | 1318904093.4e9ce11d4aaab@webmail.no-ip.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
At Monday, 10/17/2011 on 4:38 pm Simon Riggs wrote:
> On Mon, Oct 17, 2011 at 8:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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.
From: | Simon Riggs <simon(at)2ndQuadrant(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2011-10-18 08:44:12 |
Message-ID: | CA+U5nMK3hsF2ggmmfvchCux7X_tqKAg_x3=7qY7pmyiyLht89g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Tue, Oct 18, 2011 at 3:14 AM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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
From: | Bruce Momjian <bruce(at)momjian(dot)us> |
---|---|
To: | Simon Riggs <simon(at)2ndQuadrant(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2012-08-16 15:24:55 |
Message-ID: | 20120816152455.GL8353@momjian.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
From: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
---|---|
To: | Bruce Momjian <bruce(at)momjian(dot)us> |
Cc: | Simon Riggs <simon(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2012-08-16 15:38:14 |
Message-ID: | 1345131325-sup-1456@alvh.no-ip.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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
From: | Bruce Momjian <bruce(at)momjian(dot)us> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Cc: | Simon Riggs <simon(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2012-08-16 15:44:48 |
Message-ID: | 20120816154448.GN8353@momjian.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > > Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
From: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
---|---|
To: | Bruce Momjian <bruce(at)momjian(dot)us> |
Cc: | Simon Riggs <simon(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts |
Date: | 2012-08-16 16:02:44 |
Message-ID: | 1345132934-sup-1120@alvh.no-ip.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > > > Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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