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

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()

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()

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()

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()

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()

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()

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()

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()

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()

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()

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()

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()

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()

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()

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()

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()

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()

> 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()

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()

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()

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()

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()

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()

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()

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()

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 >

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

Hello all, 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 assertion to PageGetLSN() that checks whether those locks were being held