Michael Paquier wrote:
> 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...
I said exactly the same thing, and Kevin dismissed it.
I would be worried about your specific proposal though, because it's
easy to just call BufferGetPage() (i.e. the not-extended version) and
forget the old-snapshot protection completely.
I think a safer proposition would be to replace all current
BufferGetPage() calls (there are about 500) by adding the necessary
arguments: buffer, snapshot, rel, and an integer "flags". All this
without adding the feature. Then a subsequent commit would add the
TestForOldSnapshot inside BufferGetPage, *except* when a
BUFFER_NO_SNAPSHOT_TEST flag is passed. That way, new code always get
the snapshot test by default.
I don't like the new header dependency either, though.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services