On Mon, Sep 4, 2017 at 2:14 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: A>> that would trip it. The latter part is still in progress, because I'm > Well, PageGetLSN can be used in some hot code paths, xloginsert.c > being one, so it does not seem wise to me to switch it to something > more complicated than a macro, and also it is not bounded to any > locking contracts now.
I don't see how turning it into a static inline function is worse. We've been doing a fair amount of that lately and it generally makes things better, not worse, often avoiding multiple evaluation hazards at the same time it generates better machine code. I find this patch sort of messy; in particular, the definition of AssertPageIsLockedForLSN tries to reverse-engineer a buffer ID from a Page, and that's sort of ugly. But I think the concept of trying to make sure that our code is adhering to necessary locking rules is a pretty good one. I think the first question we ought to be asking ourselves is whether any of the PageGetLSN -> BufferGetLSNAtomic changes the patch introduces are live bugs. If they are, then we ought to fix those separately (and probably back-patch). If they are not, then we need to think about how to adjust the patch so that it doesn't complain about things that are in fact OK. -- Robert Haas EnterpriseDB: 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
