Re: VACUUM FULL, CLUSTER, and REPACK block on other sessions' temp tables - Mailing list pgsql-hackers

From Chao Li
Subject Re: VACUUM FULL, CLUSTER, and REPACK block on other sessions' temp tables
Date
Msg-id 5C25190B-EE60-4634-A1DE-E7B5FBFF8AE4@gmail.com
Whole thread Raw
In response to Re: VACUUM FULL, CLUSTER, and REPACK block on other sessions' temp tables  (Jim Jones <jim.jones@uni-muenster.de>)
Responses Re: VACUUM FULL, CLUSTER, and REPACK block on other sessions' temp tables
List pgsql-hackers

> On Mar 26, 2026, at 07:00, Jim Jones <jim.jones@uni-muenster.de> wrote:
>
> Hi
>
> On 25/03/2026 21:38, Zsolt Parragi wrote:
>> Shouldn't the patch also include a tap test to verify that the change
>> works / fails without it?
>
> Definitely. I just didn't want to invest much time on tests before
> getting feedback on the issue itself.
>
>> + /* Skip temp relations belonging to other sessions */
>> + {
>> + Oid nsp = get_rel_namespace(index->indrelid);
>> +
>> + if (!isTempOrTempToastNamespace(nsp) && isAnyTempNamespace(nsp))
>> + {
>>
>> Doesn't this result in several repeated syscache lookups?
>>
>> There's already a SearchSysCacheExsists1 directly above this, then a
>> get_rel_namespace, then an isAnyTempNamespace. While this probably
>> isn't performance critical, this should be doable with a single
>> SearchSysCache1(RELOID...) and then a few conditions, similarly to the
>> else branch below this?
>
> You're right. Although it is not performance critical we can solve it
> with a single SearchSysCache1.
>
> PFA v3 with the improved fix (0001) and tests (0002).
>
> Thanks for the review!
>
> Best,
Jim<v3-0001-Skip-other-sessions-temp-tables-in-REPACK-CLUSTER.patch><v3-0002-Test-VACUUM-FULL-CLUSTER-and-REPACK-with-locked-t.patch>

I don't think such a TAP test is necessary.

One reason is that, if we look at the check right above the new one:
```
        /*
         * We include partitioned tables here; depending on which operation is
         * to be performed, caller will decide whether to process or ignore
         * them.
         */
        if (classForm->relkind != RELKIND_RELATION &&
            classForm->relkind != RELKIND_MATVIEW &&
            classForm->relkind != RELKIND_PARTITIONED_TABLE)
            continue;
```

I don't see a test specifically for that check either. So I don't think we need a test for every individual path.

Second, based on [1] and [2], I got the impression that adding new tests is not always welcome considering overall test
runtime.Anyway, maybe I’m wrong, let the committers judge that. 

[1] https://postgr.es/m/mtkrkkcn2tlhytumitpch5ubxiprv2jzvprf5r5m3mjeczvq4q@p6wkzbfxuyv2 <https://postgr.es/m/>
[2] https://postgr.es/m/1449781.1773948276@sss.pgh.pa.us

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/







pgsql-hackers by date:

Previous
From: Yugo Nagata
Date:
Subject: Re: Track skipped tables during autovacuum and autoanalyze
Next
From: Masahiko Sawada
Date:
Subject: Re: Add uuid_to_base32hex() and base32hex_to_uuid() built-in functions