Thread
-
Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-03-24T06:11:33Z
Hi, I would like to propose adding new fields to pg_stat_all_tables to track tables skipped during autovacuum and autoanalyze. Currently, when autovacuum or autoanalyze is skipped because a lock cannot be acquired, this information is only emitted in log messages. However, it would be more useful if users could access this information via a system view, alongside related fields such as last_autovacuum, on a per-table basis. The attached patch add the following fields to pg_stat_all_tables: - last_skipped_autovacuum - last_skipped_autoanalyze - skipped_autovacuum_count - skipped_autoanalyze_count Are there any concerns about exposing this in pg_stat_all_tables, or suggestions for a better approach? Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Sami Imseih <samimseih@gmail.com> — 2026-03-24T14:58:48Z
Hi, Thanks for the patch! > The attached patch add the following fields to pg_stat_all_tables: > - last_skipped_autovacuum > - last_skipped_autoanalyze > - skipped_autovacuum_count > - skipped_autoanalyze_count > > Are there any concerns about exposing this in pg_stat_all_tables, or suggestions > for a better approach? I am not sure about the timestamp columns. I am not saying they will not be useful, but I think it will be better to just start with counters for this. The way the views get used, a dashboard built for tracking the deltas of the counters can easily spot when there is a spike of skipped autovacuum/autoanalyze count. Also, for tables that are being autovacuumed and skipped quickly, the timestamps will just be overwritten. So, I am +1 on the counters, -1 on the timestamps. Out of scope for this patch, but I also wonder if we should add another counter, autovacuum_started_count. If there are other types of failure scenarios such as corrupt indexes, checksum failures, etc. which terminate the autovacuum in flight, we would be able to catch this by looking at the number of autovacuums started vs completed. The skipped counters in this patch and a started counter would capture different stages of the autovacuum lifecycle; skipped means "never started" (lock contention), while a started-minus-completed delta means "started but failed." Both are useful signals, but for different reasons. In terms of the patch: 1/ + if (AmAutoVacuumWorkerProcess()) + pgstat_report_skipped_vacuum(relid); Any reason why this should not also include manual vacuum/analyze? If someone has a vacuum/analyze script that uses SKIP_LOCKED, and the operation gets skipped, this should be included in the counter. this can be done with separate counter fields for autovacuum/autoanalyze and vacuum/analyze 2/ + pg_stat_get_skipped_autovacuum_count(C.oid) AS skipped_autovacuum_count, How about a name like "autovacuum_lock_skip_count"? -- Sami Imseih Amazon Web Services (AWS)
-
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-03-24T16:28:47Z
On Tue, 24 Mar 2026 09:58:48 -0500 Sami Imseih <samimseih@gmail.com> wrote: Thank you for your comments! > > The attached patch add the following fields to pg_stat_all_tables: > > - last_skipped_autovacuum > > - last_skipped_autoanalyze > > - skipped_autovacuum_count > > - skipped_autoanalyze_count > > > > Are there any concerns about exposing this in pg_stat_all_tables, or suggestions > > for a better approach? > > I am not sure about the timestamp columns. I am not saying they will > not be useful, > but I think it will be better to just start with counters for this. > The way the views get > used, a dashboard built for tracking the deltas of the counters can easily spot > when there is a spike of skipped autovacuum/autoanalyze count. > Also, for tables that are being autovacuumed and skipped quickly, > the timestamps will just be overwritten. > > So, I am +1 on the counters, -1 on the timestamps. Although the timestamps are overwritten on each skipped autovacuum or autoanalyze, they still indicate when the last attempt was made. This can help users confirm that autovacuum is actively attempting to run, and that the issue is due to repeated skips rather than inactivity. While counters can indicate overall activity, they do not reveal when the last skip occurred. With timestamps, users can immediately see the most recent attempt, even without a separate dashboard or historical tracking. Therefore, counters are useful for monitoring overall activity, but timestamps give additional, complementary information, so it seems worthwhile to include them too. > Out of scope for this patch, but I also wonder if we should add another counter, > autovacuum_started_count. If there are other types of failure scenarios such as > corrupt indexes, checksum failures, etc. which terminate the > autovacuum in flight, > we would be able to catch this by looking at the number of autovacuums > started vs completed. The skipped counters in this patch and a started > counter would capture different stages of the autovacuum lifecycle; > skipped means > "never started" (lock contention), while a started-minus-completed delta means > "started but failed." Both are useful signals, but for different reasons. That makes sense. I was considering adding a counter to track "canceled" autovacuum or autoanalyze, but tracking the number of started or attempted autovacuums might provide a more general information than counting only the failed ones. > In terms of the patch: > > 1/ > > + if (AmAutoVacuumWorkerProcess()) > + pgstat_report_skipped_vacuum(relid); > > Any reason why this should not also include manual vacuum/analyze? > If someone has a vacuum/analyze script that uses SKIP_LOCKED, and > the operation gets skipped, this should be included in the counter. > this can be done with separate counter fields for autovacuum/autoanalyze and > vacuum/analyze For manual vacuum/analyze, an explicit WARNING is output when the operation is skipped, so I initially thought that reporting it in the stats view was not necessary. However, I now agree that it should be included. > 2/ > > + pg_stat_get_skipped_autovacuum_count(C.oid) AS > skipped_autovacuum_count, > > How about a name like "autovacuum_lock_skip_count"? I'm not sure this is a good fit, since it may be better to keep the naming consistent with other fields whose names end with "...vacuum_count" or "...analyze_count". Happy to hear other opinions. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Michael Paquier <michael@paquier.xyz> — 2026-03-25T02:07:08Z
On Wed, Mar 25, 2026 at 01:28:47AM +0900, Yugo Nagata wrote: > Although the timestamps are overwritten on each skipped autovacuum or > autoanalyze, they still indicate when the last attempt was made. This > can help users confirm that autovacuum is actively attempting to run, > and that the issue is due to repeated skips rather than inactivity. > > While counters can indicate overall activity, they do not reveal when > the last skip occurred. With timestamps, users can immediately see the > most recent attempt, even without a separate dashboard or historical > tracking. > > Therefore, counters are useful for monitoring overall activity, but > timestamps give additional, complementary information, so it seems > worthwhile to include them too. Hmm.. I can buy this argument for the timestamps, especially for database with many relations of various sizes that could take a various amount of time to process. The timestamps could offer hints about the time it takes between the skips, even if snapshots of the stats data are not taken at a very aggressive frequency. This is v20 material at this stage, of course.. -- Michael
-
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-03-25T04:24:51Z
On Wed, 25 Mar 2026 11:07:08 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Wed, Mar 25, 2026 at 01:28:47AM +0900, Yugo Nagata wrote: > > Although the timestamps are overwritten on each skipped autovacuum or > > autoanalyze, they still indicate when the last attempt was made. This > > can help users confirm that autovacuum is actively attempting to run, > > and that the issue is due to repeated skips rather than inactivity. > > > > While counters can indicate overall activity, they do not reveal when > > the last skip occurred. With timestamps, users can immediately see the > > most recent attempt, even without a separate dashboard or historical > > tracking. > > > > Therefore, counters are useful for monitoring overall activity, but > > timestamps give additional, complementary information, so it seems > > worthwhile to include them too. > > Hmm.. I can buy this argument for the timestamps, especially for > database with many relations of various sizes that could take a > various amount of time to process. The timestamps could offer hints > about the time it takes between the skips, even if snapshots of the > stats data are not taken at a very aggressive frequency. > > This is v20 material at this stage, of course.. Thank you for your comments. Yes, I understand that it's too late for this to be included in v19. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Sami Imseih <samimseih@gmail.com> — 2026-03-25T17:12:35Z
> > On Wed, Mar 25, 2026 at 01:28:47AM +0900, Yugo Nagata wrote: > > > Although the timestamps are overwritten on each skipped autovacuum or > > > autoanalyze, they still indicate when the last attempt was made. This > > > can help users confirm that autovacuum is actively attempting to run, > > > and that the issue is due to repeated skips rather than inactivity. > > > > > > While counters can indicate overall activity, they do not reveal when > > > the last skip occurred. With timestamps, users can immediately see the > > > most recent attempt, even without a separate dashboard or historical > > > tracking. > > > > > > Therefore, counters are useful for monitoring overall activity, but > > > timestamps give additional, complementary information, so it seems > > > worthwhile to include them too. > > > > Hmm.. I can buy this argument for the timestamps, especially for > > database with many relations of various sizes that could take a > > various amount of time to process. The timestamps could offer hints > > about the time it takes between the skips, even if snapshots of the > > stats data are not taken at a very aggressive frequency. I'm fine with adding timestamps, as there seem to be convincing reasons to add them. My other concern is bloat of the pg_stat_all_tables view. This patch adds 4 columns, or 8 if we also include manual vacuum and analyze (which I think we should). Given that, should we also start thinking about splitting the vacuum activity related columns into a dedicated view and out of pg_stat_all_tables for v20? -- Sami Imseih Amazon Web Services (AWS)
-
Re: Track skipped tables during autovacuum and autoanalyze
Michael Paquier <michael@paquier.xyz> — 2026-03-25T23:26:26Z
On Wed, Mar 25, 2026 at 12:12:35PM -0500, Sami Imseih wrote: > I'm fine with adding timestamps, as there seem to be convincing > reasons to add them. > My other concern is bloat of the pg_stat_all_tables view. This patch > adds 4 columns, or > 8 if we also include manual vacuum and analyze (which I think we should). > > Given that, should we also start thinking about splitting the vacuum > activity related > columns into a dedicated view and out of pg_stat_all_tables for v20? PgStat_StatTabEntry is shared between indexes and tables. A bunch of its fields apply only to tables, not indexes (aka all the vacuum and analyze ones). Few fields apply only to indexes, not tables. Not that many are shared between both. I would advocate for a clean split between indexes and tables, as a start, with a new variable-sized stats kind dedicated to indexes. -- Michael
-
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-03-26T01:18:39Z
On Thu, 26 Mar 2026 08:26:26 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Wed, Mar 25, 2026 at 12:12:35PM -0500, Sami Imseih wrote: > > I'm fine with adding timestamps, as there seem to be convincing > > reasons to add them. > > My other concern is bloat of the pg_stat_all_tables view. This patch > > adds 4 columns, or > > 8 if we also include manual vacuum and analyze (which I think we should). > > > > Given that, should we also start thinking about splitting the vacuum > > activity related > > columns into a dedicated view and out of pg_stat_all_tables for v20? > > PgStat_StatTabEntry is shared between indexes and tables. A bunch of > its fields apply only to tables, not indexes (aka all the vacuum and > analyze ones). Few fields apply only to indexes, not tables. Not > that many are shared between both. I would advocate for a clean split > between indexes and tables, as a start, with a new variable-sized > stats kind dedicated to indexes. I see that the following fields are used in pg_stat_all_indexes: - numscans - lastscan - tuples_returned - tuples_fetched - stat_reset_time but they are also shared with pg_stat_all_tables. Are you suggesting splitting these shared fields from those that are specific to tables? I'm not sure this would significantly reduce the size of PgStat_StatTabEntry. Could you elaborate on the expected benefits? Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Michael Paquier <michael@paquier.xyz> — 2026-03-26T01:31:19Z
On Thu, Mar 26, 2026 at 10:18:39AM +0900, Yugo Nagata wrote: > I'm not sure this would significantly reduce the size of > PgStat_StatTabEntry. Could you elaborate on the expected benefits? The point is that this reduces the shmem footprint for indexes (well, it's also benefitial for tables, just less), on top of being cleaner because the stats views would only need to store and query the fields they care about for each relkind. -- Michael
-
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-03-26T02:25:29Z
On Thu, 26 Mar 2026 10:31:19 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Thu, Mar 26, 2026 at 10:18:39AM +0900, Yugo Nagata wrote: > > I'm not sure this would significantly reduce the size of > > PgStat_StatTabEntry. Could you elaborate on the expected benefits? > > The point is that this reduces the shmem footprint for indexes (well, > it's also benefitial for tables, just less), on top of being cleaner > because the stats views would only need to store and query the fields > they care about for each relkind. Thank you for the clarification. I think I understand your point now. So, you mean introducing a separate stats kind for indexes, such as PGSTAT_KIND_INDEX? Regards, Yugo Nagata > -- > Michael -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-03-26T10:22:03Z
> On Tue, 24 Mar 2026 09:58:48 -0500 > Sami Imseih <samimseih@gmail.com> wrote: > > 1/ > > > > + if (AmAutoVacuumWorkerProcess()) > > + pgstat_report_skipped_vacuum(relid); > > > > Any reason why this should not also include manual vacuum/analyze? > > If someone has a vacuum/analyze script that uses SKIP_LOCKED, and > > the operation gets skipped, this should be included in the counter. > > this can be done with separate counter fields for autovacuum/autoanalyze and > > vacuum/analyze > > For manual vacuum/analyze, an explicit WARNING is output when the > operation is skipped, so I initially thought that reporting it in the > stats view was not necessary. However, I now agree that it should be > included. I've attached an updated patch to also report skipped manual vacuum/analyze. The pgstat reporting functions are unified into a single function, pgstat_report_skipped_vacuum_analyze(), which handles both auto/manual and vacuum/analyze cases. Also it is fixed to use InvalidOid for shared relations. To support manual vacuum/analyze, some hack were needed to obtain the relid before the lock attempt. When SKIP_LOCKED is specified, the relid is obtained using RangeVarGetRelid() with NoLock prior to locking. If ConditionalLockRelationOid() then fails, the skip is reported. To handle the possibility that the table is dropped between RangeVarGetRelid() and the lock attempt, SearchSysCacheExists1() is used after acquiring the lock. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Michael Paquier <michael@paquier.xyz> — 2026-03-26T23:07:29Z
On Thu, Mar 26, 2026 at 07:22:03PM +0900, Yugo Nagata wrote: > To handle the possibility that the table is dropped between > RangeVarGetRelid() and the lock attempt, SearchSysCacheExists1() is > used after acquiring the lock. (Noticed while skimming through my emails this morning..) +void +pgstat_report_skipped_vacuum_analyze(Oid relid, bool vacuum, bool analyze, + bool autovacuum) I'd recommend to replace this interface with three booleans with a set of three bitwise flags. That would be less error prone for the callers of this function, or we could finish by aggregating counters we don't want to. -- Michael
-
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-03-27T07:35:49Z
On Fri, 27 Mar 2026 08:07:29 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Thu, Mar 26, 2026 at 07:22:03PM +0900, Yugo Nagata wrote: > > To handle the possibility that the table is dropped between > > RangeVarGetRelid() and the lock attempt, SearchSysCacheExists1() is > > used after acquiring the lock. > > (Noticed while skimming through my emails this morning..) > > +void > +pgstat_report_skipped_vacuum_analyze(Oid relid, bool vacuum, bool analyze, > + bool autovacuum) > > I'd recommend to replace this interface with three booleans with a set > of three bitwise flags. That would be less error prone for the > callers of this function, or we could finish by aggregating counters > we don't want to. Thank you for the suggestion. I've attached a revised patch reflecting this change, and it also includes the documentation. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Sami Imseih <samimseih@gmail.com> — 2026-03-27T16:48:27Z
> I've attached a revised patch reflecting this change, and it also includes > the documentation. Thanks fo the update! I have some comments: 1/ +pgstat_report_skipped_vacuum_analyze(Oid relid, bits8 flags) using bit8 is fine here, but I would have just used int. For this case, it's just a matter of prefernace. 2/ +/* flags for pgstat_flush_backend() */ +#define PGSTAT_REPORT_SKIPPED_VACUUM (1 << 0) /* vacuum is skipped */ +#define PGSTAT_REPORT_SKIPPED_ANALYZE (1 << 1) /* analyze is skipped */ +#define PGSTAT_REPORT_SKIPPED_AUTOVAC (1 << 2) /* skipped during autovacuum/autoanalyze */ +#define PGSTAT_REPORT_SKIPPED_ANY (PGSTAT_REPORT_SKIPPED_VACUUM | PGSTAT_REPORT_SKIPPED_ANALYZE) can we just have 4 flags, SKIPPED_VACUUM, SKIPPED_ANALYZE, SKIPPED_AUTOVACUUM, SKIPPED_AUTOANALYZE, which can then remove the nested if/else and makes the mapping more obvious + if (flags & PGSTAT_REPORT_SKIPPED_AUTOVAC) + { + if (flags & PGSTAT_REPORT_SKIPPED_VACUUM) + { + tabentry->last_skipped_autovacuum_time = ts; + tabentry->skipped_autovacuum_count++; + } + if (flags & PGSTAT_REPORT_SKIPPED_ANALYZE) + { + tabentry->last_skipped_autoanalyze_time = ts; + tabentry->skipped_autoanalyze_count++; + } + } + else + { + if (flags & PGSTAT_REPORT_SKIPPED_VACUUM) + { + tabentry->last_skipped_vacuum_time = ts; + tabentry->skipped_vacuum_count++; + } + if (flags & PGSTAT_REPORT_SKIPPED_ANALYZE) + { + tabentry->last_skipped_analyze_time = ts; + tabentry->skipped_analyze_count++; + } + } 3/ For the sake of consistency, can we rename the fields from skipped_vacuum_count to vacuum_skipped_count, etc. ? to be similar to fields like vacuum_count 4/ field documentation could be a bit better to match existing phrasing For example, the timestamp fields: - Last time a manual vacuum on this table was attempted but skipped due to - lock unavailability (not counting <command>VACUUM FULL</command>) + The time of the last manual vacuum on this table that was skipped + due to lock unavailability (not counting <command>VACUUM FULL</command>) and the counter fields - Number of times vacuums on this table have been attempted but skipped + Number of times a manual vacuum on this table has been skipped 5/ Partitioned table asymmetry between vacuum_count and vacuum_skipped_count. vacuum_count never increments on a the parenttable, because the parent is never pocessed. On the other hand, if the manual VACUUM/ANALYZE is on the parent table, then we will skip all the children. So, we should still report the skip on the parent table, but we should add a Notes section in the docs perhaps to document this caveat? 6/ It would be nice to add a test for this, but this requires concurrency and I'm not sure it's woth it. Also, can you create a CF entry in https://commitfest.postgresql.org/59/, please. Thanks! -- Sami Imseih Amazon Web Services (AWS) -
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-03-28T07:18:02Z
On Fri, 27 Mar 2026 11:48:27 -0500 Sami Imseih <samimseih@gmail.com> wrote: > > I've attached a revised patch reflecting this change, and it also includes > > the documentation. > > Thanks fo the update! > > I have some comments: > > 1/ > +pgstat_report_skipped_vacuum_analyze(Oid relid, bits8 flags) > > using bit8 is fine here, but I would have just used int. For this > case, it's just a matter of prefernace. That makes sense, since using int for flags seems common in other places in the code. I'm not sure how much it affects performance, though. > 2/ > +/* flags for pgstat_flush_backend() */ > +#define PGSTAT_REPORT_SKIPPED_VACUUM (1 << 0) /* vacuum is skipped */ > +#define PGSTAT_REPORT_SKIPPED_ANALYZE (1 << 1) /* analyze is skipped */ > +#define PGSTAT_REPORT_SKIPPED_AUTOVAC (1 << 2) /* skipped > during autovacuum/autoanalyze */ > +#define PGSTAT_REPORT_SKIPPED_ANY (PGSTAT_REPORT_SKIPPED_VACUUM | > PGSTAT_REPORT_SKIPPED_ANALYZE) > > can we just have 4 flags, SKIPPED_VACUUM, SKIPPED_ANALYZE, > SKIPPED_AUTOVACUUM, SKIPPED_AUTOANALYZE, > which can then remove the nested if/else and makes the mapping more obvious I am fine with that. In that case, the nested logic would move to the caller side. > 3/ > For the sake of consistency, can we rename the fields from > > skipped_vacuum_count to vacuum_skipped_count, etc. ? to be similar > to fields like vacuum_count Hmm, I think skipped_vacuum_count is more consistent with fields like last_vacuum and total_vacuum_time, where the modifier comes before vacuum/analyze. What do you think about that? > 4/ > field documentation could be a bit better to match existing phrasing > > For example, the timestamp fields: > > - Last time a manual vacuum on this table was attempted but skipped due to > - lock unavailability (not counting <command>VACUUM FULL</command>) > + The time of the last manual vacuum on this table that was skipped > + due to lock unavailability (not counting <command>VACUUM FULL</command>) I intended to keep consistency with the existing last_vacuum: Last time at which this table was manually vacuumed (not counting VACUUM FULL) although "at which" was accidentally omitted. Your suggestion seems simpler and more natural to me. Should we prioritize that over consistency? > and the counter fields > > - Number of times vacuums on this table have been attempted but skipped > + Number of times a manual vacuum on this table has been skipped The "a munual" was also accidentally omitted, so I'll fix it. > 5/ > Partitioned table asymmetry between vacuum_count and vacuum_skipped_count. > > vacuum_count never increments on a the parenttable, because the parent is never > pocessed. On the other hand, if the manual VACUUM/ANALYZE is on the > parent table, > then we will skip all the children. So, we should still report the skip on the > parent table, but we should add a Notes section in the docs perhaps to > document this caveat? Yeah, we cannot report skips on the children when a manual vacuum/analyze on the parent table is skipped. (It might be possible to obtain child information with NoLock, but that would not be safe.) Therefore, I agree that the best we can do here is to add a note to the documentation of last_skipped_vacuum/analyze and skipped_vacuum/analyze_count. For example: When a manual vacuum or analyze on a parent table in an inheritance or partitioning hierarchy is skipped, the statistics are recorded only for the parent table, not for its children. > 6/ > It would be nice to add a test for this, but this requires concurrency and I'm > not sure it's woth it. I'm not sure what meaningful tests we could add for these statistics. I couldn't find any existing tests for fields like last_vacuum. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-04-13T08:05:51Z
Hello Sami Imseih, On Sat, 28 Mar 2026 16:18:02 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Fri, 27 Mar 2026 11:48:27 -0500 > Sami Imseih <samimseih@gmail.com> wrote: > > > > I've attached a revised patch reflecting this change, and it also includes > > > the documentation. > > > > Thanks fo the update! > > > > I have some comments: > > > > 1/ > > +pgstat_report_skipped_vacuum_analyze(Oid relid, bits8 flags) > > > > using bit8 is fine here, but I would have just used int. For this > > case, it's just a matter of prefernace. > > That makes sense, since using int for flags seems common in other > places in the code. I'm not sure how much it affects performance, > though. > > > 2/ > > +/* flags for pgstat_flush_backend() */ > > +#define PGSTAT_REPORT_SKIPPED_VACUUM (1 << 0) /* vacuum is skipped */ > > +#define PGSTAT_REPORT_SKIPPED_ANALYZE (1 << 1) /* analyze is skipped */ > > +#define PGSTAT_REPORT_SKIPPED_AUTOVAC (1 << 2) /* skipped > > during autovacuum/autoanalyze */ > > +#define PGSTAT_REPORT_SKIPPED_ANY (PGSTAT_REPORT_SKIPPED_VACUUM | > > PGSTAT_REPORT_SKIPPED_ANALYZE) > > > > can we just have 4 flags, SKIPPED_VACUUM, SKIPPED_ANALYZE, > > SKIPPED_AUTOVACUUM, SKIPPED_AUTOANALYZE, > > which can then remove the nested if/else and makes the mapping more obvious > > I am fine with that. In that case, the nested logic would move to the > caller side. > > > 3/ > > For the sake of consistency, can we rename the fields from > > > > skipped_vacuum_count to vacuum_skipped_count, etc. ? to be similar > > to fields like vacuum_count > > Hmm, I think skipped_vacuum_count is more consistent with > fields like last_vacuum and total_vacuum_time, where the modifier > comes before vacuum/analyze. What do you think about that? > > > 4/ > > field documentation could be a bit better to match existing phrasing > > > > For example, the timestamp fields: > > > > - Last time a manual vacuum on this table was attempted but skipped due to > > - lock unavailability (not counting <command>VACUUM FULL</command>) > > + The time of the last manual vacuum on this table that was skipped > > + due to lock unavailability (not counting <command>VACUUM FULL</command>) > > I intended to keep consistency with the existing last_vacuum: > > Last time at which this table was manually vacuumed (not counting VACUUM FULL) > > although "at which" was accidentally omitted. Your suggestion seems > simpler and more natural to me. Should we prioritize that over consistency? > > > and the counter fields > > > > - Number of times vacuums on this table have been attempted but skipped > > + Number of times a manual vacuum on this table has been skipped > > The "a munual" was also accidentally omitted, so I'll fix it. > > > 5/ > > Partitioned table asymmetry between vacuum_count and vacuum_skipped_count. > > > > vacuum_count never increments on a the parenttable, because the parent is never > > pocessed. On the other hand, if the manual VACUUM/ANALYZE is on the > > parent table, > > then we will skip all the children. So, we should still report the skip on the > > parent table, but we should add a Notes section in the docs perhaps to > > document this caveat? > > Yeah, we cannot report skips on the children when a manual > vacuum/analyze on the parent table is skipped. (It might be possible > to obtain child information with NoLock, but that would not be safe.) > > Therefore, I agree that the best we can do here is to add a note to the > documentation of last_skipped_vacuum/analyze and skipped_vacuum/analyze_count. > > For example: > > When a manual vacuum or analyze on a parent table in an inheritance > or partitioning hierarchy is skipped, the statistics are recorded > only for the parent table, not for its children. > > > 6/ > > It would be nice to add a test for this, but this requires concurrency and I'm > > not sure it's woth it. > > I'm not sure what meaningful tests we could add for these statistics. > I couldn't find any existing tests for fields like last_vacuum. I've attached a patch reflecting your comments on items 1, 2, and 5. As for items 3, 4, and 6, I am waiting for your comments, so the patch is left unchanged for now. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Sami Imseih <samimseih@gmail.com> — 2026-04-22T12:49:55Z
Thanks for the updated patch! > I've attached a patch reflecting your comments on items 1, 2, and 5. > As for items 3, 4, and 6, I am waiting for your comments, so the patch > is left unchanged for now. A few more comments: 1/ + relid = RangeVarGetRelid(vrel->relation, NoLock, false); Should this be called with "true" as the 3rd (missing_ok) argument, otherwise we end up with an error instead of a "--- relation no longer exists" log. right? 2/ Can the isolation tests src/test/isolation/specs/vacuum-skip-locked.spec be updated to check pg_stat_user_tables as well? 3/ comment fix: This: * Relation could not be opened hence generate if possible a log Should be: * Relation could not be opened, hence generate if possible a log -- Sami Imseih Amazon Web Services (AWS)
-
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-04-27T11:32:07Z
On Wed, 22 Apr 2026 07:49:55 -0500 Sami Imseih <samimseih@gmail.com> wrote: Thank you for your comments! > > 1/ > > + relid = RangeVarGetRelid(vrel->relation, NoLock, false); > > Should this be called with "true" as the 3rd (missing_ok) argument, otherwise > we end up with an error instead of a "--- relation no longer exists" log. right? No, it should be false. If missing_ok is true, VACUUM (SKIP_LOCKED) on a not-existing table would emit a "skipping vacuum of ... --- relation no longer exists" message, but it should be "relation ... does not exist". > 2/ > > Can the isolation tests > src/test/isolation/specs/vacuum-skip-locked.spec be updated > to check pg_stat_user_tables as well? Yes, we can. I've attached an updated patch including that test. While working on the test, I noticed that skipped FULL VACUUM was counted in the previous patch, so I fixed it not to avoid counting those cases. > 3/ comment fix: > > This: > * Relation could not be opened hence generate if possible a log > > Should be: > * Relation could not be opened, hence generate if possible a log Fixed. The names of the new fields are still open. The current pattern is "last_skipped_..." and "skipped_..._count". Alternatively, we could use "..._last_skip" and "..._skip_count", which would be consistent with slotsync_skip_count and slosync_last_skip. Which do you think is better? Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Sami Imseih <samimseih@gmail.com> — 2026-05-04T20:44:57Z
Thanks for the update! > > > > 1/ > > > > + relid = RangeVarGetRelid(vrel->relation, NoLock, false); > > > > Should this be called with "true" as the 3rd (missing_ok) argument, otherwise > > we end up with an error instead of a "--- relation no longer exists" log. right? > > No, it should be false. If missing_ok is true, VACUUM (SKIP_LOCKED) on a not-existing > table would emit a "skipping vacuum of ... --- relation no longer exists" message, but > it should be "relation ... does not exist". Yeah you are right. But, after looking more into this, I still think the expand_vacuum_rel() changes can be improved. The branching - */ - if (!OidIsValid(relid)) + if (!(options & VACOPT_SKIP_LOCKED)) { - if (options & VACOPT_VACUUM) - ereport(WARNING, - (errcode(ERRCODE_LOCK_NOT_AVAILABLE), - errmsg("skipping vacuum of \"%s\" --- lock not available", - vrel->relation->relname))); - else - ereport(WARNING, - (errcode(ERRCODE_LOCK_NOT_AVAILABLE), - errmsg("skipping analyze of \"%s\" --- lock not available", + relid = RangeVarGetRelidExtended(vrel->relation, + AccessShareLock, + 0, NULL, NULL); + if (!OidIsValid(relid)) + return vacrels; + } + else + { + /* Get relid for reporting before taking a lock */ + relid = RangeVarGetRelid(vrel->relation, NoLock, false); + + if (!ConditionalLockRelationOid(relid, AccessShareLock)) is not needed. We can continue just using RangeVarGetRelidExtended() with the rvr_opts and an AccessExclusiveLock, and once we need to report that we cannot obtain the lock, RangeVarGetRelid() can be called at that point for the purpose of calling pgstat_report_skipped_vacuum_analyze(). This is safer than calling ConditionalLockRelationOid() on a relid obtained with NoLock. See attached v6. >> 2/ >> >> Can the isolation tests >> src/test/isolation/specs/vacuum-skip-locked.spec be updated >> to check pg_stat_user_tables as well? > Yes, we can. I've attached an updated patch including that test. > While working on the test, I noticed that skipped FULL VACUUM was counted > in the previous patch, so I fixed it not to avoid counting those cases. The tests looks good to me. > The names of the new fields are still open. The current pattern is > "last_skipped_..." and "skipped_..._count". Alternatively, we could use > "..._last_skip" and "..._skip_count", which would be consistent with > slotsync_skip_count and slosync_last_skip. > Which do you think is better? I think last_skipped_* is better since we use last_vacuum, last_autovacuum, etc. -- Sami -
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-05-12T09:47:21Z
On Mon, 4 May 2026 15:44:57 -0500 Sami Imseih <samimseih@gmail.com> wrote: Thank you for your review! > > > 1/ > > > > > > + relid = RangeVarGetRelid(vrel->relation, NoLock, false); > > > > > > Should this be called with "true" as the 3rd (missing_ok) argument, otherwise > > > we end up with an error instead of a "--- relation no longer exists" log. right? > > > > No, it should be false. If missing_ok is true, VACUUM (SKIP_LOCKED) on a not-existing > > table would emit a "skipping vacuum of ... --- relation no longer exists" message, but > > it should be "relation ... does not exist". > > Yeah you are right. > > But, after looking more into this, I still think the > expand_vacuum_rel() changes can be > improved. The branching > - */ > - if (!OidIsValid(relid)) > + if (!(options & VACOPT_SKIP_LOCKED)) > { > - if (options & VACOPT_VACUUM) > - ereport(WARNING, > - > (errcode(ERRCODE_LOCK_NOT_AVAILABLE), > - errmsg("skipping > vacuum of \"%s\" --- lock not available", > - > vrel->relation->relname))); > - else > - ereport(WARNING, > - > (errcode(ERRCODE_LOCK_NOT_AVAILABLE), > - errmsg("skipping > analyze of \"%s\" --- lock not available", > + relid = RangeVarGetRelidExtended(vrel->relation, > + > AccessShareLock, > + > 0, NULL, NULL); > + if (!OidIsValid(relid)) > + return vacrels; > + } > + else > + { > + /* Get relid for reporting before taking a lock */ > + relid = RangeVarGetRelid(vrel->relation, NoLock, false); > + > + if (!ConditionalLockRelationOid(relid, AccessShareLock)) > > is not needed. We can continue just using RangeVarGetRelidExtended() > with the rvr_opts and an AccessExclusiveLock, and once we need to > report that we cannot obtain the lock, RangeVarGetRelid() can be > called at that point for the purpose of calling > pgstat_report_skipped_vacuum_analyze(). This is safer than calling > ConditionalLockRelationOid() on a relid obtained with NoLock. See > attached v6. It seems good to me. Initially, I was concerned that something might go wrong if a concurrent session performed DROP TABLE or ALTER TABLE RENAME between RangeVarGetRelidExtended() and RangeVarGetRelid(), but I could not find any actual issue. Even when the table name is changed, the correct statistics entry is updated correctly. So I'm fine with your version. Regards, Yugo Nagata > >> 2/ > >> > >> Can the isolation tests > >> src/test/isolation/specs/vacuum-skip-locked.spec be updated > >> to check pg_stat_user_tables as well? > > > Yes, we can. I've attached an updated patch including that test. > > > While working on the test, I noticed that skipped FULL VACUUM was counted > > in the previous patch, so I fixed it not to avoid counting those cases. > > The tests looks good to me. > > > The names of the new fields are still open. The current pattern is > > "last_skipped_..." and "skipped_..._count". Alternatively, we could use > > "..._last_skip" and "..._skip_count", which would be consistent with > > slotsync_skip_count and slosync_last_skip. > > > Which do you think is better? > > I think last_skipped_* is better since we use last_vacuum, last_autovacuum, etc. > > -- > Sami -- Yugo Nagata <nagata@sraoss.co.jp> -
Re: Track skipped tables during autovacuum and autoanalyze
Sami Imseih <samimseih@gmail.com> — 2026-05-12T14:50:18Z
> So I'm fine with your version. Thanks! I marked the CF entry as RFC. -- Sami
-
Re: Track skipped tables during autovacuum and autoanalyze
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-14T21:46:47Z
Hello! + TimestampTz last_skipped_vacuum_time; /* user initiated vacuum */ + PgStat_Counter skipped_vacuum_count; + TimestampTz last_skipped_autovacuum_time; /* autovacuum initiated */ + PgStat_Counter skipped_autovacuum_count; + TimestampTz last_skipped_analyze_time; /* user initiated */ + PgStat_Counter skipped_analyze_count; + TimestampTz last_skipped_autoanalyze_time; /* autovacuum initiated */ + PgStat_Counter skipped_autoanalyze_count; + Doesn't these also require a PGSTAT_FILE_FORMAT_ID change? There's also an asymmetric case for the skipped counters, is that intentional? | Command | `skipped_vacuum_count` | `skipped_analyze_count` | |-----------------------------------------|------------------------|-------------------------| | `VACUUM (FULL, ANALYZE, SKIP_LOCKED) t` | 0 | 1 | | `VACUUM (ANALYZE, SKIP_LOCKED) t` | 1 | 1 | | `VACUUM (FULL, SKIP_LOCKED) t` | 0 | 0 | > Initially, I was concerned that something might go wrong if a concurrent > session performed DROP TABLE or ALTER TABLE RENAME between RangeVarGetRelidExtended() > and RangeVarGetRelid(), but I could not find any actual issue. Even when the table > name is changed, the correct statistics entry is updated correctly. A DROP TABLE can cause a missed skip in statistics, which is reproducible with a custom injection point and tap test, see the attached patch. The race window is quite minimal, but it exists. -
Re: Track skipped tables during autovacuum and autoanalyze
Sami Imseih <samimseih@gmail.com> — 2026-05-15T00:59:18Z
> Doesn't these also require a PGSTAT_FILE_FORMAT_ID change? right. that was missed. Fixed in the attached. > There's also an asymmetric case for the skipped counters, is that intentional? > > | Command | `skipped_vacuum_count` | > `skipped_analyze_count` | > |-----------------------------------------|------------------------|-------------------------| > | `VACUUM (FULL, ANALYZE, SKIP_LOCKED) t` | 0 | 1 > | > | `VACUUM (ANALYZE, SKIP_LOCKED) t` | 1 | 1 > | > | `VACUUM (FULL, SKIP_LOCKED) t` | 0 | 0 Yeah, this is because vacuum_count and last_vacuum also skip VACUUM FULL. That was mentioned earlier in the thread. > > Initially, I was concerned that something might go wrong if a concurrent > > session performed DROP TABLE or ALTER TABLE RENAME between RangeVarGetRelidExtended() > > and RangeVarGetRelid(), but I could not find any actual issue. Even when the table > > name is changed, the correct statistics entry is updated correctly. > > A DROP TABLE can cause a missed skip in statistics, which is > reproducible with a custom injection point and tap test, see the > attached patch. The race window is quite minimal, but it exists. If the table is dropped, there are no stats to update. right? -- Sami
-
Re: Track skipped tables during autovacuum and autoanalyze
Yugo Nagata <nagata@sraoss.co.jp> — 2026-05-15T05:03:02Z
On Thu, 14 May 2026 19:59:18 -0500 Sami Imseih <samimseih@gmail.com> wrote: > > Doesn't these also require a PGSTAT_FILE_FORMAT_ID change? > > right. that was missed. Fixed in the attached. Thank you for updating the patch. > > > There's also an asymmetric case for the skipped counters, is that intentional? > > > > | Command | `skipped_vacuum_count` | > > `skipped_analyze_count` | > > |-----------------------------------------|------------------------|-------------------------| > > | `VACUUM (FULL, ANALYZE, SKIP_LOCKED) t` | 0 | 1 > > | > > | `VACUUM (ANALYZE, SKIP_LOCKED) t` | 1 | 1 > > | > > | `VACUUM (FULL, SKIP_LOCKED) t` | 0 | 0 > > Yeah, this is because vacuum_count and last_vacuum also skip VACUUM FULL. > That was mentioned earlier in the thread. Right. > > > Initially, I was concerned that something might go wrong if a concurrent > > > session performed DROP TABLE or ALTER TABLE RENAME between RangeVarGetRelidExtended() > > > and RangeVarGetRelid(), but I could not find any actual issue. Even when the table > > > name is changed, the correct statistics entry is updated correctly. > > > > A DROP TABLE can cause a missed skip in statistics, which is > > reproducible with a custom injection point and tap test, see the > > attached patch. The race window is quite minimal, but it exists. > > If the table is dropped, there are no stats to update. right? In my analysis, even if the table is dropped or renamed just before calling RangeVarGetRelid() (at the injection point you added), RangeVarGetRelid() still returns the table's OID. So it seems possible that the statistics entry is updated locally, but it would be released shortly afterward and thus eventually becomes invisible. However, I'm not entirely sure whether this behavior is always guaranteed. Could anyone clarify this? Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: Track skipped tables during autovacuum and autoanalyze
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-15T09:02:55Z
> If the table is dropped, there are no stats to update. right? Ops, right. I focused too much on "all warnings should be visible in the statistic, so the sum of warnings and statistics should match", but of course that's not the case. > However, I'm not entirely sure whether this behavior is always guaranteed. > Could anyone clarify this? There's another different corner-case if I move the injection point inside pgstat_report_skipped_vacuum_analyze, after releasing the syscache. If the drop happens at that point, we insert an orphaned record into the statistics, and it will be visible with the internal functions (e.g. pg_stat_get_skipped_autoanalyze_count). It is still invisible in the pg_stat_all_tables view, but now that I've looked more at the code, I think internally it will stay there permanently, even surviving pg_stat_reset? > RangeVarGetRelid() still returns the table's OID Yes, I also reached the same conclusion, I started testing because I tried to see if I could break the double relid retrieval by some scenarios (alter rename + create, drop-create etc), but it's not possible. The ereports can execute CHECK_FOR_INTERRUPTS, but that never calls ProcessCatchupInterrupt, and because of that it never runs AccceptInvalidationMessages. And that's when I noticed that the warning isn't visible in the statistics at all, and got distracted...
-
Re: Track skipped tables during autovacuum and autoanalyze
Sami Imseih <samimseih@gmail.com> — 2026-05-15T16:12:36Z
> > If the table is dropped, there are no stats to update. right? > > Ops, right. I focused too much on "all warnings should be visible in > the statistic, so the sum of warnings and statistics should match", > but of course that's not the case. > > > However, I'm not entirely sure whether this behavior is always guaranteed. > > Could anyone clarify this? > > There's another different corner-case if I move the injection point > inside pgstat_report_skipped_vacuum_analyze, after releasing the > syscache. > > If the drop happens at that point, we insert an orphaned record into > the statistics, and it will be visible with the internal functions > (e.g. pg_stat_get_skipped_autoanalyze_count). > > It is still invisible in the pg_stat_all_tables view, but now that > I've looked more at the code, I think internally it will stay there > permanently, even surviving pg_stat_reset? Yeah, you’re right. I just realized that pgstat_get_entry_ref_locked() creates an entry if one does not already exist, which means we can leak entries if the table is dropped concurrently. After releasing the syscache entry, we still have the relid, but the table may be dropped before the stats update runs. In that case, the stats update recreates the relation entry, and that entry is then leaked permanently. The tricky part of this feature is that we cannot hold a lock while updating the entry, because the whole point of the stat is to track cases where we failed to acquire a lock. So, it seems pgstat_init_function_usage() deals with the same pattern and they deal with it by doing a second syscache lookup after creating the entry, and if the relation is gone, drop the entry. We can do the same thing here. See v8-0001. I also attached a v8-0002 building on Zsolt's injection point test and his earlier suggestion to "move the injection point inside pgstat_report_skipped_vacuum_analyze, after releasing the syscache". These are isolation tests that rely on injection points. The tests are: 1. Table Drop + rollback. Lock is skipped and table is dropped but rolled back. The stats are updated. 2. Table Drop + commit. Lock is skipped and table is dropped and committed. No table entry to record a stat for. The tests use pg_stat_get_skipped_vacuum_count to verify that no orphaned entry is left behind. -- Sami
-
Re: Track skipped tables during autovacuum and autoanalyze
Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-15T20:21:01Z
Looks good and seems to handle the previous corner case, I only have two very minor comments: @@ -391,6 +392,7 @@ pgstat_report_skipped_vacuum_analyze(Oid relid, int flags) return; /* somebody deleted the rel, forget it */ isshared = ((Form_pg_class) GETSTRUCT(classTup))->relisshared; ReleaseSysCache(classTup); + INJECTION_POINT("expand-vacuum-rel-skip-locked", NULL); Nitpick, but now that the injection point lives inside pgstat_report_skipped_vacuum_analyze, it should be named accordingly. + * This mirrors the approach used in pgstat_init_function_usage() for + * functions. pgstat_init_function_usage only drops the entry conditionally if it was created immediately before, so it is not entirely the same pattern, I'm not sure if I would say "mirrors". Maybe similar to? But this is again just nitpick. The flow there (pending_entry, created flag => only rechecking on freshly created) has the advantage that it only runs AcceptInvalidationMessages+recheck when needed, but here it would also require an additional pgstat_lock_entry call. -
Re: Track skipped tables during autovacuum and autoanalyze
Sami Imseih <samimseih@gmail.com> — 2026-05-16T14:29:29Z
> + INJECTION_POINT("expand-vacuum-rel-skip-locked", NULL); > Nitpick, but now that the injection point lives inside > pgstat_report_skipped_vacuum_analyze, it should be named accordingly. right. I renamed it to skipped-vacuum-analyze-before-entry-lock > Nitpick, but now that the injection point lives inside > pgstat_report_skipped_vacuum_analyze, it should be named accordingly. > > + * This mirrors the approach used in pgstat_init_function_usage() for > + * functions. Fair. I just removed this comment. On re-reading, it really did not add more value. Attached v9. Thanks! -- Sami