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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
