On Tue, Oct 3, 2017 at 12:54 AM, Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > Michael Paquier wrote: >> On Mon, Oct 2, 2017 at 11:44 PM, Robert Haas <robertmh...@gmail.com> wrote: >> > On Mon, Oct 2, 2017 at 10:19 AM, Michael Paquier >> > <michael.paqu...@gmail.com> wrote: >> >> So those bits could be considered for integration. >> > >> > Yes, and they also tend to suggest that the rest of the patch has value. >> >> Well, there are cases where you don't need any locking checks, and the >> proposed patch ignores that. Take for example pageinspect which works >> on a copy of a page, or just WAL replay which serializes everything, >> and in both cases PageGetLSN can be used safely. So for compatibility >> reasons I think that PageGetLSN should be kept alone to not surprise >> anybody using it, or at least an equivalent should be introduced. It >> would be interesting to make BufferGetLSNAtomic hold tighter checks, >> like something that makes use of LWLockHeldByMe for example.
Jacob, here are some ideas to make this thread move on. I would suggest to produce a set of patches that do things incrementally: 1) One patch that changes the calls of PageGetLSN to BufferGetLSNAtomic which are now not appropriate. You have spotted some of them in the btree and gist code, but not all based on my first lookup. There is still one in gistFindCorrectParent and one in btree _bt_split. The monitoring of the other calls (sequence.c and vacuumlazy.c) looked safe. There is another one in XLogRecordAssemble that should be fixed I think. 2) A second patch that strengthens a bit checks around BufferGetLSNAtomic. One idea would be to use LWLockHeldByMe, as you are originally suggesting. A comment could be as well added in bufpage.h for PageGetLSN to let users know that it should be used carefully, in the vein of what is mentioned in src/backend/access/transam/README. > I'd argue about this in the same direction I argued about > BufferGetPage() needing an LSN check that's applied separately: if it's > too easy for a developer to do the wrong thing (i.e. fail to apply the > separate LSN check after reading the page) then the routine should be > patched to do the check itself, and another routine should be offered > for the cases that explicitly can do without the check. > > I was eventually outvoted in the BufferGetPage() saga, though, so I'm > not sure that that discussion sets precedent. Oh... I don't recall this discussion. A quick lookup at the archives does not show me a specific thread either. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
