pgsql: Trial fix for old cross-version upgrades.

On Thu, 2025-02-20 at 18:30 +0000, Jeff Davis wrote:

Trial fix for old cross-version upgrades.

That fixed one problem, but there are other problems with 9.2 upgrades.
And it looks like there's also a problem with upgrades from 12.
Investigating...

Regards,
Jeff Davis

On Thu, 2025-02-20 at 13:17 -0800, Jeff Davis wrote:

On Thu, 2025-02-20 at 18:30 +0000, Jeff Davis wrote:

Trial fix for old cross-version upgrades.

That fixed one problem, but there are other problems with 9.2
upgrades.

The version that I committed had the following change to
002_pg_upgrade.pl:

# Stabilize stats before pg_dumpall.
$oldnode->append_conf('postgresql.conf', 'autovacuum = off');
$oldnode->restart;

...

$newnode->append_conf('postgresql.conf', 'autovacuum = off');

I think we need a similar change in the buildfarm client's
TestUpgradeXversion.pm? Is -hackers the right place to discuss that?

Regards,
Jeff Davis

On Fri, Feb 21, 2025 at 7:57 PM Jeff Davis <pgsql@j-davis.com> wrote:

I think we need a similar change in the buildfarm client's
TestUpgradeXversion.pm? Is -hackers the right place to discuss that?

Yes, I believe -hackers is the right place to discuss that.

--
Robert Haas
EDB: http://www.enterprisedb.com

Jeff Davis <pgsql@j-davis.com> writes:

The version that I committed had the following change to
002_pg_upgrade.pl:

# Stabilize stats before pg_dumpall.
$oldnode->append_conf('postgresql.conf', 'autovacuum = off');
$oldnode->restart;

...

$newnode->append_conf('postgresql.conf', 'autovacuum = off');

I think we need a similar change in the buildfarm client's
TestUpgradeXversion.pm? Is -hackers the right place to discuss that?

I think we might indeed want that, but it doesn't seem to be the
explanation for the buildfarm failures, because the diffs look
to be consistent across runs which you'd not expect from
autovacuum-driven changes. I suspect that the problem is that
pg_dump is interpreting old-version stats in some way that doesn't
match up with what we get from restoring the dump.

I did experiment with the attached very-quick-n-dirty patch, which
should succeed in suppressing autovacuum in both the old and new
versions if I understand the code correctly (which I might well not).
It made no difference at all in the dump diffs ...

regards, tom lane

On Fri, 2025-02-21 at 21:00 -0500, Tom Lane wrote:

I think we might indeed want that, but it doesn't seem to be the
explanation for the buildfarm failures, because the diffs look
to be consistent across runs which you'd not expect from
autovacuum-driven changes.  I suspect that the problem is that
pg_dump is interpreting old-version stats in some way that doesn't
match up with what we get from restoring the dump.

I agree it doesn't explain the failures.

I did experiment with the attached very-quick-n-dirty patch, which
should succeed in suppressing autovacuum in both the old and new
versions if I understand the code correctly (which I might well not).
It made no difference at all in the dump diffs ...

In 002_pg_upgrade.pl, I disabled autovacuum and restarted after the
regression run. In other words, in the old cluster, autovacuum did have
a chance to run, just not after the first dumpall.

Regards,
Jeff Davis

I wrote:

... I suspect that the problem is that
pg_dump is interpreting old-version stats in some way that doesn't
match up with what we get from restoring the dump.

Hmm. I forced my local BF installation to run a v17-to-HEAD upgrade
test, and it still failed, though seemingly with fewer diffs than
the buildfarm is reporting for older branches. (Diffs attached for
amusement's sake.) I don't believe we've made any definitional
changes in the contents of pg_statistic since v17, so whatever's
going on here seems a little subtler than I was hoping.

I wonder if it'd be a good idea to rearrange TestUpgradeXversion.pm
so that instead of testing upgrades from oldest prior version to
newest, it tested from newest to oldest? My thought here is that
the oldest cases are most likely to fail, and when they do, it'd
be valuable information to know which branches still work.

regards, tom lane

PS: this is with the no-autovacuum hack I posted before.

On Fri, 2025-02-21 at 21:57 -0500, Tom Lane wrote:

Hmm.  I forced my local BF installation to run a v17-to-HEAD upgrade
test, and it still failed, though seemingly with fewer diffs than
the buildfarm is reporting for older branches.  (Diffs attached for
amusement's sake.)  I don't believe we've made any definitional
changes in the contents of pg_statistic since v17, so whatever's
going on here seems a little subtler than I was hoping.

Also the non-buildfarm tests (src/bin/pg_upgrade/TESTING) are all
passing for versions 10+.

I wonder if it'd be a good idea to rearrange TestUpgradeXversion.pm
so that instead of testing upgrades from oldest prior version to
newest, it tested from newest to oldest?  My thought here is that
the oldest cases are most likely to fail, and when they do, it'd
be valuable information to know which branches still work.

That's a good idea and would be a big help.

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

In 002_pg_upgrade.pl, I disabled autovacuum and restarted after the
regression run. In other words, in the old cluster, autovacuum did have
a chance to run, just not after the first dumpall.

The hack I posted should prevent autovacuum from running either
before or after dumping, in either cluster.

However, it occurred to me to try forcing a HEAD-to-HEAD upgrade,
a case the buildfarm animals have not been able to reach.
And *it failed*!? (Diffs attached.) So that eliminates the
theory that this is a cross-version compatibility problem, or
at least that is not our only problem. It seems there is
something different between what TestUpgradeXversion.pm is doing
and what 002_pg_upgrade.pl is doing. No clue what, although it
does look like an additional round of analyze'ing has added more
stats than were there before.

regards, tom lane

On 2025-02-21 Fr 10:11 PM, Tom Lane wrote:

Jeff Davis <pgsql@j-davis.com> writes:

In 002_pg_upgrade.pl, I disabled autovacuum and restarted after the
regression run. In other words, in the old cluster, autovacuum did have
a chance to run, just not after the first dumpall.

The hack I posted should prevent autovacuum from running either
before or after dumping, in either cluster.

However, it occurred to me to try forcing a HEAD-to-HEAD upgrade,
a case the buildfarm animals have not been able to reach.
And *it failed*!? (Diffs attached.) So that eliminates the
theory that this is a cross-version compatibility problem, or
at least that is not our only problem. It seems there is
something different between what TestUpgradeXversion.pm is doing
and what 002_pg_upgrade.pl is doing. No clue what, although it
does look like an additional round of analyze'ing has added more
stats than were there before.

Yeah, I don't know.

Here's what I have so far:

. for HEAD/18 disable running the analyze script / vacuumdb --analyze.

. turn off autovacuum on the old and upgraded database.

. reverse the order of testing so we do newest first

What I'm thinking of doing is running all the eligible upgrades rather
than stopping on the first failure.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

On 2025-02-21 Fr 10:11 PM, Tom Lane wrote:

... It seems there is
something different between what TestUpgradeXversion.pm is doing
and what 002_pg_upgrade.pl is doing. No clue what, although it
does look like an additional round of analyze'ing has added more
stats than were there before.

Hah! Looking at the script with less bleary eyes, I see it does
this after pg_upgrade:

if (-e "$installdir/analyze_new_cluster.sh")
{
system( "cd $installdir && sh ./analyze_new_cluster.sh "
. qq{> "$upgrade_loc/$oversion-analyse.log" 2>&1 });
return if $?;
}
else
{
system( qq{"$installdir/bin/vacuumdb" --all --analyze-only }
. qq{> "$upgrade_loc/$oversion-analyse.log" 2>&1 });
return if $?;
}

So there's our extra round of ANALYZE right there. I diked out the
vacuumdb call and things are working much better. It seems to pass
for branches back through 9.3, and upgrade from 9.2 has only some
diffs for relallvisible (see attached). We still need to figure out
why that is, but a quick-n-dirty patch could just be to make the dump
comparison logic ignore relallvisible diffs.

We might want to make this vacuumdb invocation dependent on version,
but I could also see just removing it entirely.

Here's what I have so far:

. for HEAD/18 disable running the analyze script / vacuumdb --analyze.
. turn off autovacuum on the old and upgraded database.
. reverse the order of testing so we do newest first

Check.

What I'm thinking of doing is running all the eligible upgrades rather
than stopping on the first failure.

Seems like possibly wasted work --- I'd be content with
newest-to-oldest.

regards, tom lane

On 2025-02-22 Sa 1:34 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 2025-02-21 Fr 10:11 PM, Tom Lane wrote:

... It seems there is
something different between what TestUpgradeXversion.pm is doing
and what 002_pg_upgrade.pl is doing. No clue what, although it
does look like an additional round of analyze'ing has added more
stats than were there before.

Hah! Looking at the script with less bleary eyes, I see it does
this after pg_upgrade:

if (-e "$installdir/analyze_new_cluster.sh")
{
system( "cd $installdir && sh ./analyze_new_cluster.sh "
. qq{> "$upgrade_loc/$oversion-analyse.log" 2>&1 });
return if $?;
}
else
{
system( qq{"$installdir/bin/vacuumdb" --all --analyze-only }
. qq{> "$upgrade_loc/$oversion-analyse.log" 2>&1 });
return if $?;
}

So there's our extra round of ANALYZE right there. I diked out the
vacuumdb call and things are working much better. It seems to pass
for branches back through 9.3, and upgrade from 9.2 has only some
diffs for relallvisible (see attached). We still need to figure out
why that is, but a quick-n-dirty patch could just be to make the dump
comparison logic ignore relallvisible diffs.

We might want to make this vacuumdb invocation dependent on version,
but I could also see just removing it entirely.

Here's what I have so far:
. for HEAD/18 disable running the analyze script / vacuumdb --analyze.
. turn off autovacuum on the old and upgraded database.
. reverse the order of testing so we do newest first

Check.

What I'm thinking of doing is running all the eligible upgrades rather
than stopping on the first failure.

Seems like possibly wasted work --- I'd be content with
newest-to-oldest.

OK, went with that. crake is getting a failure at 9.6 like you saw with
9.2, but things are a whole lot better than they were.

I have updated drongo and fairywren with the new code too, so they
should also improve before long.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

I wrote:

So there's our extra round of ANALYZE right there. I diked out the
vacuumdb call and things are working much better. It seems to pass
for branches back through 9.3, and upgrade from 9.2 has only some
diffs for relallvisible (see attached).

I confess that when I wrote that, I hadn't actually tested every
single branch as an upgrade source. After seeing the latest result
from crake (which failed at 9.6), I went back and filled in the gaps.
I can now say that 9.2 and 9.6 fail, but every other branch works.
What's more, the diffs from 9.2 and 9.6 are identical, and match
what crake is showing for 9.6. That's just crazy --- I would be
unsurprised if a range of back releases were misbehaving in the same
way, but not two isolated branches.

Furthermore, it can't be a coincidence that the four tables we are
seeing relallvisible diffs for are exactly the four tables in the
regression database that have hash indexes.

But I'm baffled where to look beyond that. I could believe that
CREATE INDEX with a hash index misbehaves by changing the
relallvisible value even when we're doing a binary upgrade --- but
such a bug would be on the restoring side, so how would it be
sensitive to the source branch? I'm confused.

regards, tom lane

On Sat, 2025-02-22 at 20:15 -0500, Tom Lane wrote:

That's just crazy --- I would be
unsurprised if a range of back releases were misbehaving in the same
way, but not two isolated branches.

Furthermore, it can't be a coincidence that the four tables we are
seeing relallvisible diffs for are exactly the four tables in the
regression database that have hash indexes.

But I'm baffled where to look beyond that.  I could believe that
CREATE INDEX with a hash index misbehaves by changing the
relallvisible value even when we're doing a binary upgrade --- but
such a bug would be on the restoring side, so how would it be
sensitive to the source branch?  I'm confused.

It's also strange that copperhead is consistently failing on 12 with:

pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 4163; 0 0 STATISTICS DATA "vcharidx" (no
owner)
pg_restore: error: could not execute query: ERROR: column "text" of
relation "vcharidx" does not exist

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=copperhead&amp;dt=2025-02-22%2009%3A10%3A36&amp;stg=xversion-upgrade-REL_12_STABLE-HEAD

I was puzzling through whether the attribute name uniqueness logic was
doing something strange, but it's very simple. And the table and index
should both be locked at the point of the syscache lookup.

Regards,
Jeff Davis

I wrote:

Furthermore, it can't be a coincidence that the four tables we are
seeing relallvisible diffs for are exactly the four tables in the
regression database that have hash indexes.

Oh! If the source version is <= 9.6, old_9_6_invalidate_hash_indexes()
generates a script "reindex_hash.sql" to reindex all hash indexes.
And TestUpgradeXversion faithfully executes that, *before* making
the new-database comparison dump.

I added some instrumentation and confirmed that these tables'
relallvisible values match between the pre-dump state on the old
database and the immediately-post-upgrade state on the new database.
It's definitely reindex_hash.sql that's changing them. This doesn't
in itself explain why 9.3-9.5 don't show the problem, but I noticed
something interesting: it's the pre-dump state that is out of line
in 9.2 and 9.6. All the other cases show relallvisible that's a
couple pages less than relpages, but in those two branches we
start from a state that claims these rels are fully all-visible,
and then seemingly REINDEX discovers that that's not so.
What I'm guessing is that the variance is due to changes in
vacuum/analyze's heuristics for updating pg_class.relallvisible
after a partial scan of the table.

Anyway, we know where the culprit is, and I'm not sure that
explaining the differences in behavior of long-dead branches
is an exciting use of time.

Question is, what to do about this? We probably want to continue to
test that reindex_hash.sql does something sane, so just deleting that
step won't do. I experimented with moving it from immediately before
the pg_dumpall of the new installation to immediately after, but that
didn't work at all: the indexes are marked invalid and so pg_dump just
doesn't dump them, so we still end up with a diff in the dump output.

I'm not really seeing a better answer than hacking the comparison
rules to ignore the relallvisible difference for these tables.
That's darn ugly, and I suspect the implementation will be messy,
but do we have another way?

regards, tom lane

Jeff Davis <pgsql@j-davis.com> writes:

It's also strange that copperhead is consistently failing on 12 with:

pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 4163; 0 0 STATISTICS DATA "vcharidx" (no
owner)
pg_restore: error: could not execute query: ERROR: column "text" of
relation "vcharidx" does not exist

Ugh. I see what is happening, and it's going to be problematic to
fix: our heuristics for assigning names to index expression columns
are not very consistent/stable. It didn't matter up to now, but
this patch assumes that it can reference index columns by name.
The index in question is made in btree_gist's tests:

CREATE INDEX vcharidx ON vchartmp USING GIST ( text(a) );

The index column will be given the name "text". However, it
dumps as

CREATE INDEX vcharidx ON public.vchartmp USING gist (((a)::text));

and when *that* gets loaded, the index column is given the name
"a", because FigureColname treats function-like constructs
differently from cast-like constructs. Then
pg_restore_attribute_stats unsurprisingly fails. I think the
reason that Andrew and I aren't seeing that is that we are
using machines that are fast enough to shut down the DB before
autovacuum gets around to populating some stats for this
expression index.

We have dealt with some similar issues in the past, and the
solution was to allow index columns to be referenced by
column number not name. (ALTER INDEX ... ALTER COLUMN ...
SET STATISTICS does that, not sure if there are other places.)
Recommend adopting the same solution here.

regards, tom lane

On 2025-02-22 Sa 9:20 PM, Tom Lane wrote:

I wrote:

Furthermore, it can't be a coincidence that the four tables we are
seeing relallvisible diffs for are exactly the four tables in the
regression database that have hash indexes.

Oh! If the source version is <= 9.6, old_9_6_invalidate_hash_indexes()
generates a script "reindex_hash.sql" to reindex all hash indexes.
And TestUpgradeXversion faithfully executes that, *before* making
the new-database comparison dump.

I added some instrumentation and confirmed that these tables'
relallvisible values match between the pre-dump state on the old
database and the immediately-post-upgrade state on the new database.
It's definitely reindex_hash.sql that's changing them. This doesn't
in itself explain why 9.3-9.5 don't show the problem, but I noticed
something interesting: it's the pre-dump state that is out of line
in 9.2 and 9.6. All the other cases show relallvisible that's a
couple pages less than relpages, but in those two branches we
start from a state that claims these rels are fully all-visible,
and then seemingly REINDEX discovers that that's not so.
What I'm guessing is that the variance is due to changes in
vacuum/analyze's heuristics for updating pg_class.relallvisible
after a partial scan of the table.

Anyway, we know where the culprit is, and I'm not sure that
explaining the differences in behavior of long-dead branches
is an exciting use of time.

Question is, what to do about this? We probably want to continue to
test that reindex_hash.sql does something sane, so just deleting that
step won't do. I experimented with moving it from immediately before
the pg_dumpall of the new installation to immediately after, but that
didn't work at all: the indexes are marked invalid and so pg_dump just
doesn't dump them, so we still end up with a diff in the dump output.

I'm not really seeing a better answer than hacking the comparison
rules to ignore the relallvisible difference for these tables.
That's darn ugly, and I suspect the implementation will be messy,
but do we have another way?

Having slept on it I can't see anything better. It's only for very old
branches, and nothing is really going wrong here, so ignoring the
difference seems quite reasonable.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

On 2025-02-22 Sa 9:20 PM, Tom Lane wrote:

I'm not really seeing a better answer than hacking the comparison
rules to ignore the relallvisible difference for these tables.
That's darn ugly, and I suspect the implementation will be messy,
but do we have another way?

Having slept on it I can't see anything better. It's only for very old
branches, and nothing is really going wrong here, so ignoring the
difference seems quite reasonable.

Agreed, and done.

regards, tom lane

On Sat, 2025-02-22 at 21:48 -0500, Tom Lane wrote:

CREATE INDEX vcharidx ON vchartmp USING GIST ( text(a) );

The index column will be given the name "text".  However, it
dumps as

CREATE INDEX vcharidx ON public.vchartmp USING gist (((a)::text));

Thank you! I dismissed the naming issue too early, going down the path
of something related to GiST.

and when *that* gets loaded, the index column is given the name
"a", because FigureColname treats function-like constructs
differently from cast-like constructs.

Ugh.

We have dealt with some similar issues in the past, and the
solution was to allow index columns to be referenced by
column number not name.  (ALTER INDEX ... ALTER COLUMN ...
SET STATISTICS does that, not sure if there are other places.)
Recommend adopting the same solution here.

I'll submit a patch to the -hackers thread. There are a few minor
choices here that might get some discussion.

Regards,
Jeff Davis

I was running "make check-world" this morning on a machine that has an
old version of perl, 5.16, and the check-world was hung on
/usr/bin/perl t/002_pg_upgrade.pl.

I never saw "make check-world" hang on this old version of perl,
but I bisected the regression to this commit fc0d0ce978752493,
the subject of this thread.

This is the pstack of the stuck perl process.

#0 0x00007f5964b4b1dc in Perl_fbm_instr () from
/usr/lib64/perl5/CORE/libperl.so
#1 0x00007f5964bd146f in Perl_re_intuit_start () from
/usr/lib64/perl5/CORE/libperl.so
#2 0x00007f5964bd3400 in Perl_regexec_flags () from
/usr/lib64/perl5/CORE/libperl.so
#3 0x00007f5964b6d3fc in Perl_pp_subst () from /usr/lib64/perl5/CORE/libperl.so
#4 0x00007f5964b67e1e in Perl_runops_standard () from
/usr/lib64/perl5/CORE/libperl.so
#5 0x00007f5964b07463 in perl_run () from /usr/lib64/perl5/CORE/libperl.so
#6 0x0000000000400cd9 in main ()

and specifically, the process hangs with this specific change.

-       $dump =~ s ['version', '\d+'::integer,]
-               ['version', '000000'::integer,]mg;
+       $dump =~ s {(^\s+'version',) '\d+'::integer,$}
+               {$1 '000000'::integer,}mg;

I repro'd on several machines with 5.16, and then issue went away once
I upgraded to a more recent version of perl; at least with 5.25, the
issue does
not occur.

I don't know if we need to do anything here, except for making sure we are
building with the most recent version of perl, but I wanted to mention this
here for awareness.

--

Sami