On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
> 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. Fair point. > 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. In many of the places that BufferGetPage is called there is not a snapshot available. I assume that you would be OK with an Assert that the flag was passed if the snapshot is NULL? I had been picturing what you were requesting as just adding a snapshot parameter and assuming that NULL meant not to check; adding two parameters where the flag explicitly calls that the check is not needed might do more to prevent accidents, but I do wonder how much it would help during copy/paste frenzy. Touching all spots to use the new function signature would be a mechanical job with the compiler catching any errors, so it doesn't seem crazy to refactor that now, but I would like to hear what some others think about this. > 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. We have a lot of places in our code where people need to know things that they are not reminded of by the surrounding code, but I'm not about to argue that's a good thing; if the consensus is that this would help prevent future bugs when new BufferGetPage calls are added, I can go with the flow. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
