Kevin Grittner wrote: > On Wed, Mar 30, 2016 at 11:37 AM, Tom Lane <[email protected]> wrote: > > Alvaro Herrera <[email protected]> writes: > >> 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. > > > > That seems awfully invasive, > > That's the argument I made, which Álvaro described as "dismissing" > his suggestion. In this post from October of 2015, I pointed out > that there are 36 calls where we need a snapshot and 450 where we > don't. > > http://www.postgresql.org/message-id/[email protected]
I understand the invasiveness argument, but to me the danger of introducing new bugs trumps that. The problem is not the current code, but future patches: it is just too easy to make the mistake of not checking the snapshot in new additions of BufferGetPage. And you said that the result of missing a check is silent wrong results from queries that should instead be cancelled, which seems fairly bad to me. My impression was that you were actually considering doing something about that -- sorry for the lack of clarity. We have made similarly invasive changes in the past -- the SearchSysCache API for instance. > > not to mention performance-killing if the expectation is that > > most such calls are going to need a snapshot check. > > This patch is one which has allowed a customer where we could not > meet their performance requirements to pass them. It is the > opposite of performance-killing. I think Tom misunderstood what I said and you misunderstood what Tom said. Let me attempt to set things straight. I said that we should change BufferGetPage into having the snapshot check built-in, except in the cases where a flag is passed; and the flag would be passed in all cases except those 30-something you identified. In other words, the behavior in all the current callsites would be identical to what's there today; we could have a macro do the first check so that we don't introduce the overhead of a function call in the 450 cases where it's not needed. Tom said that my proposal would be performance-killing, not that your patch would be performance-killing. But as I argue above, with my proposal performance would stay the same, so we're actually okay. I don't think nobody disputes that your patch is good in general. I would be happy with it in 9.6, but I have my reservations about the aforementioned problem. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
