Re: Bug in autovacuum.c?

Looking over the autovacuum.c code, I see:

/*
* Determine the oldest datfrozenxid/relfrozenxid that we will allow to
* pass without forcing a vacuum. (This limit can be tightened for
* particular tables, but not loosened.)
*/
recentXid = ReadNewTransactionId();
xidForceLimit = recentXid - autovacuum_freeze_max_age;
/* ensure it's a "normal" XID, else TransactionIdPrecedes misbehaves */
if (xidForceLimit < FirstNormalTransactionId)
xidForceLimit -= FirstNormalTransactionId;

This last line doesn't look right to me; should it be:

xidForceLimit = FirstNormalTransactionId;

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Mar 31, 2011 at 12:17 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Looking over the autovacuum.c code, I see:
>
>    /*
>     * Determine the oldest datfrozenxid/relfrozenxid that we will allow to
>     * pass without forcing a vacuum.  (This limit can be tightened for
>     * particular tables, but not loosened.)
>     */
>    recentXid = ReadNewTransactionId();
>    xidForceLimit = recentXid - autovacuum_freeze_max_age;
>    /* ensure it's a "normal" XID, else TransactionIdPrecedes misbehaves */
>    if (xidForceLimit < FirstNormalTransactionId)
>        xidForceLimit -= FirstNormalTransactionId;
>
> This last line doesn't look right to me;  should it be:
>
>        xidForceLimit = FirstNormalTransactionId;

That would probably work, but the existing coding actually makes more
sense. It's essentially trying to scan backwards by
autovacuum_freeze_max_age XIDs through the circular XID space. But
the XID space isn't actually circular, because there are 3 special
values. So if we land on one of those values we want to skip backward
by 3. Here FirstNormalTransactionId doesn't represent itself, but
rather the number of special XIDs that exist.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of jue mar 31 13:58:41 -0300 2011:
> On Thu, Mar 31, 2011 at 12:17 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Looking over the autovacuum.c code, I see:
> >
> >    /*
> >     * Determine the oldest datfrozenxid/relfrozenxid that we will allow to
> >     * pass without forcing a vacuum.  (This limit can be tightened for
> >     * particular tables, but not loosened.)
> >     */
> >    recentXid = ReadNewTransactionId();
> >    xidForceLimit = recentXid - autovacuum_freeze_max_age;
> >    /* ensure it's a "normal" XID, else TransactionIdPrecedes misbehaves */
> >    if (xidForceLimit < FirstNormalTransactionId)
> >        xidForceLimit -= FirstNormalTransactionId;
> >
> > This last line doesn't look right to me;  should it be:
> >
> >        xidForceLimit = FirstNormalTransactionId;
>
> That would probably work, but the existing coding actually makes more
> sense. It's essentially trying to scan backwards by
> autovacuum_freeze_max_age XIDs through the circular XID space. But
> the XID space isn't actually circular, because there are 3 special
> values. So if we land on one of those values we want to skip backward
> by 3. Here FirstNormalTransactionId doesn't represent itself, but
> rather the number of special XIDs that exist.

Yeah, I think this change would have the effect of moving the freeze
limit by one (or two?) counts. Given the moving nature of values
returned by ReadNewTransactionId this would probably have no practical
effect. Still, the code as is seems more natural to me (Tom wrote this
bit IIRC, not me).

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> > That would probably work, but the existing coding actually makes more
> > sense. It's essentially trying to scan backwards by
> > autovacuum_freeze_max_age XIDs through the circular XID space. But
> > the XID space isn't actually circular, because there are 3 special
> > values. So if we land on one of those values we want to skip backward
> > by 3. Here FirstNormalTransactionId doesn't represent itself, but
> > rather the number of special XIDs that exist.
>
> Yeah, I think this change would have the effect of moving the freeze
> limit by one (or two?) counts. Given the moving nature of values
> returned by ReadNewTransactionId this would probably have no practical
> effect. Still, the code as is seems more natural to me (Tom wrote this
> bit IIRC, not me).

I am now thinking the code is correct --- it maps values from 0 to
FirstNormalTransactionId into the top of the (unsigned) xid range.
Unless someone objects, I will add a C comment about this behavior so
future readers are not confused.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> > Yeah, I think this change would have the effect of moving the freeze
> > limit by one (or two?) counts. Given the moving nature of values
> > returned by ReadNewTransactionId this would probably have no practical
> > effect. Still, the code as is seems more natural to me (Tom wrote this
> > bit IIRC, not me).
>
> I am now thinking the code is correct --- it maps values from 0 to
> FirstNormalTransactionId into the top of the (unsigned) xid range.
> Unless someone objects, I will add a C comment about this behavior so
> future readers are not confused.

OK, now I think it is wrong. :-)

The effect is to map max xid + 1 to max xid -
FirstNormalTransactionId(3) + 1, which makes the xid look like it is
going backwards, less than max xid --- not good.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Bruce Momjian wrote:
>> > Yeah, I think this change would have the effect of moving the freeze
>> > limit by one (or two?) counts.  Given the moving nature of values
>> > returned by ReadNewTransactionId this would probably have no practical
>> > effect.  Still, the code as is seems more natural to me (Tom wrote this
>> > bit IIRC, not me).
>>
>> I am now thinking the code is correct --- it maps values from 0 to
>> FirstNormalTransactionId into the top of the (unsigned) xid range.
>> Unless someone objects, I will add a C comment about this behavior so
>> future readers are not confused.
>
> OK, now I think it is wrong.   :-)
>
> The effect is to map max xid + 1 to max xid -
> FirstNormalTransactionId(3) + 1, which makes the xid look like it is
> going backwards, less than max xid --- not good.

The XID space is *circular*.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Bruce Momjian wrote:
> >> > Yeah, I think this change would have the effect of moving the freeze
> >> > limit by one (or two?) counts. ?Given the moving nature of values
> >> > returned by ReadNewTransactionId this would probably have no practical
> >> > effect. ?Still, the code as is seems more natural to me (Tom wrote this
> >> > bit IIRC, not me).
> >>
> >> I am now thinking the code is correct --- it maps values from 0 to
> >> FirstNormalTransactionId into the top of the (unsigned) xid range.
> >> Unless someone objects, I will add a C comment about this behavior so
> >> future readers are not confused.
> >
> > OK, now I think it is wrong. ? :-)
> >
> > The effect is to map max xid + 1 to max xid -
> > FirstNormalTransactionId(3) + 1, which makes the xid look like it is
> > going backwards, less than max xid --- not good.
>
> The XID space is *circular*.

Right but you would think that as the xid moves forward, the caculation
of how far back to vacuum should move only forward. In this case,
incrementing the xid by one would cause the vacuum horizon to move
backward by two.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Mar 31, 2011 at 4:16 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> > Bruce Momjian wrote:
>> >> > Yeah, I think this change would have the effect of moving the freeze
>> >> > limit by one (or two?) counts. ?Given the moving nature of values
>> >> > returned by ReadNewTransactionId this would probably have no practical
>> >> > effect. ?Still, the code as is seems more natural to me (Tom wrote this
>> >> > bit IIRC, not me).
>> >>
>> >> I am now thinking the code is correct --- it maps values from 0 to
>> >> FirstNormalTransactionId into the top of the (unsigned) xid range.
>> >> Unless someone objects, I will add a C comment about this behavior so
>> >> future readers are not confused.
>> >
>> > OK, now I think it is wrong. ? :-)
>> >
>> > The effect is to map max xid + 1 to max xid -
>> > FirstNormalTransactionId(3) + 1, which makes the xid look like it is
>> > going backwards, less than max xid --- not good.
>>
>> The XID space is *circular*.
>
> Right but you would think that as the xid moves forward, the caculation
> of how far back to vacuum should move only forward.  In this case,
> incrementing the xid by one would cause the vacuum horizon to move
> backward by two.

I don't see how that would happen. The XID immediately preceding
FirstNormalTransactionId is 2^32-1, and that's exactly what this
calculation produces.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> >> > The effect is to map max xid + 1 to max xid -
> >> > FirstNormalTransactionId(3) + 1, which makes the xid look like it is
> >> > going backwards, less than max xid --- not good.
> >>
> >> The XID space is *circular*.
> >
> > Right but you would think that as the xid moves forward, the caculation
> > of how far back to vacuum should move only forward. ?In this case,
> > incrementing the xid by one would cause the vacuum horizon to move
> > backward by two.
>
> I don't see how that would happen. The XID immediately preceding
> FirstNormalTransactionId is 2^32-1, and that's exactly what this
> calculation produces.

OK, let me see if I understand --- the caculation is below:

xidForceLimit = recentXid - autovacuum_freeze_max_age;
if (xidForceLimit < FirstNormalTransactionId)
xidForceLimit -= FirstNormalTransactionId;

The values:

xidForceLimit Result
---------------------------
max_xid-2 max_xid-2
max_xid-1 max_xid-1
max_xid max_xid
0 max_xid-3 <- backward here
1 max_xid-2
2 max_xid-1
3 3

With the -= change to =, we get:

xidForceLimit Result
---------------------------
max_xid-2 max_xid-2
max_xid-1 max_xid-1
max_xid max_xid
0 3
1 3
2 3
3 3

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Mar 31, 2011 at 4:35 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> >> > The effect is to map max xid + 1 to max xid -
>> >> > FirstNormalTransactionId(3) + 1, which makes the xid look like it is
>> >> > going backwards, less than max xid --- not good.
>> >>
>> >> The XID space is *circular*.
>> >
>> > Right but you would think that as the xid moves forward, the caculation
>> > of how far back to vacuum should move only forward. ?In this case,
>> > incrementing the xid by one would cause the vacuum horizon to move
>> > backward by two.
>>
>> I don't see how that would happen.   The XID immediately preceding
>> FirstNormalTransactionId is 2^32-1, and that's exactly what this
>> calculation produces.
>
> OK, let me see if I understand --- the caculation is below:
>
>    xidForceLimit = recentXid - autovacuum_freeze_max_age;
>    if (xidForceLimit < FirstNormalTransactionId)
>        xidForceLimit -= FirstNormalTransactionId;
>
> The values:
>
>        xidForceLimit   Result
>        ---------------------------
>        max_xid-2       max_xid-2
>        max_xid-1       max_xid-1
>        max_xid         max_xid
>        0               max_xid-3       <- backward here
>        1               max_xid-2
>        2               max_xid-1
>        3               3

You have to consider those three lines all of a piece. Suppose
autovacuum_freeze_age is 100. Then:

105 -> 5
104 -> 4
103 -> 3
102 -> max_xid
101 -> max_xid - 1
100 -> max_xid - 2

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> > ? ?xidForceLimit = recentXid - autovacuum_freeze_max_age;
> > ? ?if (xidForceLimit < FirstNormalTransactionId)
> > ? ? ? ?xidForceLimit -= FirstNormalTransactionId;
> >
> > The values:
> >
> > ? ? ? ?xidForceLimit ? Result
> > ? ? ? ?---------------------------
> > ? ? ? ?max_xid-2 ? ? ? max_xid-2
> > ? ? ? ?max_xid-1 ? ? ? max_xid-1
> > ? ? ? ?max_xid ? ? ? ? max_xid
> > ? ? ? ?0 ? ? ? ? ? ? ? max_xid-3 ? ? ? <- backward here
> > ? ? ? ?1 ? ? ? ? ? ? ? max_xid-2
> > ? ? ? ?2 ? ? ? ? ? ? ? max_xid-1
> > ? ? ? ?3 ? ? ? ? ? ? ? 3
>
> You have to consider those three lines all of a piece. Suppose
> autovacuum_freeze_age is 100. Then:
>
> 105 -> 5
> 104 -> 4
> 103 -> 3
> 102 -> max_xid
> 101 -> max_xid - 1
> 100 -> max_xid - 2

OK, just keep going below 100:

105 -> 5
104 -> 4
103 -> 3
102 -> max_xid
101 -> max_xid - 1
100 -> max_xid - 2
99 -> max_id
98 -> max_id -1

Wouldn't you rather:

105 -> 5
104 -> 4
103 -> 3
102 -> 3
101 -> 3
100 -> 3
99 -> max_id
98 -> max_id -1

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Mar 31, 2011 at 10:59 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> OK, just keep going below 100:
>
>        105 -> 5
>        104 -> 4
>        103 -> 3
>        102 -> max_xid
>        101 -> max_xid - 1
>        100 -> max_xid - 2
>         99 -> max_id
>         98 -> max_id -1

Yeah, I think this is what the code is doing.

>
> Wouldn't you rather:
>
>        105 -> 5
>        104 -> 4
>        103 -> 3
>        102 -> 3
>        101 -> 3
>        100 -> 3
>         99 -> max_id
>         98 -> max_id -1
>

I think I would expect

>        105 -> 5
>        104 -> 4
>        103 -> 3
>        102 -> max_id
>        101 -> max_id-1
>        100 -> max_id-2
>         99 -> max_id-3

But it doesn't really matter either way, does it? We don't even allow
setting vacuum_max_freeze_age to 2^31-1 or any value that would be
close to triggering a problem here.

--
greg

Greg Stark wrote:
> On Thu, Mar 31, 2011 at 10:59 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > OK, just keep going below 100:
> >
> > ? ? ? ?105 -> 5
> > ? ? ? ?104 -> 4
> > ? ? ? ?103 -> 3
> > ? ? ? ?102 -> max_xid
> > ? ? ? ?101 -> max_xid - 1
> > ? ? ? ?100 -> max_xid - 2
> > ? ? ? ? 99 -> max_id
> > ? ? ? ? 98 -> max_id -1
>
> Yeah, I think this is what the code is doing.
>
> >
> > Wouldn't you rather:
> >
> > ? ? ? ?105 -> 5
> > ? ? ? ?104 -> 4
> > ? ? ? ?103 -> 3
> > ? ? ? ?102 -> 3
> > ? ? ? ?101 -> 3
> > ? ? ? ?100 -> 3
> > ? ? ? ? 99 -> max_id
> > ? ? ? ? 98 -> max_id -1
> >
>
> I think I would expect
>
> > ? ? ? ?105 -> 5
> > ? ? ? ?104 -> 4
> > ? ? ? ?103 -> 3
> > ? ? ? ?102 -> max_id
> > ? ? ? ?101 -> max_id-1
> > ? ? ? ?100 -> max_id-2
> > ? ? ? ? 99 -> max_id-3
>
> But it doesn't really matter either way, does it? We don't even allow
> setting vacuum_max_freeze_age to 2^31-1 or any value that would be
> close to triggering a problem here.

It doesn't need to be that high because it is subtracted from the
current xid counter, so if vacuum_max_freeze_age is 100, and the xid
counter is 101, we see the problem.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Mar 31, 2011 at 5:59 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> OK, just keep going below 100:
>
>        105 -> 5
>        104 -> 4
>        103 -> 3
>        102 -> max_xid
>        101 -> max_xid - 1
>        100 -> max_xid - 2
>         99 -> max_id
>         98 -> max_id -1
>
> Wouldn't you rather:
>
>        105 -> 5
>        104 -> 4
>        103 -> 3
>        102 -> 3
>        101 -> 3
>        100 -> 3
>         99 -> max_id
>         98 -> max_id -1

Oh, quite right. Sorry I missed that. I suppose if we wanted to fix
this for real, we'd want to get:

105->5
104->4
103->3
102->max_xid
101->max_xid-1
100->max_xid-2
99->max_xid-3
98->max_xid-4

But it doesn't seem worth getting excited about.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> Oh, quite right. Sorry I missed that. I suppose if we wanted to fix
> this for real, we'd want to get:
>
> 105->5
> 104->4
> 103->3
> 102->max_xid
> 101->max_xid-1
> 100->max_xid-2
> 99->max_xid-3
> 98->max_xid-4
>
> But it doesn't seem worth getting excited about.

I think (?) the problem with that is the every time you wrap around you
get more out of sync. :-O

Thinking more, the problem is that when the xid counter wraps around
from max_xid to 3, we jump the freeze horizon by three, e.g 5000 to
5003. So when, the freeze horizon wraps, we can either have that jump
by three, e.g set it to FirstNormalTransactionId, or delay by three,
e.g. set it to MaxTransactionId.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Apr 1, 2011 at 11:18 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Robert Haas wrote:
>> Oh, quite right.  Sorry I missed that.  I suppose if we wanted to fix
>> this for real, we'd want to get:
>>
>> 105->5
>> 104->4
>> 103->3
>> 102->max_xid
>> 101->max_xid-1
>> 100->max_xid-2
>> 99->max_xid-3
>> 98->max_xid-4
>>
>> But it doesn't seem worth getting excited about.
>
> I think (?) the problem with that is the every time you wrap around you
> get more out of sync.  :-O

It's not clear to me that it matters a bit, though.

> Thinking more, the problem is that when the xid counter wraps around
> from max_xid to 3, we jump the freeze horizon by three, e.g 5000 to
> 5003.  So when, the freeze horizon wraps, we can either have that jump
> by three, e.g set it to FirstNormalTransactionId, or delay by three,
> e.g. set it to MaxTransactionId.

So what? :-)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Fri, Apr 1, 2011 at 11:18 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Robert Haas wrote:
> >> Oh, quite right. ?Sorry I missed that. ?I suppose if we wanted to fix
> >> this for real, we'd want to get:
> >>
> >> 105->5
> >> 104->4
> >> 103->3
> >> 102->max_xid
> >> 101->max_xid-1
> >> 100->max_xid-2
> >> 99->max_xid-3
> >> 98->max_xid-4
> >>
> >> But it doesn't seem worth getting excited about.
> >
> > I think (?) the problem with that is the every time you wrap around you
> > get more out of sync. ?:-O
>
> It's not clear to me that it matters a bit, though.

To do the right thing every computation that passes over the xid
wraparound bounary should subtract FirstNormalTransactionId, not just
those that fall in the boundry. That would prevent the value from going
backward and still allow the mapping you liked; it isn't worth it, but
that is the right answer.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011:

> To do the right thing every computation that passes over the xid
> wraparound bounary should subtract FirstNormalTransactionId, not just
> those that fall in the boundry. That would prevent the value from going
> backward and still allow the mapping you liked; it isn't worth it, but
> that is the right answer.

This code is only concerned calculating an immediate the wrap horizon
for the autovacuuming run that's about to take place. If it's wrong in
one or three counts doesn't mean much. Consider what would happen if
load was high and it would have taken 100 extra milliseconds to get to
that bit: ReadNewTransactionId would have returned a value 3
transactions later. Furthermore, before this value is even used at all
for vacuuming, there has to be a whole lot of inter-process signalling,
a fork, and a new backend startup.

I think this should be left alone. As you said, it isn't worth it.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011:
>
> > To do the right thing every computation that passes over the xid
> > wraparound bounary should subtract FirstNormalTransactionId, not just
> > those that fall in the boundry. That would prevent the value from going
> > backward and still allow the mapping you liked; it isn't worth it, but
> > that is the right answer.
>
> This code is only concerned calculating an immediate the wrap horizon
> for the autovacuuming run that's about to take place. If it's wrong in
> one or three counts doesn't mean much. Consider what would happen if
> load was high and it would have taken 100 extra milliseconds to get to
> that bit: ReadNewTransactionId would have returned a value 3
> transactions later. Furthermore, before this value is even used at all
> for vacuuming, there has to be a whole lot of inter-process signalling,
> a fork, and a new backend startup.
>
> I think this should be left alone. As you said, it isn't worth it.

Agreed it is not worth it but I think we should at least C comment
something. I think at a minimum we should set it to
FirstNormalTransactionId.

I am not so concerned about this case but about other cases where we are
computing xid distances across the invalid range.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> > I think this should be left alone. As you said, it isn't worth it.
>
> Agreed it is not worth it but I think we should at least C comment
> something. I think at a minimum we should set it to
> FirstNormalTransactionId.
>
> I am not so concerned about this case but about other cases where we are
> computing xid distances across the invalid range.

Please ignore the patch I had attached. I realize it just moves the
problem around.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Apr 1, 2011, at 4:48 PM, Bruce Momjian wrote:
> I am not so concerned about this case but about other cases where we are
> computing xid distances across the invalid range.

Bruce, I think you hit the nail on the head earlier:

> To do the right thing every computation that passes over the xid
> wraparound bounary should subtract FirstNormalTransactionId, not just
> those that fall in the boundry.

Put another way: XID calculations should not use just +/-, but an operator (presumably a macro) that understands wraparound and the special values. Surely we have a similar problem in the code that increments XIDs, and possibly other places as well.
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Alvaro Herrera wrote:
>> Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011:
>>
>> > To do the right thing every computation that passes over the xid
>> > wraparound bounary should subtract FirstNormalTransactionId, not just
>> > those that fall in the boundry.  That would prevent the value from going
>> > backward and still allow the mapping you liked;  it isn't worth it, but
>> > that is the right answer.
>>
>> This code is only concerned calculating an immediate the wrap horizon
>> for the autovacuuming run that's about to take place.  If it's wrong in
>> one or three counts doesn't mean much.  Consider what would happen if
>> load was high and it would have taken 100 extra milliseconds to get to
>> that bit: ReadNewTransactionId would have returned a value 3
>> transactions later.  Furthermore, before this value is even used at all
>> for vacuuming, there has to be a whole lot of inter-process signalling,
>> a fork, and a new backend startup.
>>
>> I think this should be left alone.  As you said, it isn't worth it.
>
> Agreed it is not worth it but I think we should at least C comment
> something.   I think at a minimum we should set it to
> FirstNormalTransactionId.

I think you should leave it well enough alone.

> I am not so concerned about this case but about other cases where we are
> computing xid distances across the invalid range.

Such as?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Alvaro Herrera wrote:
> >> Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011:
> >>
> >> > To do the right thing every computation that passes over the xid
> >> > wraparound bounary should subtract FirstNormalTransactionId, not just
> >> > those that fall in the boundry. ?That would prevent the value from going
> >> > backward and still allow the mapping you liked; ?it isn't worth it, but
> >> > that is the right answer.
> >>
> >> This code is only concerned calculating an immediate the wrap horizon
> >> for the autovacuuming run that's about to take place. ?If it's wrong in
> >> one or three counts doesn't mean much. ?Consider what would happen if
> >> load was high and it would have taken 100 extra milliseconds to get to
> >> that bit: ReadNewTransactionId would have returned a value 3
> >> transactions later. ?Furthermore, before this value is even used at all
> >> for vacuuming, there has to be a whole lot of inter-process signalling,
> >> a fork, and a new backend startup.
> >>
> >> I think this should be left alone. ?As you said, it isn't worth it.
> >
> > Agreed it is not worth it but I think we should at least C comment
> > something. ? I think at a minimum we should set it to
> > FirstNormalTransactionId.
>
> I think you should leave it well enough alone.
>
> > I am not so concerned about this case but about other cases where we are
> > computing xid distances across the invalid range.
>
> Such as?

This causes the freeze horizon to move backward slighly, but that is OK.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Robert Haas wrote:
> On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Alvaro Herrera wrote:
> >> Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011:
> >>
> >> > To do the right thing every computation that passes over the xid
> >> > wraparound bounary should subtract FirstNormalTransactionId, not just
> >> > those that fall in the boundry. ?That would prevent the value from going
> >> > backward and still allow the mapping you liked; ?it isn't worth it, but
> >> > that is the right answer.
> >>
> >> This code is only concerned calculating an immediate the wrap horizon
> >> for the autovacuuming run that's about to take place. ?If it's wrong in
> >> one or three counts doesn't mean much. ?Consider what would happen if
> >> load was high and it would have taken 100 extra milliseconds to get to
> >> that bit: ReadNewTransactionId would have returned a value 3
> >> transactions later. ?Furthermore, before this value is even used at all
> >> for vacuuming, there has to be a whole lot of inter-process signalling,
> >> a fork, and a new backend startup.
> >>
> >> I think this should be left alone. ?As you said, it isn't worth it.
> >
> > Agreed it is not worth it but I think we should at least C comment
> > something. ? I think at a minimum we should set it to
> > FirstNormalTransactionId.
>
> I think you should leave it well enough alone.

OK, but we still need a C comment, which I suggested as:

This causes the freeze horizon to move backward slighly, but that is OK.

> > I am not so concerned about this case but about other cases where we are
> > computing xid distances across the invalid range.
>
> Such as?

Not sure. I have not had time to research this, but there might be
cases where this backward movement matters --- remember our XIDs are
valid only within about a 2 billion range, and we do less/greater
comparisons in that range (using a macro). That macro is not going to
cover over backward xid movement.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> > > I am not so concerned about this case but about other cases where we are
> > > computing xid distances across the invalid range.
> >
> > Such as?
>
> Not sure. I have not had time to research this, but there might be
> cases where this backward movement matters --- remember our XIDs are
> valid only within about a 2 billion range, and we do less/greater
> comparisons in that range (using a macro). That macro is not going to
> cover over backward xid movement.

OK, I am done training for the day, and found this macro:

/* advance a transaction ID variable, handling wraparound correctly */
#define TransactionIdAdvance(dest) \
do { \
(dest)++; \
if ((dest) < FirstNormalTransactionId) \
(dest) = FirstNormalTransactionId; \
} while(0)

which seems OK, but we the -= all over varsup.c

/*
* We'll refuse to continue assigning XIDs in interactive mode once we get
* within 1M transactions of data loss. This leaves lots of room for the
* DBA to fool around fixing things in a standalone backend, while not
* being significant compared to total XID space. (Note that since
* vacuuming requires one transaction per table cleaned, we had better be
* sure there's lots of XIDs left...)
*/
xidStopLimit = xidWrapLimit - 1000000;
if (xidStopLimit < FirstNormalTransactionId)
xidStopLimit -= FirstNormalTransactionId;

Now I am not sure where to add a C comment. :-(

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> Agreed it is not worth it but I think we should at least C comment
>> something. I think at a minimum we should set it to
>> FirstNormalTransactionId.

> I think you should leave it well enough alone.

Yes. The point of the existing coding is to ensure that we don't
overestimate the table age at which vacuums should be forced.
Bruce's proposed change would move the inaccuracy in the wrong
direction, and thus cause some cases to not force autovac though an
exact calculation would have done so. It's not worth trying to be
exactly correct here, but I don't think that we want to err in
that direction.

If we had a symbol for the max normal XID, we could instead code
like this:

if (xidForceLimit < FirstNormalTransactionId)
xidForceLimit = LastNormalTransactionId;

But AFAIR we don't, and I don't especially want to introduce one,
because people might be misled by it. As you mentioned earlier,
the XID space is circular so there isn't really a "last" XID.

regards, tom lane

Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> Agreed it is not worth it but I think we should at least C comment
> >> something. I think at a minimum we should set it to
> >> FirstNormalTransactionId.
>
> > I think you should leave it well enough alone.
>
> Yes. The point of the existing coding is to ensure that we don't
> overestimate the table age at which vacuums should be forced.
> Bruce's proposed change would move the inaccuracy in the wrong
> direction, and thus cause some cases to not force autovac though an
> exact calculation would have done so. It's not worth trying to be
> exactly correct here, but I don't think that we want to err in
> that direction.
>
> If we had a symbol for the max normal XID, we could instead code
> like this:
>
> if (xidForceLimit < FirstNormalTransactionId)
> xidForceLimit = LastNormalTransactionId;
>
> But AFAIR we don't, and I don't especially want to introduce one,
> because people might be misled by it. As you mentioned earlier,
> the XID space is circular so there isn't really a "last" XID.

Sorry for the late reply but it seems HighestNormalTransactionId might
be an apporopriate name. However, it is not code I normally deal with
so unless someone who works in this area wants to make this cleanup, I
will ignore the issue.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +