Re: Global temporary tables

On 13.08.2019 11:21, Craig Ringer wrote:
> On Fri, 9 Aug 2019 at 22:07, Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)ru <mailto: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.
>
I have supported both forms "create session table" and "create global temp".
Both "session" and "global" are expected PostgreSQL keywords so we do
not need to introduce new one (unlike "public" or "shared").
The form "global temp" is used in Oracle so it seems to be natural to
provide similar syntax.

I am not insisting on this syntax and will consider all other suggestions.
But IMHO almost any SQL keyword is overloaded and have different meaning.
Temporary tables has session as living area (or transaction if created
with "ON COMMIT DROP" clause).
So calling them "session tables" is actually more clear than just
"temporary tables".
But "local temp tables" can be also considered as session tables. So may
be it is really not so good idea
to use "session" keyword for them.

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

Sorry, are you really speaking about global_private_temp-1.patch?
This patch doesn't change bufmgr file at all.
May be you looked at another patch - global_shared_temp-1.patch
which is accessing shared tables though shared buffers and so have to
change buffer tag to include backend ID in it.

>
> 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.
>
In global_private_temp-1.patch TruncateSessionRelations does nothing
with shared buffers, it just delete relation files.

>
> 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?"
>
I consider to call such macro IsSessionRelation() or something like this
but you do not like notion "session".
Is IsBackendScopedRelation() a better choice?

>
> 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?
>
>
Just because I wrote them in different moment of times:)
I think that adding RelGlobalTempUninitialized is really good idea.

> Sequence initialization ignores sequence startval/firstval settings. Why?
>
>
I am handling only case of implicitly created sequences for
SERIAL/BIGSERIAL columns.
Is it possible to explicitly specify initial value and step for them?
If so, this place should definitely be rewritten.

> - else if (newrelpersistence == RELPERSISTENCE_PERMANENT)
> + else if (newrelpersistence != RELPERSISTENCE_TEMP)
>
> Doesn't this change the test outcome for RELPERSISTENCE_UNLOGGED?
>
RELPERSISTENCE_UNLOGGED case is handle in previous IF branch.

>
> 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?
>
If it is local temp table, then XACT_FLAGS_ACCESSEDTEMPNAMESPACE is set
and  so there is no need to check this flag.
And as far as XACT_FLAGS_ACCESSEDTEMPNAMESPACE is not set now  for
global temp tables, I have to remove this check.
So for local temp table original behavior is preserved.

The question is why I do not set XACT_FLAGS_ACCESSEDTEMPNAMESPACE for
global temp tables?
I wanted to avoid current limitation for using temp tables in prepared
transactions.
Global metadata allows to eliminate some problems related with using
temp tables in 2PC.
But I am not sure that it eliminates ALL problems and there are no
strange effects related with
prepared transactions&global temp tables.

>
> + 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?
>
Once again, it is change from global_shared_temp-1.patch, not from
global_private_temp-1.patch

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company