Hi, Dean
On Thu, 02 Jul 2026 at 17:11, Japin Li <[email protected]> wrote: > Hi, Dean > > On Wed, 01 Jul 2026 at 08:32, Dean Rasheed <[email protected]> wrote: >> On Wed, 1 Jul 2026 at 07:24, Japin Li <[email protected]> wrote: >>> >>> Thanks for working on this. >> >> Thank you for testing it! >> >>> While testing the v5 patch, I encountered a lock wait. >>> >>> 2026-07-01 14:18:43.603 CST [1486593] LOG: process 1486593 still waiting >>> for AccessExclusiveLock on relation 16384 of database 5 after 1000.106 ms >>> 2026-07-01 14:18:43.603 CST [1486593] DETAIL: Process holding the lock: >>> 1486484. Wait queue: 1486593. >>> 2026-07-01 14:18:43.603 CST [1486593] CONTEXT: waiting for >>> AccessExclusiveLock on relation 16384 of database 5 >>> 2026-07-01 14:18:43.603 CST [1486593] STATEMENT: SELECT * FROM gtt_delete; >>> >>> Is this expected? >> >> Ah yes, you're right. That's not good. >> >> The root cause looks to be the same issue that Pavel reported -- the >> ON COMMIT DELETE ROWS does a TRUNCATE when the transaction is >> committed, which requires an AccessExclusiveLock. I think the patch >> should reduce the lock level required for TRUNCATE on a global >> temporary relation to RowExclusiveLock, like DELETE. >> > > Here is my review on v5-0002. Please take a look. > > 1. > $ git am ~/Patches/gtt/*.patch > Applying: Save temporary table ON COMMIT actions to pg_class. > Applying: Basic support for global temporary tables. > Applying: Support indexes on global temporary tables. > Applying: Support global temporary sequences. > Applying: Support global temporary catalog tables and add pg_temp_class. > Applying: Add relation statistics columns to pg_temp_class. > Applying: Add relfrozenxid and relminmxid columns to pg_temp_class. > .git/rebase-apply/patch:940: new blank line at EOF. > + > warning: 1 line adds whitespace errors. > Applying: Add pg_temp_statistic global temporary catalog table. > Applying: Add pg_temp_statistic_ext_data global temporary catalog table. > Applying: Add pg_temp_index global temporary catalog table. > > 2. > +static void > +gtr_delete_all_storage_on_exit(int code, Datum arg) > +{ > + ProcNumber backend; > + HASH_SEQ_STATUS status; > + GtrStorageEntry *entry; > + SMgrRelation srel; > + > + /* Loop over all storage created and delete it */ > + backend = ProcNumberForTempRelations(); > + hash_seq_init(&status, gtr_local_storage); > + while ((entry = hash_seq_search(&status)) != NULL) > + { > + srel = smgropen(entry->rlocator, backend); > + smgrdounlinkall(&srel, 1, false); > + smgrclose(srel); > + } > +} > > We can restrict srel to the loop scope. > > 3. > +gtr_remove_all_usage_on_exit(int code, Datum arg) > +{ > + HASH_SEQ_STATUS status; > + GtrUsageEntry *local_entry; > + GtrSharedUsageKey key; > + GtrSharedUsageEntry *shared_entry; > + > + /* Loop over all the global temporary relations we were using */ > + hash_seq_init(&status, gtr_local_usage); > + while ((local_entry = hash_seq_search(&status)) != NULL) > + { > + /* Find the shared usage entry */ > + key.dbid = MyDatabaseId; > + key.relid = local_entry->relid; > + shared_entry = dshash_find(gtr_shared_usage, &key, true); > > Same as above: move the declarations of key and shared_entry inside the loop. > > 4. > +static void > +gtr_init_usage_tables(void) > +{ > + HASHCTL ctl; > + MemoryContext oldcontext; > + > + /* Local usage table */ > + if (gtr_local_usage == NULL) > + { > + ctl.keysize = sizeof(Oid); > + ctl.entrysize = sizeof(GtrUsageEntry); > + > + gtr_local_usage = hash_create("Global temporary relations in > use locally", > + 128, > &ctl, HASH_ELEM | HASH_BLOBS); > + } > + > + /* Shared usage table */ > + if (gtr_shared_usage == NULL) > + { > + /* Use a lock to ensure only one process creates the table */ > + LWLockAcquire(GlobalTempRelControlLock, LW_EXCLUSIVE); > + > + /* Be sure any local memory allocated by DSA routines is > persistent */ > + oldcontext = MemoryContextSwitchTo(TopMemoryContext); > > Likewise, narrow ctl to the first if and oldContext to the second. > > 5. > + * the usage hash table entries for it. Otherwise, reset the hash > entry's > + * subids to zero. > > Instead of zero, how about using an invalid sub-transaction ID? > > 6. > +static void > +gtr_process_invalidated_gtrs(void) > +{ > + MemoryContext oldcontext; > + Oid relid; > + > + /* > + * Scan the gtrs_invalidated list and add any dropped relations to the > + * gtrs_dropped list. Since the transaction might fail later on, we > need > + * the gtrs_dropped list to persist until we can successfully process > it. > + */ > + oldcontext = MemoryContextSwitchTo(TopMemoryContext); > + > + /* > + * As we scan the gtrs_invalidated list, more invalidation messages > might > + * arrive, so keep going until it is empty. > + */ > + while (gtrs_invalidated != NIL) > + { > + /* Pop the last relid from the list */ > + relid = llast_oid(gtrs_invalidated); > > Restrict relid to the while loop scope. > > 7. > +void > +TrackGlobalTempRelationStorage(Oid relid, RelFileLocator rlocator, > + ProcNumber backend, > bool create) > +{ > + GtrStorageEntry *entry; > + bool found; > + SMgrRelation srel; > + > + if (create) > + { > + /* Initialize the storage table, if necessary */ > + gtr_init_storage_table(); > + > + /* Insert an entry to track the storage */ > + entry = hash_search(gtr_local_storage, &rlocator, HASH_ENTER, > &found); > > Likewise, restrict found and srel to the if statement. > > 8. > + /* Register the ON COMMIT action for relation, if it's DELETE > ROWS */ > + Assert(relation->rd_rel->reloncommit == > RELONCOMMIT_PRESERVE_ROWS || > + relation->rd_rel->reloncommit == > RELONCOMMIT_DELETE_ROWS); > + > + if (relation->rd_rel->reloncommit == RELONCOMMIT_DELETE_ROWS) > + register_on_commit_action(relation->rd_id, > ONCOMMIT_DELETE_ROWS); > > How about moving the comment to just before the if statement? > > 9. > +void > +InvalidateGlobalTempRelation(Oid relid) > +{ > + MemoryContext oldcontext; > + HASH_SEQ_STATUS status; > + GtrUsageEntry *entry; > + > + /* Quick exit if we haven't used any global temporary relations */ > + if (gtr_local_usage == NULL) > + return; > > Restrict status and entry to the else statement. > > 10. > +void > +GlobalTempRelationDropped(Oid relid) > > Not a fan of this name, but no better idea yet. > > 11. > +void > +PreCommit_GlobalTempRelation(void) > +{ > + HASH_SEQ_STATUS status; > + GtrStorageEntry *storage_entry; > + Relation rel; > + > > Restrict rel to the while loop. > > 12. > +List * > +AllGlobalTempRelationsInUse(Oid dbid) > > How about GetGlobalTempReplationsInUse()? > > 13. > + ereport(ERROR, > + > (errcode(ERRCODE_INVALID_TABLE_DEFINITION), > + errmsg("cannot create > relations in temporary schemas of other sessions"))); > > For consistency with the above, I suggest changing the error message to: > > cannot create global temporary relation in temporary schema of other > sessions > > 14. > + * If they're global temp relations, reassign rel2's storage to rel1. > > We use "global temporary relations" elsewhere – should we keep the same > terminology here? > > 15. > + errmsg(RELATION_IS_GLOBAL_TEMP(relation) > + ? "cannot create a local temporary relation as > partition of global temporary relation \"%s\"" > + : "cannot create a temporary relation as > partition of permanent relation \"%s\"", > > Should we use the following error message: > > cannot create a local temporary relation as partition of permanent > relation \"%s\" > > 16. > + : relpersistence == RELPERSISTENCE_GLOBAL_TEMP > + ? "cannot create a global temporary relation as > partition of local temporary relation \"%s\"" > : "cannot create a permanent relation as > partition of temporary relation \"%s\"", > > Same as above: > > cannot create a permanent relation as partition of local temporary > relation \"%s\" > > 17. > + if (RELATION_IS_GLOBAL_TEMP(OldHeap) && > + IsOtherUsingGlobalTempRelation(RelationGetRelid(OldHeap))) > + ereport(ERROR, > + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("cannot rewrite global temporary table \"%s\" > because it is being used in another session", > + RelationGetRelationName(OldHeap))); > > > 18. > + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("cannot add or alter constraints of > global temporary table \"%s\" because it is being used in another session", > + get_rel_name(tab->relid))); > > 19. > if (pkrel->rd_rel->relpersistence != > RELPERSISTENCE_TEMP) > ereport(ERROR, > > (errcode(ERRCODE_INVALID_TABLE_DEFINITION), > - errmsg("constraints on > temporary tables may reference only temporary tables"))); > + errmsg("constraints on local > temporary tables may reference only local temporary tables"))); > if (!pkrel->rd_islocaltemp || !rel->rd_islocaltemp) > ereport(ERROR, > > (errcode(ERRCODE_INVALID_TABLE_DEFINITION), > - errmsg("constraints on > temporary tables must involve temporary tables of this session"))); > + errmsg("constraints on local > temporary tables must involve local temporary tables of this session"))); > + break; > + case RELPERSISTENCE_GLOBAL_TEMP: > + if (!RELATION_IS_GLOBAL_TEMP(pkrel)) > + ereport(ERROR, > + > (errcode(ERRCODE_INVALID_TABLE_DEFINITION), > + errmsg("constraints on global > temporary tables may reference only global temporary tables"))); > > s/global temporary table/global temporary relation/g > s/local temporary table/local temporary relation/g > > 20. > attachrel->rd_rel->relpersistence == RELPERSISTENCE_TEMP) > ereport(ERROR, > (errcode(ERRCODE_WRONG_OBJECT_TYPE), > - errmsg("cannot attach a temporary relation as > partition of permanent relation \"%s\"", > + errmsg(RELATION_IS_GLOBAL_TEMP(rel) > + ? "cannot attach a local > temporary relation as partition of global temporary relation \"%s\"" > + : "cannot attach a temporary > relation as partition of permanent relation \"%s\"", > RelationGetRelationName(rel)))); > > Same as 15: > > cannot attach a local temporary relation as partition of permanent > relation \"%s\" > > 21. > attachrel->rd_rel->relpersistence != RELPERSISTENCE_TEMP) > ereport(ERROR, > (errcode(ERRCODE_WRONG_OBJECT_TYPE), > - errmsg("cannot attach a permanent relation as > partition of temporary relation \"%s\"", > + errmsg(RELATION_IS_GLOBAL_TEMP(attachrel) > + ? "cannot attach a global > temporary relation as partition of local temporary relation \"%s\"" > + : "cannot attach a permanent > relation as partition of temporary relation \"%s\"", > RelationGetRelationName(rel)))); > > Same as 16: > > cannot attach a permanent relation as partition of local temporary > relation \"%s\ > > 22. > @@ -22776,7 +22828,9 @@ createPartitionTable(List **wqueue, RangeVar > *newPartName, > newPartName->relpersistence == RELPERSISTENCE_TEMP) > ereport(ERROR, > errcode(ERRCODE_WRONG_OBJECT_TYPE), > - errmsg("cannot create a temporary relation as > partition of permanent relation \"%s\"", > + errmsg(parent_relform->relpersistence == > RELPERSISTENCE_GLOBAL_TEMP > + ? "cannot create a local temporary > relation as partition of global temporary relation \"%s\"" > + : "cannot create a temporary > relation as partition of permanent relation \"%s\"", > > RelationGetRelationName(parent_rel))); > > /* Permanent rels cannot be partitions belonging to a temporary parent. > */ > > Suggestion: > > cannot create a local temporary relation as partition of permanent > relation \"%s\" > > 23. > @@ -22784,7 +22838,9 @@ createPartitionTable(List **wqueue, RangeVar > *newPartName, > parent_relform->relpersistence == RELPERSISTENCE_TEMP) > ereport(ERROR, > errcode(ERRCODE_WRONG_OBJECT_TYPE), > - errmsg("cannot create a permanent relation as > partition of temporary relation \"%s\"", > + errmsg(newPartName->relpersistence == > RELPERSISTENCE_GLOBAL_TEMP > + ? "cannot create a global temporary > relation as partition of local temporary relation \"%s\"" > + : "cannot create a permanent > relation as partition of temporary relation \"%s\"", > > RelationGetRelationName(parent_rel))); > > Suggest: > > cannot create a permanent relation as partition of local temporary > relation \"%s\" > > While testing the v5 patch, I noticed that the index of a table with ON COMMIT DELETE ROWS has the PRESERVE ROWS flag. See the example below. postgres=# CREATE EXTENSION pageinspect; CREATE EXTENSION postgres=# CREATE GLOBAL TEMPORARY TABLE gtt_delete (id int primary key, info text) ON COMMIT DELETE ROWS; CREATE TABLE postgres=# CREATE GLOBAL TEMPORARY TABLE gtt_preserve (id int primary key, info text); CREATE TABLE postgres=# SELECT relname, reloncommit FROM pg_class WHERE relname ~ '^gtt_.*'; relname | reloncommit -------------------+------------- gtt_delete | d gtt_delete_pkey | p <-- See here. gtt_preserve | p gtt_preserve_pkey | p (4 rows) postgres=# BEGIN; BEGIN postgres=*# INSERT INTO gtt_delete SELECT id FROM generate_series(1, 10000) id; INSERT 0 10000 postgres=*# SELECT * FROM bt_metap('gtt_delete_pkey'); magic | version | root | level | fastroot | fastlevel | last_cleanup_num_delpages | last_cleanup_num_tuples | allequalimage --------+---------+------+-------+----------+-----------+---------------------------+-------------------------+--------------- 340322 | 4 | 3 | 1 | 3 | 1 | 0 | -1 | t (1 row) postgres=*# SELECT * FROM bt_page_items('gtt_delete_pkey', 1) limit 2; itemoffset | ctid | itemlen | nulls | vars | data | dead | htid | tids ------------+-------+---------+-------+------+-------------------------+------+-------+------ 1 | (1,1) | 16 | f | f | 6f 01 00 00 00 00 00 00 | | | 2 | (0,1) | 16 | f | f | 01 00 00 00 00 00 00 00 | f | (0,1) | (2 rows) postgres=*# END; COMMIT postgres=# SELECT * FROM bt_metap('gtt_delete_pkey'); magic | version | root | level | fastroot | fastlevel | last_cleanup_num_delpages | last_cleanup_num_tuples | allequalimage --------+---------+------+-------+----------+-----------+---------------------------+-------------------------+--------------- 340322 | 4 | 0 | 0 | 0 | 0 | 0 | -1 | t (1 row) postgres=# SELECT * FROM bt_page_items('gtt_delete_pkey', 1) limit 2; ERROR: block number 1 is out of range postgres=# BEGIN; BEGIN postgres=*# INSERT INTO gtt_preserve SELECT id FROM generate_series(1, 10000) id; INSERT 0 10000 postgres=*# SELECT * FROM bt_metap('gtt_preserve_pkey'); magic | version | root | level | fastroot | fastlevel | last_cleanup_num_delpages | last_cleanup_num_tuples | allequalimage --------+---------+------+-------+----------+-----------+---------------------------+-------------------------+--------------- 340322 | 4 | 3 | 1 | 3 | 1 | 0 | -1 | t (1 row) postgres=*# SELECT * FROM bt_page_items('gtt_preserve_pkey', 1) limit 2; itemoffset | ctid | itemlen | nulls | vars | data | dead | htid | tids ------------+-------+---------+-------+------+-------------------------+------+-------+------ 1 | (1,1) | 16 | f | f | 6f 01 00 00 00 00 00 00 | | | 2 | (0,1) | 16 | f | f | 01 00 00 00 00 00 00 00 | f | (0,1) | (2 rows) postgres=*# COMMIT; COMMIT postgres=# SELECT * FROM bt_metap('gtt_preserve_pkey'); magic | version | root | level | fastroot | fastlevel | last_cleanup_num_delpages | last_cleanup_num_tuples | allequalimage --------+---------+------+-------+----------+-----------+---------------------------+-------------------------+--------------- 340322 | 4 | 3 | 1 | 3 | 1 | 0 | -1 | t (1 row) postgres=# SELECT * FROM bt_page_items('gtt_preserve_pkey', 1) limit 2; itemoffset | ctid | itemlen | nulls | vars | data | dead | htid | tids ------------+-------+---------+-------+------+-------------------------+------+-------+------ 1 | (1,1) | 16 | f | f | 6f 01 00 00 00 00 00 00 | | | 2 | (0,1) | 16 | f | f | 01 00 00 00 00 00 00 00 | f | (0,1) | (2 rows) Is this expected behavior? Since the index data for gtt_delete is cleared upon transaction commit, I would expect its reloncommit flag to be 'd'. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
