Re: Shared buffer access rule violations?

From: Andres Freund <andres(at)anarazel(dot)de>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Asim R P <apraveen(at)pivotal(dot)io>, Peter Geoghegan <pg(at)bowt(dot)ie>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, dkimura(at)pivotal(dot)io
Subject: Re: Shared buffer access rule violations?
Date: 2019-02-03 10:31:38
Message-ID: 20190203103138.smsadzawhki3i5ts@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On 2019-01-13 19:52:32 -0500, Tom Lane wrote:
> Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
> > On Thu, Aug 9, 2018 at 12:59 PM Asim R P <apraveen(at)pivotal(dot)io> wrote:
> >> On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> >>> I wonder if it would be a better idea to enable Valgrind's memcheck to
> >>> mark buffers as read-only or read-write.
>
> >> Basic question: how do you mark buffers as read-only using memcheck
> >> tool? Running installcheck with valgrind didn't uncover any errors:
>
> > Presumably with VALGRIND_xxx macros, but is there a way to make that
> > work for shared memory?
>
> > I like the mprotect() patch. This could be enabled on a build farm
> > animal.
>
> I think this is a cute idea and potentially useful as an alternative
> to valgrind, but I don't like the patch much. It'd be better to
> set things up so that the patch adds support for catching bad accesses
> with either valgrind or mprotect, according to compile options. Also,
> this sort of thing is gratitously ugly:
>
> +#ifdef MPROTECT_BUFFERS
> + BufferMProtect(buf, PROT_NONE);
> +#endif
>
> The right way IMO is to just have macro calls like
>
> ProtectBuffer(buf, NO_ACCESS);
>
> which expand to nothing at all if the feature isn't enabled by #ifdefs,
> and otherwise to whatever we need it to do. (The access-type symbols
> then need to be something that can be defined correctly for either
> implementation.)

As this has not been addressed since, and the CF has ended, I'm marking
this patch as returned with feedback. Please resubmit once that's
addressed.

> > I guess it would either fail explicitly or behave incorrectly
> > for VM page size > BLCKSZ depending on OS, but at least on Linux/amd64
> > you have to go out of your way to get pages > 4KB so that seems OK for
> > a debugging feature.
>
> What worries me more is that I don't think we try hard to ensure that
> buffers are aligned on system page boundaries.

It's probably worthwhile to just always force that, to reduce
unnecessary page misses.

Greetings,

Andres Freund

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2019-02-03 10:34:36 Re: Tid scan improvements
Previous Message Andres Freund 2019-02-03 10:28:18 Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT