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. +submake-test_decoding: + $(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 parameter</primary> snapshot_valid_limit? 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? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers