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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Search Discussions

  • Robert Haas at Mar 31, 2011 at 4:59 pm

    On Thu, Mar 31, 2011 at 12:17 PM, Bruce Momjian 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
  • Alvaro Herrera at Mar 31, 2011 at 5:33 pm

    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 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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Bruce Momjian at Mar 31, 2011 at 6:54 pm

    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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Bruce Momjian at Mar 31, 2011 at 6:59 pm

    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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Robert Haas at Mar 31, 2011 at 7:15 pm

    On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian 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
  • Bruce Momjian at Mar 31, 2011 at 8:17 pm

    Robert Haas wrote:
    On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian 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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Robert Haas at Mar 31, 2011 at 8:22 pm

    On Thu, Mar 31, 2011 at 4:16 PM, Bruce Momjian wrote:
    Robert Haas wrote:
    On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian 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
  • Bruce Momjian at Mar 31, 2011 at 8:35 pm

    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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Robert Haas at Mar 31, 2011 at 9:33 pm

    On Thu, Mar 31, 2011 at 4:35 PM, Bruce Momjian 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
  • Bruce Momjian at Mar 31, 2011 at 9:59 pm

    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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Greg Stark at Apr 1, 2011 at 1:54 am

    On Thu, Mar 31, 2011 at 10:59 PM, Bruce Momjian 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
  • Bruce Momjian at Apr 1, 2011 at 2:29 pm

    Greg Stark wrote:
    On Thu, Mar 31, 2011 at 10:59 PM, Bruce Momjian 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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Robert Haas at Apr 1, 2011 at 2:34 pm

    On Thu, Mar 31, 2011 at 5:59 PM, Bruce Momjian 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
  • Bruce Momjian at Apr 1, 2011 at 3:18 pm

    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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Robert Haas at Apr 1, 2011 at 4:38 pm

    On Fri, Apr 1, 2011 at 11:18 AM, Bruce Momjian 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
  • Bruce Momjian at Apr 1, 2011 at 7:50 pm

    Robert Haas wrote:
    On Fri, Apr 1, 2011 at 11:18 AM, Bruce Momjian 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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Alvaro Herrera at Apr 1, 2011 at 8:10 pm

    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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Bruce Momjian at Apr 1, 2011 at 9:49 pm

    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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Bruce Momjian at Apr 1, 2011 at 9:53 pm

    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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Jim Nasby at Apr 1, 2011 at 9:59 pm

    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@nasby.net
    512.569.9461 (cell) http://jim.nasby.net
  • Robert Haas at Apr 1, 2011 at 10:23 pm

    On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian 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
  • Bruce Momjian at Apr 1, 2011 at 10:48 pm

    Robert Haas wrote:
    On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian 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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Bruce Momjian at Apr 1, 2011 at 11:04 pm

    Robert Haas wrote:
    On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian 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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Bruce Momjian at Apr 1, 2011 at 11:11 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.
    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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Tom Lane at Apr 2, 2011 at 8:56 pm

    Robert Haas writes:
    On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian 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
  • Bruce Momjian at May 7, 2011 at 7:29 pm

    Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian 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@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedMar 31, '11 at 4:17p
activeMay 7, '11 at 7:29p
posts27
users6
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase