Re: POC: Sharing record typmods between backends

On Tue, Aug 15, 2017 at 5:44 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Mon, Aug 14, 2017 at 12:32 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> But architecturally I'm still not sure I quite like the a bit ad-hoc
>> manner session state is defined here. I think we much more should go
>> towards a PGPROC like PGSESSION array, that PGPROCs reference. That'd
>> also be preallocated in "normal" shmem. From there things like the
>> handle for a dht typmod table could be referenced. I think we should
>> slowly go towards a world where session state isn't in a lot of file
>> local static variables. I don't know if this is the right moment to
>> start doing so, but I think it's quite soon.
>
> No argument from me about that general idea. All our global state is
> an obstacle for testability, multi-threading, new CPU scheduling
> architectures etc. I had been trying to avoid getting too adventurous
> here, but here goes nothing... In this version there is an honest
> Session struct. There is still a single global variable --
> CurrentSession -- which would I guess could be a candidate to become a
> thread-local variable from the future (or alternatively an argument to
> every function that needs session access). Is this better? Haven't
> tested this much yet but seems like better code layout to me.

> 0006-Introduce-a-shared-memory-record-typmod-registry.patch

+/*
+ * A struct encapsulating some elements of a user's session. For now this
+ * manages state that applies to parallel query, but it principle it could
+ * include other things that are currently global variables.
+ */
+typedef struct Session
+{
+ dsm_segment *segment; /* The session-scoped
DSM segment. */
+ dsa_area *area; /* The
session-scoped DSA area. */
+
+ /* State managed by typcache.c. */
+ SharedRecordTypmodRegistry *typmod_registry;
+ dshash_table *record_table; /* Typmods indexed by tuple
descriptor */
+ dshash_table *typmod_table; /* Tuple descriptors indexed
by typmod */
+} Session;

Upon reflection, these members should probably be called
shared_record_table etc. Presumably later refactoring would introduce
(for example) local_record_table, which would replace the following
variable in typcache.c:

static HTAB *RecordCacheHash = NULL;

... and likewise for NextRecordTypmod and RecordCacheArray which
together embody this session's local typmod registry and ability to
make more.

The idea here is eventually to move all state that is tried to a
session into this structure, though I'm not proposing to do any more
of that than is necessary as part of *this* patchset. For now I'm
just looking for a decent place to put the minimal shared session
state, but in a way that allows us "slowly [to] go towards a world
where session state isn't in a lot of file local static variables" as
you put it.

There's a separate discussion to be had about whether things like
assign_record_type_typmod() should take a Session pointer or access
the global variable (and perhaps in future thread-local)
CurrentSession, but the path of least resistance for now is, I think,
as I have it.

On another topic, I probably need to study and test some failure paths better.

Thoughts?

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