On Tue, Mar 22, 2016 at 5:05 AM, Kevin Grittner <kgri...@gmail.com> wrote:
> Thanks to all for the feedback; I will try to respond later this
> week.  First I'm trying to get my reviews for other patches posted.

I have been looking at 4a, the test module, and things are looking
good IMO. Something that I think would be adapted would be to define
the options for isolation tests in a variable, like ISOLATION_OPTS to
allow MSVC scripts to fetch those option values more easily.

+   $(MAKE) -C $(top_builddir)/src/test/modules/snapshot_too_old
The target name here is incorrect. This should refer to snapshot_too_old.

Regarding the main patch:
+        <primary><varname>old_snapshot_threshold</> configuration

    page = BufferGetPage(buf);
+   TestForOldSnapshot(scan->xs_snapshot, rel, page);
This is a sequence repeated many times in this patch, a new routine,
say BufferGetPageExtended with a uint8 flag, one flag being used to
test old snapshots would be more adapted. But this would require
creating a header dependency between the buffer manager and
SnapshotData.. Or more simply we may want a common code path when
fetching a page that a backend is going to use to fetch tuples. I am
afraid of TestForOldSnapshot() being something that could be easily
forgotten in code paths introduced in future patches...

+   if (whenTaken < 0)
+   {
+       elog(DEBUG1,
+            "MaintainOldSnapshotTimeMapping called with negative
whenTaken = %ld",
+            (long) whenTaken);
+       return;
+   }
+   if (!TransactionIdIsNormal(xmin))
+   {
+       elog(DEBUG1,
+            "MaintainOldSnapshotTimeMapping called with xmin = %lu",
+            (unsigned long) xmin);
+       return;
+   }
Material for two assertions?

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to