From 443ded06d14ac4b76fe5d057627d52a72d9395e0 Mon Sep 17 00:00:00 2001 From: Manu Date: Tue, 22 Sep 2026 21:33:44 -0300 Subject: [PATCH v3 6/6] Check the documented progress phases against the reported ones A progress phase lives in three places that nothing keeps in sync: its value in commands/progress.h, the text the view gives it in system_views.sql, and its row in the phase table of monitoring.sgml. Adding a phase in two of them compiles and passes every test. The new test 002_doc_phases.pl runs VACUUM, ANALYZE, CLUSTER, CREATE INDEX CONCURRENTLY and REPACK (CONCURRENTLY) on a PROGRESS_DEBUG build, and checks that each phase they report is mapped to a text by the view and listed in the documentation. The row order of the tables is not checked, as it is not meant as a guarantee of the order of execution; 001_progress.pl checks the actual succession of phases. The check for a PROGRESS_DEBUG build moves into ProgressCheck.pm as compiled_with_progress_debug(), so that both tests detect it the same way. --- src/test/modules/test_progress/DocPhases.pm | 193 ++++++++++++++++++ src/test/modules/test_progress/Makefile | 5 + .../modules/test_progress/ProgressCheck.pm | 24 +++ src/test/modules/test_progress/meson.build | 3 + .../modules/test_progress/t/001_progress.pl | 10 +- .../modules/test_progress/t/002_doc_phases.pl | 93 +++++++++ 6 files changed, 319 insertions(+), 9 deletions(-) create mode 100644 src/test/modules/test_progress/DocPhases.pm create mode 100644 src/test/modules/test_progress/t/002_doc_phases.pl diff --git a/src/test/modules/test_progress/DocPhases.pm b/src/test/modules/test_progress/DocPhases.pm new file mode 100644 index 00000000000..535b742a9be --- /dev/null +++ b/src/test/modules/test_progress/DocPhases.pm @@ -0,0 +1,193 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +=pod + +=head1 NAME + +DocPhases - cross-check the documented phases against the ones a command +really reports + +=head1 SYNOPSIS + + use DocPhases; + + my $doc = DocPhases::load($share_dir); + my @problems = DocPhases::check_against_trace($doc, 'VACUUM', \@phases_seen); + +=head1 DESCRIPTION + +A progress phase exists in three places in the tree, and nothing checks +that the three agree: + +=over + +=item * F defines the value +(C is 1). + +=item * F turns the value into the +text the user sees (C). + +=item * F lists the phases in a table. + +=back + +This module reads the last two and compares them with what a command +actually reported, so that a phase the view does not name, or the +documentation does not list, becomes visible. The row order of the table +is not checked: it is not meant as a guarantee of the order of execution. + +Nothing here is hardcoded: adding a phase in the three places keeps the +check quiet, adding it in two of them does not. + +=cut + +package DocPhases; + +use strict; +use warnings FATAL => 'all'; + +use Carp; + +# view name -> the phase table in monitoring.sgml +our %PHASE_TABLE = ( + pg_stat_progress_vacuum => 'vacuum-phases', + pg_stat_progress_analyze => 'analyze-phases', + pg_stat_progress_cluster => 'cluster-phases', + pg_stat_progress_repack => 'repack-phases', + pg_stat_progress_create_index => 'create-index-phases', + pg_stat_progress_basebackup => 'basebackup-phases', +); + +# command name as PROGRESS_DEBUG logs it -> view name +our %COMMAND_VIEW = ( + VACUUM => 'pg_stat_progress_vacuum', + ANALYZE => 'pg_stat_progress_analyze', + CLUSTER => 'pg_stat_progress_cluster', + REPACK => 'pg_stat_progress_repack', + # progress.h calls it CREATE_INDEX, which is how PROGRESS_DEBUG logs it + CREATE_INDEX => 'pg_stat_progress_create_index', + BASEBACKUP => 'pg_stat_progress_basebackup', +); + +=pod + +=head2 load($system_views_sql, $monitoring_sgml) + +Read the phase texts from system_views.sql and the phases documented in +monitoring.sgml. Returns a hashref keyed by view name, each holding + + value_to_text { 0 => 'initializing', 1 => 'scanning heap', ... } + documented { 'initializing' => 1, 'scanning heap' => 1, ... } + +The two paths come from the Makefile, the same way the module that reads +progress.h gets its own. + +=cut + +sub load +{ + my ($system_views_sql, $monitoring_sgml) = @_; + my %out; + + my $views = _slurp($system_views_sql); + my $sgml = _slurp($monitoring_sgml); + + for my $view (keys %PHASE_TABLE) + { + # the CASE that maps the phase parameter to its text + next unless $views =~ /CREATE VIEW \Q$view\E AS(.*?);\n/s; + my $body = $1; + # The phase CASE, and only it: some views have another CASE just + # before it (create_index maps param1 to a command name), so the + # match must not run from one CASE into the next one's END. The + # keyword case varies too ("END as phase"). + next + unless $body =~ + /CASE \s+ S\.param\d+ ((?: (?! \bEND\s+AS\b ) . )*?) \s+ END \s+ AS \s+ phase/isx; + my $case = $1; + + my %v2t; + while ($case =~ /WHEN\s+(\d+)\s+THEN\s+'([^']*)'/g) + { + $v2t{$1} = $2; + } + next unless %v2t; + + my $id = $PHASE_TABLE{$view}; + my %documented; + if ($sgml =~ /(.*?)<\/table>/s) + { + my $table = $1; + while ($table =~ /([^<]*)<\/literal><\/entry>/g) + { + $documented{$1} = 1; + } + } + + $out{$view} = + { value_to_text => \%v2t, documented => \%documented }; + } + + return \%out; +} + +=pod + +=head2 check_against_trace($doc, $command, $phases) + +$phases is the sequence of phase values a single command run reported, as +ProgressCheck::phases_of() returns it. Returns a list of problems, each a +hashref with 'kind' and 'detail'. The only kind is 'undocumented': a value +was reported that system_views.sql does not map to a text, or whose text +the documentation table does not list. Each such value is reported once. + +=cut + +sub check_against_trace +{ + my ($doc, $command, $phases) = @_; + my @problems; + + my $view = $COMMAND_VIEW{$command} or return (); + my $d = $doc->{$view} or return (); + + my %reported; + for my $val (@$phases) + { + next if $reported{$val}++; + + my $text = $d->{value_to_text}{$val}; + if (!defined $text) + { + push @problems, + { + kind => 'undocumented', + detail => "$command reported phase value $val, which " + . "system_views.sql does not map to any text" + }; + } + elsif (!$d->{documented}{$text}) + { + push @problems, + { + kind => 'undocumented', + detail => "$command reported phase \"$text\", which is not " + . "listed in the documentation table" + }; + } + } + + return @problems; +} + +sub _slurp +{ + my ($path) = @_; + open my $fh, '<', $path or croak "could not open $path: $!"; + local $/; + my $c = <$fh>; + close $fh; + return $c; +} + +1; diff --git a/src/test/modules/test_progress/Makefile b/src/test/modules/test_progress/Makefile index f5dbca84920..38afb1198b2 100644 --- a/src/test/modules/test_progress/Makefile +++ b/src/test/modules/test_progress/Makefile @@ -18,3 +18,8 @@ endif # The test checks its description of the progress parameters against this. export PROGRESS_H := $(abs_top_srcdir)/src/include/commands/progress.h + +# ... and the phases a command reports against the text the view gives them +# and the table the documentation lists them in. +export SYSTEM_VIEWS_SQL := $(abs_top_srcdir)/src/backend/catalog/system_views.sql +export MONITORING_SGML := $(abs_top_srcdir)/doc/src/sgml/monitoring.sgml diff --git a/src/test/modules/test_progress/ProgressCheck.pm b/src/test/modules/test_progress/ProgressCheck.pm index 4a1daf910a7..1a301628898 100644 --- a/src/test/modules/test_progress/ProgressCheck.pm +++ b/src/test/modules/test_progress/ProgressCheck.pm @@ -39,6 +39,7 @@ use strict; use warnings FATAL => 'all'; use Carp; +use PostgreSQL::Test::Utils qw(run_command slurp_file); # The number of parameters in a backend's progress state; see # PGSTAT_NUM_PROGRESS_PARAM. @@ -216,6 +217,29 @@ our %COMMANDS = ( =over +=item compiled_with_progress_debug($progress_h) + +Return true if the server is compiled with PROGRESS_DEBUG. It is set in +the compiler flags, as a buildfarm animal would (CPPFLAGS or CFLAGS with +configure, c_args with meson), or in pg_config_manual.h, which is found +next to $progress_h. This does not start a server, so that a test costs +nothing in other builds. + +=cut + +sub compiled_with_progress_debug +{ + my ($progress_h) = @_; + my ($cppflags) = run_command([ 'pg_config', '--cppflags' ]); + my ($cflags) = run_command([ 'pg_config', '--cflags' ]); + (my $manual_h = $progress_h) =~ + s{commands/progress\.h$}{pg_config_manual.h}; + return "$cppflags $cflags" =~ /-DPROGRESS_DEBUG\b/ + || slurp_file($manual_h) =~ /^\s*#\s*define\s+PROGRESS_DEBUG\b/m; +} + +=pod + =item load_spec($progress_h) Read the macros of commands/progress.h and resolve %COMMANDS against them. diff --git a/src/test/modules/test_progress/meson.build b/src/test/modules/test_progress/meson.build index 0f8a6d86dba..0743267d106 100644 --- a/src/test/modules/test_progress/meson.build +++ b/src/test/modules/test_progress/meson.build @@ -7,9 +7,12 @@ tests += { 'tap': { 'env': { 'PROGRESS_H': meson.project_source_root() / 'src/include/commands/progress.h', + 'SYSTEM_VIEWS_SQL': meson.project_source_root() / 'src/backend/catalog/system_views.sql', + 'MONITORING_SGML': meson.project_source_root() / 'doc/src/sgml/monitoring.sgml', }, 'tests': [ 't/001_progress.pl', + 't/002_doc_phases.pl', ], # The test reads the server log, which is cluster-wide. 'runningcheck': false, diff --git a/src/test/modules/test_progress/t/001_progress.pl b/src/test/modules/test_progress/t/001_progress.pl index b665b76e80b..7e4cc206de9 100644 --- a/src/test/modules/test_progress/t/001_progress.pl +++ b/src/test/modules/test_progress/t/001_progress.pl @@ -31,15 +31,7 @@ is_deeply(\@problems, [], 'ProgressCheck.pm describes every macro of commands/progress.h') or diag(join("\n", @problems)); -# Is the server compiled with PROGRESS_DEBUG? It is set in the compiler -# flags, as a buildfarm animal would (CPPFLAGS or CFLAGS with configure, -# c_args with meson), or in pg_config_manual.h. Find out without starting -# a server, so that the test costs nothing in other builds. -my ($cppflags) = run_command([ 'pg_config', '--cppflags' ]); -my ($cflags) = run_command([ 'pg_config', '--cflags' ]); -(my $manual_h = $progress_h) =~ s{commands/progress\.h$}{pg_config_manual.h}; -if ("$cppflags $cflags" !~ /-DPROGRESS_DEBUG\b/ - && slurp_file($manual_h) !~ /^\s*#\s*define\s+PROGRESS_DEBUG\b/m) +if (!ProgressCheck::compiled_with_progress_debug($progress_h)) { note 'not compiled with PROGRESS_DEBUG, skipping the traces'; done_testing(); diff --git a/src/test/modules/test_progress/t/002_doc_phases.pl b/src/test/modules/test_progress/t/002_doc_phases.pl new file mode 100644 index 00000000000..67a9d096dc4 --- /dev/null +++ b/src/test/modules/test_progress/t/002_doc_phases.pl @@ -0,0 +1,93 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Check the progress phases a command really reports against the phases the +# documentation lists for it. +# +# A phase lives in three places that nothing keeps in sync: its value in +# progress.h, its text in system_views.sql, and its row in the table in +# monitoring.sgml. This test runs the commands and compares. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +use FindBin; +use lib $FindBin::RealBin . '/..'; + +use ProgressCheck; +use DocPhases; + +my $progress_h = $ENV{PROGRESS_H} + or plan skip_all => 'PROGRESS_H is not set'; +plan skip_all => 'not compiled with PROGRESS_DEBUG' + unless ProgressCheck::compiled_with_progress_debug($progress_h); + +my $system_views = $ENV{SYSTEM_VIEWS_SQL} + or plan skip_all => 'SYSTEM_VIEWS_SQL is not set'; +my $monitoring = $ENV{MONITORING_SGML} + or plan skip_all => 'MONITORING_SGML is not set'; + +my $node = PostgreSQL::Test::Cluster->new('doc_phases'); +$node->init; +# several rounds of index vacuuming, so that VACUUM reports each of its +# phases more than once +$node->append_conf('postgresql.conf', 'maintenance_work_mem = 1024'); +# REPACK (CONCURRENTLY) refuses to run below "replica" +$node->append_conf('postgresql.conf', 'wal_level = replica'); +$node->start; + +$node->safe_psql('postgres', + q{CREATE TABLE phases_tab (a int primary key, b text)}); +$node->safe_psql('postgres', + q{INSERT INTO phases_tab SELECT g, repeat('y', 40) FROM generate_series(1, 200000) g}); +$node->safe_psql('postgres', q{CREATE INDEX phases_b ON phases_tab (b)}); +$node->safe_psql('postgres', q{DELETE FROM phases_tab}); + +my $doc = DocPhases::load($system_views, $monitoring); +ok(keys %$doc, 'found phase tables in the documentation'); + +my $spec = ProgressCheck::load_spec($progress_h); + +my @commands = ( + [ 'VACUUM' => 'VACUUM phases_tab' ], + [ 'ANALYZE' => 'ANALYZE phases_tab' ], + [ 'CLUSTER' => 'CLUSTER phases_tab USING phases_tab_pkey' ], + [ 'CREATE INDEX CONCURRENTLY' => + 'CREATE INDEX CONCURRENTLY phases_c ON phases_tab (a)' ], + [ 'REPACK (CONCURRENTLY)' => 'REPACK (CONCURRENTLY) phases_tab' ], +); + +for my $c (@commands) +{ + my ($name, $sql) = @$c; + + my $offset = -s $node->logfile; + $node->safe_psql('postgres', $sql); + + my $contents = slurp_file($node->logfile, $offset); + my $trace = ProgressCheck::parse_log($contents); + + my %ran = ProgressCheck::commands_run($trace); + for my $command (sort keys %ran) + { + next unless defined $spec->{$command}{phase_param}; + + for my $phases (ProgressCheck::phases_of($trace, $spec, $command, undef)) + { + next unless @$phases; + + # A value the view or the documentation does not know about is a + # plain bug: the three places have drifted apart. + my @undocumented = + DocPhases::check_against_trace($doc, $command, $phases); + is(scalar @undocumented, 0, + "$name: every phase $command reports is documented") + or diag(join("\n", map { $_->{detail} } @undocumented)); + } + } +} + +done_testing(); -- 2.55.0