| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
| Cc: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Fix stale comment in parallel_vacuum_main(). |
| Date: | 2026-07-29 06:00:19 |
| Message-ID: | C696D03D-5D85-4B8B-B171-342821C98A6A@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Jul 29, 2026, at 02:24, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Tue, Jul 28, 2026 at 11:00 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>
>> On Mon, Jul 27, 2026 at 6:00 PM Bharath Rupireddy
>> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>>
>>> Hi,
>>>
>>> While working on the parallel heap vacuum feature I noticed a stale
>>> comment in parallel_vacuum_main(). It says a parallel vacuum worker
>>> must have only PROC_IN_VACUUM set because parallel index vacuum is not
>>> supported for autovacuum. That is no longer true after commit
>>> 1ff3180ca01, which enabled autovacuum to use parallel vacuum workers.
>>
>> Good catch!
>>
>>> IMHO, the assertion itself is still worth keeping. It ensures the
>>> parallel worker carries PROC_IN_VACUUM, so that other backends ignore
>>> its xmin when computing the removable-tuple horizon, which is what we
>>> want for a worker that is part of a vacuum. The leader, whether it is
>>> a backend running a VACUUM command or an autovacuum worker, sets
>>> PROC_IN_VACUUM when it starts vacuuming the table, and the worker
>>> inherits it when it imports the leader's snapshot (see
>>> ProcArrayInstallRestoredXmin). PROC_IS_AUTOVACUUM never reaches the
>>> worker, because parallel workers are launched as regular background
>>> workers rather than autovacuum workers, so the flag is never set on
>>> them, and importing the leader's snapshot doesn't carry it over
>>> either. So the worker ends up with exactly PROC_IN_VACUUM even under
>>> parallel autovacuum, and the assertion holds.
>>>
>>> The attached patch rewords the comment to explain this. Since PG19 is
>>> still in beta, I would like to backpatch this to 19 where 1ff3180ca01
>>> was introduced.
>>>
>>> Thoughts?
>>
>> I agree with your analysis. And the proposed comment looks clear to
>> me. Will push the patch barring any objections.
>>
>
> After reviewing the new comment closely, I think it's better to
> mention the PROC_VACUUM_FOR_WRAPAROUND flag too as this is also used
> for autovacuums. I've attached the updated patch, please review it.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
> <v2-0001-Fix-stale-comment-in-parallel_vacuum_main.patch>
I checked the code. An autovacuum leader already has PROC_IS_AUTOVACUUM. Then vacuum_rel() adds PROC_IN_VACUUM and may also add PROC_VACUUM_FOR_WRAPAROUND when params.is_wraparound is true. The parallel worker starts without any of these flags, and snapshot restoration copies only the PROC_XMIN_FLAGS bits. Since the vacuum leader has PROC_IN_VACUUM but not PROC_IN_SAFE_IC, the worker ends up with only PROC_IN_VACUUM.
So, v2 looks good to me.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Richard Guo | 2026-07-29 06:21:18 | Re: remove_useless_joins vs. bug #19560 |
| Previous Message | Ayush Tiwari | 2026-07-29 05:41:24 | Re: Prove a NOT IN's left-hand expressions non-nullable from quals |