| From: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
|---|---|
| To: | Haibo Yan <tristan(dot)yim(at)gmail(dot)com> |
| Cc: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Japin Li <japinli(at)hotmail(dot)com>, Kirk Wolak <wolakk(at)gmail(dot)com> |
| Subject: | Re: Global temporary tables |
| Date: | 2026-09-17 17:28:44 |
| Message-ID: | CAEZATCUvPKqEvJUn+TP21yD_-DX=VqaJ_ggKTK1ArEGFgfRisQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Wed, 16 Sept 2026 at 04:55, Haibo Yan <tristan(dot)yim(at)gmail(dot)com> wrote:
>
> Thanks for the v11 update. I spent some more time testing the new series.
> Overall, the v11 refactoring looks good to me, particularly the simplified
> `GtrInfo` state/history, sequence handling, invalidation processing, and the
> new `DISCARD GLOBAL TEMP` implementation.
Thanks for testing.
> I reproduced a case where session A had existing rows, session B created a
> UNIQUE index, and session A could then silently insert a duplicate of one of its
> pre-existing rows. Duplicates among rows inserted afterwards were
> still detected,
> so the effective result was only partial uniqueness enforcement.
>
> I think the cleanest rule is to reject creation of a unique or
> exclusion-enforcing index when another backend is already using the relevant
> GTT. An ordinary locally-invalid index only loses an access path, while an
> incomplete unique/exclusion index loses an enforcement guarantee.
An alternative is for session A to mark the index as not ready
(indisready == false) as well as not valid locally, so that it doesn't
attempt to enforce uniqueness at all, until REINDEX is run. That has a
couple of advantages:
1). It doesn't act as a blocker for session B to create unique
indexes. Otherwise session B might have to wait for all other sessions
to exit, which might be a pain on a busy system.
2). It works better for any kind of index -- if session A continues to
execute INSERTs after the index is defined, it doesn't waste time
inserting entries into the index, which is pointless while the index
is invalid, because it can't be used in queries, and a later REINDEX
would throw the partial index away and rebuild from scratch anyway.
Since point 2 applies to all kinds of indexes, not just unique
indexes, it seems worth doing, regardless of whether we reject unique
indexes on a table being used by another backend, but I'd prefer not
to reject it, if we don't have to.
Attached is v12, doing this.
GtrInfo now has an indisready field as well as indisvalid, and I've
replaced pg_gtr_index_is_valid() with pg_gtr_index_info(), which
returns both fields.
> The second issue is `ON COMMIT DELETE ROWS`. I found that after the implicit
> truncate, the GTT can remain empty while retaining its old local
> `relfrozenxid`/`relminmxid`, which in turn keeps the backend's `tempfrozenxid`
> unnecessarily old. In one test the frozen horizon remained unchanged across
> hundreds of later transactions and advanced immediately after an explicit
> `TRUNCATE`.
>
> Since the physical truncate is nontransactional and the relation is known to be
> empty once it succeeds, I think it is safe to refresh the GTT-local freeze
> horizons at that point. The second attached patch does that using the same
> horizon values used when initializing fresh heap storage, and relies on the
> existing end-of-transaction GTT code to recompute the published PGPROC minimum.
Yes, that makes sense. I merged that into 0006, except that I didn't
like the way it was computing the freeze horizons in heap.c. That felt
like a layering violation to me, because I think it's really up to the
table access method to decide on the freeze horizons.
So I modified TableAmRoutine.relation_nontransactional_truncate to
return freezeXid and minmulti, in the same way that
TableAmRoutine.relation_set_new_filelocator does. I think that makes
sense, because they're both used to truncate a relation, so it's nice
to make them consistent.
While testing that, I noticed another issue -- vacuum() was calling
UpdateTempFrozenXids() and then vac_update_datfrozenxid(). However,
because UpdateTempFrozenXids() was only scheduling a commit-time
update to tempfrozenxid and tempminmxid, rather than updating them
immediately, the new values were not available to
vac_update_datfrozenxid(). So datfrozenxid would always end up a
little behind values in the relations just vacuumed.
To fix that, I added an "immediate" parameter to
UpdateTempFrozenXids() to allow it to optionally make the updates
immediately. That's safe from vacuum(), because the changes it makes
cannot be rolled back. Every other caller has to pass immediate ==
false, even a non-transactional truncate, because although the effects
of the truncate cannot be rolled back, there might have been other
changes in the same transaction that can be rolled back if the
transaction aborts.
> I also noticed two smaller issues that I haven't included in these patches:
>
> 1.`CREATE INDEX CONCURRENTLY` / `REINDEX CONCURRENTLY` on a GTT
> are currently
> accepted but downgraded to non-concurrent operations. The existing
> comments/docs
> justify this by saying that no other session can access a
> temporary relation,
> which isn't true for a GTT. I think the behavior should at least
> be documented
> explicitly; whether GTTs should instead reject `CONCURRENTLY` or use lighter
> locking seems like a separate design question.
Thinking about that some more, I think the best option is to reject
CONCURRENTLY for global temporary relations for now. For local
temporary relations several paths through the code do a lock upgrade,
which is OK, because no other session can access a local temporary
relation. For a global temporary relation, it's not OK (it could
deadlock).
So v12 now explicitly rejects CONCURRENTLY for GTTs, and I've updated the docs.
> 2. `repack.c:check_index_requirements()` has one direct read of
> `pg_index.indisvalid` rather than the session-effective value. The same
> file already uses `GetEffective_indisvalid()` in another path, so this
> looks like a small missed overlay site.
Ah, yes. Fixed in v12.
I also made a couple of other changes to 0001:
1). I changed RelationBuildLocalRelation() to take a char reloncommit
parameter, instead of an enum OnCommitAction parameter (doing the
translation in heap_create() instead, which is its only caller). This
saves having to #include primnodes.h from relcache.h.
2). I made a plain \d display the ON COMMIT action, rather than
requiring \d+. It's only one line of text, and it seems like
sufficiently important information to always include it for temporary
tables.
Regards,
Dean
| Attachment | Content-Type | Size |
|---|---|---|
| v12-0001-Save-temporary-table-ON-COMMIT-actions-to-pg_cla.patch | text/x-patch | 14.5 KB |
| v12-0002-Basic-support-for-global-temporary-tables.patch | text/x-patch | 238.5 KB |
| v12-0003-Add-support-for-indexes-on-global-temporary-tabl.patch | text/x-patch | 111.0 KB |
| v12-0004-Add-support-for-global-temporary-sequences.patch | text/x-patch | 42.4 KB |
| v12-0005-Support-relation-statistics-on-global-temporary-.patch | text/x-patch | 65.0 KB |
| v12-0006-Support-local-vacuuming-of-global-temporary-tabl.patch | text/x-patch | 82.8 KB |
| v12-0007-Allow-catalog-tables-to-be-global-temporary-and-.patch | text/x-patch | 72.7 KB |
| v12-0008-Add-pg_temp_statistic_ext_data-global-temporary-.patch | text/x-patch | 60.6 KB |
| v12-0009-Add-DISCARD-GLOBAL-TEMP.patch | text/x-patch | 19.9 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Haibo Yan | 2026-09-17 18:20:28 | Re: [PATCH] Use Boyer-Moore-Horspool for simple LIKE contains patterns |
| Previous Message | Masahiko Sawada | 2026-09-17 17:20:10 | Re: pgoutput: schema cache cleanup after streamed 2PC |