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