From d6f2446c1e7a73f1e17ae0015f0f9f88c2b77f25 Mon Sep 17 00:00:00 2001 From: Manuel Reyes Bravo Date: Tue, 3 Nov 2026 21:46:17 -0300 Subject: [PATCH v6 2/2] 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. --- src/test/modules/test_progress/DocPhases.pm | 190 ++++++++++++++++++ src/test/modules/test_progress/Makefile | 5 + src/test/modules/test_progress/meson.build | 3 + .../modules/test_progress/t/001_progress.pl | 3 +- .../modules/test_progress/t/002_doc_phases.pl | 95 +++++++++ 5 files changed, 295 insertions(+), 1 deletion(-) 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..36d74e3ed69 --- /dev/null +++ b/src/test/modules/test_progress/DocPhases.pm @@ -0,0 +1,190 @@ +# 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 4a47cb7d09a..80a77f42ad9 100644 --- a/src/test/modules/test_progress/Makefile +++ b/src/test/modules/test_progress/Makefile @@ -21,3 +21,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/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 da4eb676e78..ff28ea822f2 100644 --- a/src/test/modules/test_progress/t/001_progress.pl +++ b/src/test/modules/test_progress/t/001_progress.pl @@ -1630,7 +1630,8 @@ my ($part_run) = grep { @{ $_->[0] } } map { my $i = $_; - [ map { (values_of($trace, 'ANALYZE', $part_relid, $_))[$i] } + [ + map { (values_of($trace, 'ANALYZE', $part_relid, $_))[$i] } map { "PROGRESS_ANALYZE_$_" } qw(CURRENT_CHILD_TABLE_RELID BLOCKS_TOTAL BLOCKS_DONE CHILD_TABLES_DONE) ] 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..181f79b133f --- /dev/null +++ b/src/test/modules/test_progress/t/002_doc_phases.pl @@ -0,0 +1,95 @@ +# 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; +$node->append_conf('postgresql.conf', 'trace_progress = on'); +# REPACK (CONCURRENTLY) refuses to run below "replica" +$node->append_conf('postgresql.conf', 'wal_level = replica'); +$node->start; + +# The indexes are built before the rows go in, which keeps a trace of every +# tuple they index out of the server log. +$node->safe_psql('postgres', + q{CREATE TABLE phases_tab (a int primary key, b text)}); +$node->safe_psql('postgres', q{CREATE INDEX phases_b ON phases_tab (b)}); +$node->safe_psql('postgres', + q{INSERT INTO phases_tab SELECT g, repeat('y', 40) FROM generate_series(1, 2000) g} +); +$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); + + # One check per command, over all of its runs: how many runs report + # phases varies, as a parallel worker reports some only when it gets to + # process an index. + my %ran = ProgressCheck::commands_run($trace); + for my $command (sort keys %ran) + { + next unless defined $spec->{$command}{phase_param}; + + # A value the view or the documentation does not know about is a + # plain bug: the three places have drifted apart. + my @undocumented = + map { DocPhases::check_against_trace($doc, $command, $_) } + grep { @$_ } + ProgressCheck::phases_of($trace, $spec, $command, undef); + is(scalar @undocumented, + 0, "$name: every phase $command reports is documented") + or diag(join("\n", map { $_->{detail} } @undocumented)); + } +} + +done_testing(); -- 2.56.0