Re: Global temporary tables

On Fri, 9 Aug 2019 at 22:07, Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>
wrote:

>
>
> Ok, here it is: global_private_temp-1.patch
>

Initial pass review follows.

Relation name "SESSION" is odd. I guess you're avoiding "global" because
the data is session-scoped, not globally temporary. But I'm not sure
"session" fits either - after all regular temp tables are also session
temporary tables. I won't focus on naming further beyond asking that it be
consistent though, right now there's a mix of "global" in some places and
"session" in others.

Why do you need to do all this indirection with changing RelFileNode to
RelFileNodeBackend in the bufmgr, changing BufferGetTag etc? Similarly,
your changes of RelFileNodeBackendIsTemp to RelFileNodeBackendIsLocalTemp .
Did you look into my suggestion of extending the relmapper so that global
temp tables would have a relfilenode of 0 like pg_class etc, and use a
backend-local map of oid-to-relfilenode mappings? I'm guessing you did it
the way you did instead to lay the groundwork for cross-backend sharing,
but if so it should IMO be in that patch. Maybe my understanding of the
existing temp table mechanics is just insufficient as I
see RelFileNodeBackendIsTemp is already used in some aspects of existing
temp relation handling.

Similarly, TruncateSessionRelations probably shouldn't need to exist in
this patch in its current form; there's no shared_buffers use to clean and
the same file cleanup mechanism should handle both session-temp and
local-temp relfilenodes.

A number of places make a change like this:

rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP ||
+ rel->rd_rel->relpersistence == RELPERSISTENCE_SESSION

and I'd like to see a test macro or inline static for it since it's
repeated so much. Mostly to make the intent clear: "is this a relation with
temporary backend-scoped data?"

This test:

+ if (blkno == BTREE_METAPAGE && PageIsNew(BufferGetPage(buf)) &&
IsSessionRelationBackendId(rel->rd_backend))
+ _bt_initmetapage(BufferGetPage(buf), P_NONE, 0);

seems sensible but I'm wondering if there's a way to channel initialization
of global-temp objects through a bit more of a common path, so it reads
more obviously as a common test applying to all global-temp tables. "Global
temp table not initialized in session yet? Initialize it." So we don't have
to have every object type do an object type specific test for "am I
actually uninitialized?" in all paths it might hit. I guess I expected to
see something more like a

if (RelGlobalTempUninitialized(rel))

but maybe I've been doing too much Java ;)

A similar test reappears here for sequences:

+ if (rel->rd_rel->relpersistence == RELPERSISTENCE_SESSION &&
PageIsNew(page))

Why is this written differently?

Sequence initialization ignores sequence startval/firstval settings. Why?

- else if (newrelpersistence == RELPERSISTENCE_PERMANENT)
+ else if (newrelpersistence != RELPERSISTENCE_TEMP)

Doesn't this change the test outcome for RELPERSISTENCE_UNLOGGED?

In PreCommit_on_commit_actions, in the the ONCOMMIT_DELETE_ROWS case, is
there any way to still respect the XACT_FLAGS_ACCESSEDTEMPNAMESPACE flag if
the oid is for a backend-temp table not a global-temp table?

+ bool isLocalBuf = SmgrIsTemp(smgr) && relpersistence ==
RELPERSISTENCE_TEMP;

Won't session-temp tables have local buffers too? Until your next patch
that adds shared_buffers storage for them anyway?

+ * These is no need to separate them at file system level, so just
subtract SessionRelFirstBackendId
+ * to avoid too long file names.

I agree that there's no reason to be able to differentiate between
local-temp and session-temp relfilenodes at the filesystem level.

> Also I have attached updated version of the global temp tables with shared
> buffers - global_shared_temp-1.patch
> It is certainly larger (~2k lines vs. 1.5k lines) because it is changing
> BufferTag and related functions.
> But I do not think that this different is so critical.
> Still have a wish to kill two birds with one stone:)
>
>
>
>
>
>
>
>
> --
>
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise