Fix two bugs in MaintainOldSnapshotTimeMapping.

The previous coding was confused about whether head_timestamp was
intended to represent the timestamp for the newest bucket in the
mapping or the oldest timestamp for the oldest bucket in the mapping.
Decide that it's intended to be the oldest one, and repair
accordingly.

To do that, we need to do two things. First, when advancing to a
new bucket, don't categorically set head_timestamp to the new
timestamp. Do this only if we're blowing out the map completely
because a lot of time has passed since we last maintained it. If
we're replacing entries one by one, advance head_timestamp by
1 minute for each; if we're filling in unused entries, don't
advance head_timestamp at all.

Second, fix the computation of how many buckets we need to advance.
The previous formula would be correct if head_timestamp were the
timestamp for the new bucket, but we're now making all the code
agree that it's the timestamp for the oldest bucket, so adjust the
formula accordingly.

This is certainly a bug fix, but I don't feel good about
back-patching it without the introspection tools added by commit
aecf5ee2bb36c597d3c6142e367e38d67816c777, and perhaps also some
actual tests. Since back-patching the introspection tools might
not attract sufficient support and since there are no automated
tests of these fixes yet, I'm just committing this to master for
now.

Patch by me, reviewed by Thomas Munro, Dilip Kumar, Hamid Akhtar.

Discussion: http://postgr.es/m/CA+TgmoY=aqf0zjTD+3dUWYkgMiNDegDLFjo+6ze=Wtpik+3XqA@mail.gmail.com

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 650c2aa815953a970e8ca2318d50877b8cad689c..8c41483e87c52cef1dc6d339932c8e35ccee2348 100644 (file)
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1949,10 +1949,32 @@ MaintainOldSnapshotTimeMapping(TimestampTz whenTaken, TransactionId xmin)
    else
    {
        /* We need a new bucket, but it might not be the very next one. */
-       int         advance = ((ts - oldSnapshotControl->head_timestamp)
-                              / USECS_PER_MINUTE);
+       int         distance_to_new_tail;
+       int         distance_to_current_tail;
+       int         advance;
 
-       oldSnapshotControl->head_timestamp = ts;
+       /*
+        * Our goal is for the new "tail" of the mapping, that is, the entry
+        * which is newest and thus furthest from the "head" entry, to
+        * correspond to "ts". Since there's one entry per minute, the
+        * distance between the current head and the new tail is just the
+        * number of minutes of difference between ts and the current
+        * head_timestamp.
+        *
+        * The distance from the current head to the current tail is one
+        * less than the number of entries in the mapping, because the
+        * entry at the head_offset is for 0 minutes after head_timestamp.
+        *
+        * The difference between these two values is the number of minutes
+        * by which we need to advance the mapping, either adding new entries
+        * or rotating old ones out.
+        */
+       distance_to_new_tail =
+           (ts - oldSnapshotControl->head_timestamp) / USECS_PER_MINUTE;
+       distance_to_current_tail =
+           oldSnapshotControl->count_used - 1;
+       advance = distance_to_new_tail - distance_to_current_tail;
+       Assert(advance > 0);
 
        if (advance >= OLD_SNAPSHOT_TIME_MAP_ENTRIES)
        {
@@ -1960,6 +1982,7 @@ MaintainOldSnapshotTimeMapping(TimestampTz whenTaken, TransactionId xmin)
            oldSnapshotControl->head_offset = 0;
            oldSnapshotControl->count_used = 1;
            oldSnapshotControl->xid_by_minute[0] = xmin;
+           oldSnapshotControl->head_timestamp = ts;
        }
        else
        {
@@ -1978,6 +2001,7 @@ MaintainOldSnapshotTimeMapping(TimestampTz whenTaken, TransactionId xmin)
                    else
                        oldSnapshotControl->head_offset = old_head + 1;
                    oldSnapshotControl->xid_by_minute[old_head] = xmin;
+                   oldSnapshotControl->head_timestamp += USECS_PER_MINUTE;
                }
                else
                {