| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Jim Jones <jim(dot)jones(at)uni-muenster(dot)de> |
| Cc: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Antonin Houska <ah(at)cybertec(dot)at> |
| Subject: | Re: VACUUM FULL, CLUSTER, and REPACK block on other sessions' temp tables |
| Date: | 2026-03-26 02:52:45 |
| Message-ID: | 5C25190B-EE60-4634-A1DE-E7B5FBFF8AE4@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Mar 26, 2026, at 07:00, Jim Jones <jim(dot)jones(at)uni-muenster(dot)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/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Masahiko Sawada | 2026-03-26 03:13:55 | Re: Add uuid_to_base32hex() and base32hex_to_uuid() built-in functions |
| Previous Message | Yugo Nagata | 2026-03-26 02:25:29 | Re: Track skipped tables during autovacuum and autoanalyze |