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\" -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
