Date: Fri, 15 Sep 2017 11:14:36 -0700
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Thomas Munro <thomas.munro@enterprisedb.com>,
	pgsql-committers <pgsql-committers@postgresql.org>
Subject: Re: pgsql: Add support for coordinating record typmods
 among parallel worke
Message-ID: <20170915181436.u6j27rx65giyntmk@alap3.anarazel.de>
References: <E1dsguX-00056N-9x@gemulon.postgresql.org>
 <13971.1505445491@sss.pgh.pa.us>
 <CAEepm=08r6ssWnM7wOAN=omz=s9DHujsJjJY6SN_PTHayPR0hw@mail.gmail.com>
 <14932.1505447120@sss.pgh.pa.us>
 <CAEepm=13KfNVsjzvdz_u4E_3_joAWfL+k6TNaFkr-=WTT_Ou=A@mail.gmail.com>
 <CAEepm=2q2OoJ5K1Mg2_XaqjSSRbMTAmE4V7fsM5O3yTWWkb2aw@mail.gmail.com>
 <29888.1505451575@sss.pgh.pa.us>
 <CAEepm=0y78KA-UFVA=q8joA4m9tPre5y7VxMy2Jzigo+O6E6MA@mail.gmail.com>
 <21075.1505486017@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <21075.1505486017@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-committers-owner@postgresql.org

On 2017-09-15 10:33:37 -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
> > Yeah.  The regress tests in d36f7efb39e1b9613193b2e12717dbe2418ddae5
> > tested this stuff in parallel mode, but only a happy case to
> > demonstrate blessed RECORD types travelling through tqueue.c correctly
> > before and after ripping out the remapping stuff.  The problem here
> > was that I do some cleanup in the DSM detach hook of the new session
> > segment, and that involved reading data that could point to another
> > segment (if the DSA area needed more space).  That can blow up if that
> > other segment happens to have been unmapped first in the arbitrary
> > order of resource cleanup in an aborting worker.  I do have some
> > thoughts on how to solve that general problem, but in this case it's
> > completely unnecessary anyway.  The attached patch fixes the problem
> > by getting rid of the code that accesses shmem in the detach hook.
> > With this patch applied installcheck survives on a cluster with
> > force_parallel_worker=regress.
> 
> I don't much like your proposed comment; the only way that this code
> is even approximately correct is if we're exiting the process and
> will never touch the RecordCacheArray again.  (Otherwise, it risks
> reassigning a previously used local typmod.)

How'd it reuse it after running the detach hook in workers?


> My inclination is to just leave the local data structures alone.
> There's nothing we can do to them that doesn't create more problems
> than it solves, if the process continues to use the cache.

The problem is that RecordCacheArray[n] can point into shared
memory. Which means that the pushed change leaves around danging
pointers into shared memory - unless I'm missing something (didn't yet
have coffee, had to run to the store).

Greetings,

Andres Freund


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers