Обсуждение: [COMMITTERS] pgsql: Add support for coordinating record typmods among parallelworke

Add support for coordinating record typmods among parallel workers.

Tuples can have type RECORDOID and a typmod number that identifies a blessed
TupleDesc in a backend-private cache.  To support the sharing of such tuples
through shared memory and temporary files, provide a typmod registry in
shared memory.

To achieve that, introduce per-session DSM segments, created on demand when a
backend first runs a parallel query.  The per-session DSM segment has a
table-of-contents just like the per-query DSM segment, and initially the
contents are a shared record typmod registry and a DSA area to provide the
space it needs to grow.

State relating to the current session is accessed via a Session object
reached through global variable CurrentSession that may require significant
redesign further down the road as we figure out what else needs to be shared
or remodelled.

Author: Thomas Munro
Reviewed-By: Andres Freund
Discussion: https://postgr.es/m/CAEepm=0ZtQ-SpsgCyzzYpsXS6e=kZWqk3g5Ygn3MDV7A8dabUA@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/cc5f81366c36b3dd8f02bd9be1cf75b2cc8482bd

Modified Files
--------------
src/backend/access/common/Makefile    |   2 +-
src/backend/access/common/session.c   | 208 +++++++++++
src/backend/access/common/tupdesc.c   |  16 +
src/backend/access/transam/parallel.c |  44 ++-
src/backend/storage/lmgr/lwlock.c     |   8 +-
src/backend/utils/cache/typcache.c    | 634 ++++++++++++++++++++++++++++++++--
src/backend/utils/init/postinit.c     |   4 +
src/include/access/session.h          |  44 +++
src/include/access/tupdesc.h          |   6 +
src/include/storage/lwlock.h          |   3 +
src/include/utils/typcache.h          |  10 +
src/tools/pgindent/typedefs.list      |   4 +
12 files changed, 946 insertions(+), 37 deletions(-)


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

Andres Freund <andres@anarazel.de> writes:
> Add support for coordinating record typmods among parallel workers.

Buildfarm not happy ...
        regards, tom lane


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

On Fri, Sep 15, 2017 at 3:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Add support for coordinating record typmods among parallel workers.
>
> Buildfarm not happy ...

My compiler, C++ and more recent C standards are OK with identical
redefinition of a typedef like that, but not the older standards or
those particular compilers.  D'oh.  (I should figure out how to make
my automatic patch tester this fussy).  I think we should probably
just do this:

diff --git a/src/include/access/session.h b/src/include/access/session.h
index 8376dc53127..910a9815d78 100644
--- a/src/include/access/session.h
+++ b/src/include/access/session.h
@@ -13,9 +13,7 @@#define SESSION_H
#include "lib/dshash.h"
-
-/* Defined in typcache.c */
-typedef struct SharedRecordTypmodRegistry SharedRecordTypmodRegistry;
+#include "utils/typcache.h"
/* * A struct encapsulating some elements of a user's session.  For now this

-- 
Thomas Munro
http://www.enterprisedb.com


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

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> My compiler, C++ and more recent C standards are OK with identical
> redefinition of a typedef like that, but not the older standards or
> those particular compilers.  D'oh.  (I should figure out how to make
> my automatic patch tester this fussy).  I think we should probably
> just do this:

Our usual locution for this sort of thing is to use
"struct SharedRecordTypmodRegistry" in headers instead of
the typedef name, if we don't want to pull in the header
where the typedef is defined.  It might be all right to do
what you suggest, but we've been burnt in the past by circular
dependencies and/or pulling some header into essentially
the entire build (which tends to lead to a mess down the road).
I'd generally lean in the direction of not adding #includes to
header files except where absolutely necessary.
        regards, tom lane


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

On September 14, 2017 8:38:09 PM PDT, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
>On Fri, Sep 15, 2017 at 3:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> Add support for coordinating record typmods among parallel workers.
>>
>> Buildfarm not happy ...
>
>My compiler, C++ and more recent C standards are OK with identical
>redefinition of a typedef like that, but not the older standards or
>those particular compilers.  D'oh.  (I should figure out how to make
>my automatic patch tester this fussy).  I think we should probably
>just do this:
>
>diff --git a/src/include/access/session.h
>b/src/include/access/session.h
>index 8376dc53127..910a9815d78 100644
>--- a/src/include/access/session.h
>+++ b/src/include/access/session.h
>@@ -13,9 +13,7 @@
> #define SESSION_H
>
> #include "lib/dshash.h"
>-
>-/* Defined in typcache.c */
>-typedef struct SharedRecordTypmodRegistry SharedRecordTypmodRegistry;
>+#include "utils/typcache.h"
>
> /*
>* A struct encapsulating some elements of a user's session.  For now
>this

Sorry for missing that during review - unfortunately I don't have a computer with me now - so I won't get around to
thistill tomorrow... 

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.


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

Andres Freund <andres@anarazel.de> writes:
> Sorry for missing that during review - unfortunately I don't have a computer with me now - so I won't get around to
thistill tomorrow... 

It turns out that this breaks my local build, too, so I went ahead and
pushed a fix.

At least two buildfarm members think there's something deeper broken
here:

2017-09-15 00:08:34.061 EDT [97092:75] LOG:  worker process: parallel worker for PID 101175 (PID 101256) was terminated
bysignal 11: Segmentation fault 
2017-09-15 00:08:34.061 EDT [97092:76] DETAIL:  Failed process was running: select * from
pg_get_object_address('operatorof access method', '{btree,integer_ops,1}', '{int4,bool}'); 

I wonder if this indicates you forgot to consider transmission of state
to parallel worker processes.
        regards, tom lane


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

On Fri, Sep 15, 2017 at 3:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> My compiler, C++ and more recent C standards are OK with identical
>> redefinition of a typedef like that, but not the older standards or
>> those particular compilers.  D'oh.  (I should figure out how to make
>> my automatic patch tester this fussy).  I think we should probably
>> just do this:
>
> Our usual locution for this sort of thing is to use
> "struct SharedRecordTypmodRegistry" in headers instead of
> the typedef name, if we don't want to pull in the header
> where the typedef is defined.  It might be all right to do
> what you suggest, but we've been burnt in the past by circular
> dependencies and/or pulling some header into essentially
> the entire build (which tends to lead to a mess down the road).
> I'd generally lean in the direction of not adding #includes to
> header files except where absolutely necessary.

Here's a version that just forward declares SharedRecordTypmodRegistry
in session.h (but keeps the typedef in typcache.h where it is useful
for shorter function prototypes).  Neither GCC 6 -Wall -std=c89 not
Clang -Wall -std=c89 complained about this problem.  I found an old
GCC 4.3 compiler and it complained about the typedef redefinition and
also the use of an anonymous union, also fixed in the attached.  Sorry
about that -- clearly I need to test my patches on more compilers in
future.

-- 
Thomas Munro
http://www.enterprisedb.com

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

On Fri, Sep 15, 2017 at 4:34 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> Here's a version that just forward declares SharedRecordTypmodRegistry
> in session.h (but keeps the typedef in typcache.h where it is useful
> for shorter function prototypes).  Neither GCC 6 -Wall -std=c89 not
> Clang -Wall -std=c89 complained about this problem.  I found an old
> GCC 4.3 compiler and it complained about the typedef redefinition and
> also the use of an anonymous union, also fixed in the attached.  Sorry
> about that -- clearly I need to test my patches on more compilers in
> future.

Rebased on your fix.  Investigating the runtime failure...

-- 
Thomas Munro
http://www.enterprisedb.com

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

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Sorry about that -- clearly I need to test my patches on more compilers
> in future.

Don't sweat about it too much, this is exactly what the buildfarm is for.
        regards, tom lane


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

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Rebased on your fix.

Ah, yet a different issue :-(.  Pushed that too.

> Investigating the runtime failure...

Looks like force_parallel_mode might be the key to causing that.
        regards, tom lane


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

On Fri, Sep 15, 2017 at 4:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Looks like force_parallel_mode might be the key to causing that.

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.

-- 
Thomas Munro
http://www.enterprisedb.com

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

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

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.
        regards, tom lane


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

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> 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.

In hopes of getting the buildfarm back to green, I went ahead and pushed
a slightly more aggressive form of this --- I took out the worker-specific
detach function altogether.  We can resurrect it if we ever have a more
plausible idea of how to do worker recycling, but for now I don't see a
reason to waste bytes on useless and incorrect code.
        regards, tom lane


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

On 2017-09-15 00:59:35 -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
> > Rebased on your fix.
> 
> Ah, yet a different issue :-(.  Pushed that too.

Thanks for pushing the fixes - I'd waited till the first few animals
came back green, and then went to somebody's birthday
celebrations. Didn't wait long enough...


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

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

Andres Freund <andres@anarazel.de> writes:
> On 2017-09-15 10:33:37 -0400, Tom Lane wrote:
>> 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?

Resetting NextRecordTypmod to zero means that we could reuse a typmod
that was previously used.  Maybe that's safe because no memory of the
old record definition exists anywhere else in the process, but I'm
not exactly convinced of that.  For that matter, I'm not exactly
convinced that nothing else is holding on to an actual pointer to the
record tupdesc, making it moot whether or not we flush typcache.c's
pointer.  (I'm pretty sure that plpgsql, for one, might hold onto
tupdesc pointers it gets from the typcache.  That's why they have
refcounts in the first place.)  There's never previously been any
expectation that record typmod registrations weren't good for the life
of the process, so I don't trust any of the assumptions this code
wants to make.

It's moot as long as we're not reusing workers, anyway: nothing is
going to touch any of the record-tupdesc data before process exit.
        regards, tom lane


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

On 2017-09-15 15:07:14 -0400, Tom Lane wrote:
> It's moot as long as we're not reusing workers, anyway: nothing is
> going to touch any of the record-tupdesc data before process exit.

Right - I'm ok with the end-result, I was just a bit confused about the
justification, because neither before nor after the patch it's safe to
continue executing arbitrary things in the worker. Just unsafe in
different ways. Before you could reuse a typemod, now you can use a
dangling pointer.

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

On Sat, Sep 16, 2017 at 7:07 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2017-09-15 10:33:37 -0400, Tom Lane wrote:
>>> 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?
>
> Resetting NextRecordTypmod to zero means that we could reuse a typmod
> that was previously used.  Maybe that's safe because no memory of the
> old record definition exists anywhere else in the process, but I'm
> not exactly convinced of that.  For that matter, I'm not exactly
> convinced that nothing else is holding on to an actual pointer to the
> record tupdesc, making it moot whether or not we flush typcache.c's
> pointer.  (I'm pretty sure that plpgsql, for one, might hold onto
> tupdesc pointers it gets from the typcache.  That's why they have
> refcounts in the first place.)  There's never previously been any
> expectation that record typmod registrations weren't good for the life
> of the process, so I don't trust any of the assumptions this code
> wants to make.
>
> It's moot as long as we're not reusing workers, anyway: nothing is
> going to touch any of the record-tupdesc data before process exit.

Thanks for straightening this out.  I think that what I was trying to
do (bugs aside) is probably the right direction, but you're quite
right that there are plenty more problems to solve before it could
actually work, so my forward looking but incomplete code looks a bit
goofy in hindsight.  I have this vague notion that we should
eventually hunt down and separate all forms of caching that are
dependent on the current session (references to session-specific
typmods being the only example I can think of right now) and separate
them from caching that is OK between different sessions (since reused
backends might as well benefit from prewarmed caches where it makes
sense).  For example, if plpgsql is holding onto RECORD TupleDescs or
typmods it would need to stop doing that when it receives a
session-going-away callback, or store its state in some data structure
connected to the current session, or <insert hand waving>.

-- 
Thomas Munro
http://www.enterprisedb.com


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