Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-08 Thread Michael Paquier
On Thu, Nov 9, 2017 at 8:25 AM, Asim Praveen wrote: > Indeed, the assertion tripped during WAL replay on the standby. This was > caught by TAP tests under src/test/recovery. The assertion is now fixed so > that WAL replay is exempt from the check. Please find the new patch

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-08 Thread Asim Praveen
Hi Michael On Mon, Nov 6, 2017 at 6:18 PM, Michael Paquier wrote: > > Did you really test WAL replay? This still ignores that PageGetLSN is > as well taken in some code paths, like recovery, where actions on the > page are guaranteed to be serialized, like during

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-07 Thread Michael Paquier
On Wed, Nov 8, 2017 at 2:26 AM, Jacob Champion wrote: > On Mon, Nov 6, 2017 at 6:18 PM, Michael Paquier > wrote: >> Did you really test WAL replay? > > Is there a way to test this other than installcheck-world? The only > failure we've run into at

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-07 Thread Jacob Champion
On Tue, Nov 7, 2017 at 9:26 AM, Jacob Champion wrote: > On Mon, Nov 6, 2017 at 6:18 PM, Michael Paquier > wrote: >> It seems to me that 0001 is good for a committer lookup, that will get >> rid of all existing bugs. For 0002, what you are

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-07 Thread Jacob Champion
Hi Michael, On Mon, Nov 6, 2017 at 6:18 PM, Michael Paquier wrote: > Did you really test WAL replay? Is there a way to test this other than installcheck-world? The only failure we've run into at the moment is in the snapshot-too-old tests. Maybe we're not configuring

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-06 Thread Michael Paquier
On Tue, Nov 7, 2017 at 7:27 AM, Asim Praveen wrote: > On Mon, Oct 2, 2017 at 6:48 PM, Michael Paquier > wrote: >> Jacob, here are some ideas to make this thread move on. I would >> suggest to produce a set of patches that do things incrementally:

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-11-06 Thread Asim Praveen
Hi Michael On Mon, Oct 2, 2017 at 6:48 PM, Michael Paquier wrote: > > 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 >

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-06 Thread Michael Paquier
On Fri, Oct 6, 2017 at 7:34 PM, Alvaro Herrera wrote: > Just search for "Æsop" in the archives: > https://www.postgresql.org/message-id/CACjxUsPPCbov6DDPnuGpR=fmxhsjsn_mri3rjygvbrmcrff...@mail.gmail.com (laugh) I didn't know this one. -- Michael -- Sent via

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-06 Thread Alvaro Herrera
Michael Paquier wrote: > On Tue, Oct 3, 2017 at 12:54 AM, Alvaro Herrera > wrote: > > 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

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-03 Thread Alvaro Herrera
Michael Paquier wrote: > On Tue, Oct 3, 2017 at 12:54 AM, Alvaro Herrera > wrote: > > 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

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-02 Thread Michael Paquier
On Tue, Oct 3, 2017 at 12:54 AM, Alvaro Herrera wrote: > Michael Paquier wrote: >> On Mon, Oct 2, 2017 at 11:44 PM, Robert Haas wrote: >> > On Mon, Oct 2, 2017 at 10:19 AM, Michael Paquier >> > wrote: >> >> So those bits

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-02 Thread Robert Haas
On Mon, Oct 2, 2017 at 11:05 AM, Michael Paquier wrote: > Well, there are cases where you don't need any locking checks, and the > proposed patch ignores that. I understand that, but shouldn't we then look for a way to adjust the patch so that it doesn't have that

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-02 Thread Alvaro Herrera
Michael Paquier wrote: > On Mon, Oct 2, 2017 at 11:44 PM, Robert Haas wrote: > > On Mon, Oct 2, 2017 at 10:19 AM, Michael Paquier > > wrote: > >> So those bits could be considered for integration. > > > > Yes, and they also tend to suggest that

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-02 Thread Michael Paquier
On Mon, Oct 2, 2017 at 11:44 PM, Robert Haas wrote: > On Mon, Oct 2, 2017 at 10:19 AM, Michael Paquier > wrote: >> So those bits could be considered for integration. > > Yes, and they also tend to suggest that the rest of the patch has value.

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-02 Thread Robert Haas
On Mon, Oct 2, 2017 at 10:19 AM, Michael Paquier wrote: > So those bits could be considered for integration. Yes, and they also tend to suggest that the rest of the patch has value. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-02 Thread Michael Paquier
On Mon, Oct 2, 2017 at 9:09 PM, Robert Haas wrote: > 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

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-02 Thread Robert Haas
On Mon, Sep 4, 2017 at 2:14 AM, Michael Paquier 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 >

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-10-02 Thread Daniel Gustafsson
> On 20 Sep 2017, at 00:29, Jacob Champion wrote: > > On Wed, Sep 6, 2017 at 8:37 AM, Jacob Champion wrote: >> On Tue, Sep 5, 2017 at 10:49 PM, Michael Paquier >> wrote: >>> In short, it seems to me that this patch should

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-19 Thread Jacob Champion
On Wed, Sep 6, 2017 at 8:37 AM, Jacob Champion wrote: > On Tue, Sep 5, 2017 at 10:49 PM, Michael Paquier > wrote: >> In short, it seems to me that this patch should be rejected in its >> current shape. > > Is the half of the patch that switches

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-06 Thread Jacob Champion
On Tue, Sep 5, 2017 at 10:49 PM, Michael Paquier wrote: > On Wed, Sep 6, 2017 at 1:15 AM, Jacob Champion wrote: >> On Sun, Sep 3, 2017 at 11:14 PM, Michael Paquier >> wrote: >>> +#if defined(USE_ASSERT_CHECKING) &&

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-05 Thread Michael Paquier
On Wed, Sep 6, 2017 at 1:15 AM, Jacob Champion wrote: > On Sun, Sep 3, 2017 at 11:14 PM, Michael Paquier > wrote: >> +#if defined(USE_ASSERT_CHECKING) && !defined(FRONTEND) >> +void >> +AssertPageIsLockedForLSN(Page page) >> +{ >> From a design

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-05 Thread Jacob Champion
Hi Michael, thanks for the review! On Sun, Sep 3, 2017 at 11:14 PM, Michael Paquier wrote: >> While working on checksum support for GPDB, we noticed that several >> callers of PageGetLSN() didn't follow the correct locking procedure. > > Well, PageGetLSN can be used in

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-04 Thread Michael Paquier
On Fri, Sep 1, 2017 at 3:53 AM, Jacob Champion wrote: > The patch is really two pieces: add the assertion, and fix the callers > that would trip it. The latter part is still in progress, because I'm > running into some places where I'm not sure what the correct way > forward

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-01 Thread Robert Haas
On Fri, Sep 1, 2017 at 12:24 PM, Jacob Champion wrote: > On Fri, Sep 1, 2017 at 9:12 AM, Robert Haas wrote: >> It's a good idea to add patches to commitfest.postgresql.org > > Hi Robert, I added it yesterday as >

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-01 Thread Jacob Champion
On Fri, Sep 1, 2017 at 9:12 AM, Robert Haas wrote: > It's a good idea to add patches to commitfest.postgresql.org Hi Robert, I added it yesterday as https://commitfest.postgresql.org/14/1279/ . Let me know if I need to touch anything up there. Thanks! --Jacob -- Sent

Re: [HACKERS] [PATCH] Assert that the correct locks are held when calling PageGetLSN()

2017-09-01 Thread Robert Haas
On Thu, Aug 31, 2017 at 2:53 PM, Jacob Champion wrote: > While working on checksum support for GPDB, we noticed that several > callers of PageGetLSN() didn't follow the correct locking procedure. > To try to help ferret out present and future mistakes, we added an >