| From: | Nathan Bossart <nathandbossart(at)gmail(dot)com> |
|---|---|
| To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
| Cc: | Sami Imseih <samimseih(at)gmail(dot)com>, Robert Treat <rob(at)xzilla(dot)net>, satyanarlapuram(at)gmail(dot)com, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: Add pg_stat_autovacuum_priority |
| Date: | 2026-03-31 18:34:46 |
| Message-ID: | acwTxpz3Toxt0ty8@nathan |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Tue, Mar 31, 2026 at 11:09:43AM -0700, Bharath Rupireddy wrote:
> 1/ + while ((classTup = heap_getnext(relScan, ForwardScanDirection)) != NULL)
>
> Missing check_for_interrupts call while scanning the pg_class system catalog.
From a glance I don't see one in the scanning code in do_autovacuum(),
either. I'm not sure we need to be worried about this.
> + avopts = extract_autovac_opts(classTup, pg_class_desc);
> +
> + compute_autovac_score(classTup, pg_class_desc,
> + effective_multixact_freeze_max_age, avopts,
> + true, &dovacuum, &doanalyze,
> + &wraparound, &scores);
> +
> + if (avopts)
> + pfree(avopts);
>
> When a database has a large number of tables (which is quite common in
> production scenarios), I expect the costs of palloc and pfree being
> used for fetching autovacuum relopts would make this function slower.
> Can we invent a new function or pass a caller-allocated AutoVacOpts
> memory to just copy the relopts and use that in this tight loop when
> scanning for all the relations?
Before making this code more complicated, I think we ought to demonstrate
there's an actual problem or slowness. I am quite dubious we need to do
anything here.
> + values[8] = Float8GetDatum(scores.vac_ins);
> + values[9] = Float8GetDatum(scores.anl);
>
> Nit: It's a matter of taste. How about using something like below
> instead of hardcoded column numbers? I expect this view to grow in the
> future, so it helps to keep things simple.
>
> values[i++] = Float8GetDatum(scores.anl);
> Assert(i == NUM_AV_SCORE_COLS);
I don't think either way is substantially better.
> + The <link linkend="monitoring-pg-stat-autovacuum-priority-view">
> + <structname>pg_stat_autovacuum_priority</structname></link> view can be
> + used to inspect each table's autovacuum need and priority score.
>
> How about adding "as of the moment" to convey that it doesn't report
> what currently running autovacuum or pending autovacuum would
> consider?
I don't think "as of the moment" adds any clarity about that. If we did
want to add something along those lines, I'd add a separate sentence that
says that it doesn't report the values of current autovacuum workers and is
freshly calculated by the current backend (or something like that).
--
nathan
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Andres Freund | 2026-03-31 18:49:19 | Re: Don't synchronously wait for already-in-progress IO in read stream |
| Previous Message | Tomas Vondra | 2026-03-31 18:33:15 | Re: EXPLAIN: showing ReadStream / prefetch stats |