On Wed, Jul 22, 2026 at 3:51 AM Sami Imseih <[email protected]> wrote:
>
> Sorry, I missed attaching the injection point test.
>
> v11 includes it.
Thanks for the updated patches! I have a few comments.
In 0001,
> +step check_stat
> +{
> + SELECT relname,
> + vacuum_count, lock_skipped_vacuum_count,
> + analyze_count, lock_skipped_analyze_count
> + FROM pg_stat_all_tables
> + WHERE relname IN ('parted', 'part1', 'part2');
> +}
How about adding "ORDER BY relname" here? The row order is not
guaranteed otherwise.
> +{ oid => '8142', descr => 'statistics: last skipped vacuum time for a table',
> + proname => 'pg_stat_get_last_lock_skipped_vacuum_time', provolatile => 's',
The proname now says lock_skipped, but the descr still says just
"skipped". I think the descr should mention the lock cause as well,
and the same applies to the other seven entries.
> +#define PGSTAT_REPORT_SKIPPED_VACUUM (1 << 0) /* vacuum is skipped */
> +#define PGSTAT_REPORT_SKIPPED_ANALYZE (1 << 1) /* analyze is
> skipped */
The struct fields were renamed to lock_skipped_*, but these flags and
pgstat_report_skipped_vacuum_analyze() keep the generic name. Should
we rename them too, for consistency?
In 0002,
> + 'vacuum_skip_lock_stats',
This also needs to be added to the ISOLATION list in
src/test/modules/injection_points/Makefile. Without it the test never
runs in the autoconf build. I confirmed it passes once registered
there.
--
Best regards,
Shinya Kato
NTT OSS Center
