| From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
|---|---|
| To: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
| Cc: | Daniel Gustafsson <daniel(at)yesql(dot)se>, Nikolay Samokhvalov <nik(at)postgres(dot)ai>, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>, pgsql-bugs(at)lists(dot)postgresql(dot)org |
| Subject: | Re: autovacuum: automatically propagate updated parameters |
| Date: | 2026-09-25 21:41:39 |
| Message-ID: | CALj2ACUBkuGeS8m+o_5OzrTOtf=NhekVJ8PLS5yNTyHYnH0jpg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-bugs |
Hi,
On Thu, Sep 24, 2026 at 4:47 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > > How about doing the check inside the existing wait loop, only when the process is an autovacuum worker, along the lines of the attached WIP? This is simple, the pattern already exists elsewhere in the code, and it looks safe. I also believe the wait loop is not in a performance-critical hot path, and this check is not costly. I checked that it still fixes the reported issues.
>
> > It's not great to sprinkle in worker specific code in the generic parallel
> > handling. My original thinking was to reject the idea, but the vacuum costing
> > is already used for non-vacuum purposes (and there have been discussions to
> > rename and generalize it) so with that in mind I am less concerned for this
> > particular case.
>
> Same here. After thinking on the Bharath's patch, I now think it might
> make sense. I guess it's likely that when autovacuum wants to wait for
> other workers to finish in other cases like parallel heap vacuum, we
> would want to use WaitForParallelWorkersToFinish() and want it to
> update the cost-based delay params during the wait.
>
> > > What I am less sure about is the fix for the second issue in the attached WIP patch, which needs any leader waiting for its workers to be woken up after the cost limit is rebalanced. I haven't found a better one yet.
>
> > Not sure I see a better solution either, and we are running short of time
> > before 19 RC1.
>
> While it's a good idea to wake the leader up instead of polling, I'm a
> bit concerned that we call SetLatch() on all autovacuum workers
> participating in cost balancing, whereas we need it only in a narrow
> situation: when the worker is participating in cost balancing, using
> parallel vacuum, and waiting for its parallel workers to finish.
> Calling SetLatch() on a process that is not waiting doesn't send a
> signal, butit still leaves the latch set, causing a spurious wakeup
> the next time the process waits for something else.
>
> I think a condition variable fits better here.
Thanks all for the comments.
I understand the cost of setting the latch and waking up every leader
participating in rebalancing could be higher. But we only need to wake
the leaders that are in the wait-for-parallel-workers-to-finish loop.
Those are the ones that need the rebalanced cost limit, so they can
propagate it down to their workers still doing index vacuuming. That
way the workers can adjust to the system needs. The user may have
changed the cost limits to slow things down or speed them up, and
either way it is not good for the workers to miss them.
Now, on the cost of setting the latch. It depends on three things.
First, how many leaders can exist, which depends on
autovacuum_max_workers and autovacuum_worker_slots. Second, how often
the number of leaders sharing the cost limit changes, that is, when
the recount comes out different from the previous count. This happens
when a leader starts or exits, or when the user sets or removes a
per-table cost limit, which moves a leader in or out of the sharing
set the next time that table is vacuumed. Third, how often a leader is
waiting in the loop at that moment.
Having all three happen together looks fairly rare in practice IMHO.
And even if it happens often, the cost is small, since set-latch sends
a signal only to a process that is in a latch wait at that moment. For
a leader not in the loop, it only marks the latch as set and returns,
and that leader's next latch wait just returns once early and
re-checks.
If we still want to set the latch only for the leaders in the loop,
there are a couple of ways, and I don't prefer either. We could check
the leader's wait event info to see whether it is in the loop, or the
leader could set a flag in its WorkerInfo when it enters the loop and
clear it when it leaves. Both are prone to race conditions and add
more code than they save. I would leave it as a follow-up if anyone
still wants to pursue it.
That said, please find the attached v3 patch set. 0001 has only the
fixes, and 0002 has the tests posted upthread along with the injection
points. I think 0001 is ready to go in, unless there are further
comments. I'm fine leaving 0002 here for the record, since I haven't
reviewed it in depth and I don't think it has had a close review yet,
and it needs more dev cycles. Anyone with cycles can pick it up for
HEAD. Given the time left, my suggestion would be to consider 0001 for
PG19 and HEAD.
Thoughts?
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-Refresh-autovacuum-costs-while-waiting-for-parall.patch | application/x-patch | 7.3 KB |
| v3-0002-Add-tests-for-autovacuum-cost-parameter-refresh-w.patch | application/x-patch | 13.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Manu | 2026-09-25 22:03:24 | Re: autovacuum: automatically propagate updated parameters |
| Previous Message | Corey Huinker | 2026-09-25 19:45:10 | Re: BUG #19715: pg_restore_attribute_stats() rejects range statistics for a domain over int4multirange |