Re: Global temporary tables

On Thu, 8 Aug 2019 at 15:03, Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>
wrote:

>
>
> On 08.08.2019 5:40, Craig Ringer wrote:
>
> On Tue, 6 Aug 2019 at 16:32, Konstantin Knizhnik <
> k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
>
>> New version of the patch with several fixes is attached.
>> Many thanks to Roman Zharkov for testing.
>>
>
> FWIW I still don't understand your argument with regards to using
> shared_buffers for temp tables having connection pooling benefits. Are you
> assuming the presence of some other extension in your extended version of
> PostgreSQL ? In community PostgreSQL a temp table's contents in one backend
> will not be visible in another backend. So if your connection pooler in
> transaction pooling mode runs txn 1 on backend 42 and it populates temp
> table X, then the pooler runs the same app session's txn 2 on backend 45,
> the contents of temp table X are not visible anymore.
>
>
> Certainly here I mean built-in connection pooler which is not currently
> present in Postgres,
> but it is part of PgPRO-EE and there is my patch for vanilla at commitfest:
> https://commitfest.postgresql.org/24/2067
>

OK, that's what I assumed.

You're trying to treat this change as if it's a given that the other
functionality you want/propose is present in core or will be present in
core. That's far from given. My suggestion is to split it up so that the
parts can be reviewed and committed separately.

> In PgPRO-EE this problem was solved by binding session to backend. I.e.
> one backend can manage multiple sessions,
> but session can not migrate to another backend. The drawback of such
> solution is obvious: one long living transaction can block transactions of
> all other sessions scheduled to this backend.
> Possibility to migrate session to another backend is one of the obvious
> solutions of the problem. But the main show stopper for it is temporary
> tables.
> This is why I consider moving temporary tables to shared buffers as very
> important step.
>

I can see why it's important for your use case.

I am not disagreeing.

I am however strongly suggesting that your patch has two fairly distinct
functional changes in it, and you should separate them out.

* Introduce global temp tables, a new relkind that works like a temp table
but doesn't require catalog changes. Uses per-backend relfilenode and
cleanup like existing temp tables. You could extend the relmapper to handle
the mapping of relation oid to per-backend relfilenode.

* Associate global temp tables with session state and manage them in
shared_buffers so they can work with the in-core connection pooler (if
committed)

Historically we've had a few efforts to get in-core connection pooling that
haven't gone anywhere. Without your pooler patch the changes you make to
use shared_buffers etc are going to be unhelpful at best, if not actively
harmful to performance, and will add unnecessary complexity. So I think
there's a logical series of patches here:

* global temp table relkind and support for it
* session state separation
* connection pooling
* pooler-friendly temp tables in shared_buffers

Make sense?

> But even if we forget about built-in connection pooler, don't you think
> that possibility to use parallel query plans for temporary tables is itself
> strong enough motivation to access global temp table through shared buffers?
>

I can see a way to share temp tables across parallel query backends being
very useful for DW/OLAP workloads, yes. But I don't know if putting them in
shared_buffers is the right answer for that. We have DSM/DSA, we have
shm_mq, various options for making temp buffers share-able with parallel
worker backends.

I'm suggesting that you not tie the whole (very useful) global temp tables
feature to this, but instead split it up into logical units that can be
understood, reviewed and committed separately.

I would gladly participate in review.

Would DISCARD TEMP_BUFFERS meet your needs?
>
>
> Actually I have already implemented DropLocalBuffers function (three line
> of code:)
>
> [...]
>
I do not think that we need such command at user level (i.e. have
> correspondent SQL command).
>

I'd be very happy to have it personally, but don't think it needs to be
tied in with your patch set here. Maybe I can cook up a patch soon.

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