On Mon, Sep 4, 2017 at 2:14 AM, Michael Paquier
<michael.paquier@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