Re: Global temporary tables

On Fri, 9 Aug 2019 at 22:07, Konstantin Knizhnik <k.knizhnik@postgrespro.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