[#26240] YSQL: Import 'Transfer statistics during pg_upgrade'

[hari90]

Import upstream 17 postgres commit from master related to the feature Transfer statistics during pg_upgrade.
Master commits:
f0d0083 | pg_dump: Fix query for gathering attribute stats on older versions. | 2025-04-04 21:05:30 -0500
9c02e3a | pg_dump: Retrieve attribute statistics in batches. | 2025-04-04 14:51:08 -0500
7d5c83b | pg_dump: Reduce memory usage of dumps with statistics. | 2025-04-04 14:51:08 -0500
a0a4601 | Matview statistics depend on matview data. | 2025-03-28 16:12:55 -0700
bde2fb7 | Add pg_dump --with-{schema|data|statistics} options. | 2025-03-25 17:36:38 -0700
650ab8a | Stats: use schemaname/relname instead of regclass. | 2025-03-25 11:16:06 -0700
1852aea | Don't convert to and from floats in pg_dump. | 2025-03-08 11:25:36 -0800
d611f8b | CREATE INDEX: don't update table stats if autovacuum=off. | 2025-03-06 19:36:34 2025 -0800
1d33de9 | Organize and deduplicate statistics import tests. | 2025-03-06 00:19:22 2025 -0800
424eded | Adjust pg_dump tag for relation stats. | 2025-02-27 20:42:12 -0800
40e27d0 | Use attnum to identify index columns in pg_restore_attribute_stats(). | 2025-02-26 16:36:20 -0500
6ee3b91 | pg_dump: prepare attribute stats query. | 2025-02-25 19:52:11 -0800
8f42718 | Avoid unnecessary relation stats query in pg_dump. | 2025-02-25 19:51:45 -0800
fc0d0ce | Ignore hash's relallvisible when checking pg_upgrade from pre-v10. | 2025-02-23 14:16:26 2025 -0500
ab84d0f | Trial fix for old cross-version upgrades. | Thu 2025-02-20 10:21:24 2025 -0800
1fd1bd8 | Transfer statistics during pg_upgrade. | 2025-02-20 01:29:06 -0800
41a2844 | Clean up some pg_dump tests | 2023-10-18 08:03:39 2023 +0200

Phorge Revision: D43002

Conflict resolution:

Individual commits contain the conflict resolution messages. analyze_cherry_pick.py was used to compute resolution messages.

Changes from cherry-picked commit
< Changes from resolved commit

Testing:

./configure  --enable-tap-tests
make clean
make
make check
cd src/bin/pg_dump
make PROVE_TESTS=t/002_pg_dump.pl check

[timothy-e]

How did you identify these 13 commits? How do we know there's not a 14th that fixes a sneaky bug? That's probably worth specifying in the description.

Otherwise, the commits look good to me. I didn't diff them to the original PG commits though

[timothy-e]

One nit: you could use markdown table syntax to format the Master commits: section a bit better

[jasonyb]

For each conflict, please not only reference the filename but also another marker within that file, such as function name and/or code line. Line number is not reliable since there are two sides (which side are you talking about for line number?) and these can change in case someone else does an import before you.

1fd1bd8:

• pg_dump.sgml
  • No incoming changes accepted
    • This is not true. You take some patches and ignore some patches. If you are meaning that, for the merge conflict you got, you don't take incoming changes, then say that more specifically (and it would be better to take upstream's changes instead in most cases). Or perhaps more easily, resolve the conflict so you don't have to describe this.
• pg_backup_archiver.c
  • 983: Removed txnCount related code that does not exist in pg15
    • Are you talking about the upstream change that adds REQ_STATS? It's not super clear. "txnCount" is on a context line on the upstream side's patch.
• pg_dump_sort.c
  • (none)
    • lack of "+StaticAssertDecl(lengthof(dbObjectTypePriority) == NUM_DUMPABLE_OBJECT_TYPES," change is not explained.
• src/bin/pg_dump/t/002_pg_dump.pl
  • (none)
    • lack of explanation for odd differences in this file
• other minor changes that must have been conflicts are not listed. not a blocker

Stopping review at this point.

[jasonyb]

40e27d0 resolution is very unclear.

stats_import.sql
All incoming changes accepted

What does that mean? I see a bunch of extra content brought in that does not exist in 40e27d0. Take for instance +-- error: argument name is NULL. That does not exist in git show 40e27d04b4f643cfb78af8db42a1f2e700ec9876

Also, the commit author is screwed up. It says hari when it should be 2025-02-26T16:36:11-05:00 Tom Lane <tgl@sss.pgh.pa.us>

[hari90]

How did you identify these 13 commits? How do we know there's not a 14th that fixes a sneaky bug? That's probably worth specifying in the description.

I looked at the commit history of stats_import.sql, pg_dump.c and pg_dump_all.c. I am also tracking the PG mailing list topic.

[jasonyb]

please use -x when cherry-picking

[jasonyb]

Again, reviewed only the first commit eb53a12 and found issues:

• pg_dump.sql: there are missing patches that are not explained
• pg_backup_archiver.c: REQ_STATS explanation is now completely gone.
• pg_dump_sort.c: lack of +StaticAssertDecl(lengthof(dbObjectTypePriority) == NUM_DUMPABLE_OBJECT_TYPES, is still not explained. In fact, it shouldn't be missing. It should be fixed, and YB change should be transfered to pg_dump.h.
• 002_pg_dump.pl:
  • + statistics_only => { change is uncalled for.
  • upstream's + no_schema => 1, patch on @@ -1598,6 +1631,7 @@ my %tests = ( appears missing
  • - binary_upgrade => 1, changes are uncalled for. "Test cases ... were also updated so that they match imcoming changes and for the tests to pass" is vague reasoning.

[jasonyb]

Looked at the (new) first commit:

Removal of only_dump_measurement => 1, in section Check ordering of a matview that depends on a primary key (aka @@ -2354,7 +2352,6 @@ my %tests = () was never explained. It looks like it was trying to follow the original patch's change on @@ -3210,7 +3207,6 @@ my %tests = (, but that is on completely different 'COPY test_compression_method' => {.

Resolution: Keeping the blob terminology as per pg15. Skipping the runs that are not available in pg15.: you're not "keeping" blob terminology since these are concerning deleted lines. More importantly, section_post_data => 1, lines should not be changed: leave them spaced like section_post_data => 1, to reduce diff. Minor improvement is to reference the PG commit that redid the spacing.

[jasonyb]

Reviewed 5920cb4

---

41a2844 Clean up some pg_dump tests

• For section COPY test_compression_method, you need an explanation similar to Cause: Missing 00d9dcf5bebbb355152a60f0e2120cdf7f9e7ddd: 'LO create (with no data)' test is not present in pg15.

• You should add an explanation for section ALTER INDEX ... ATTACH PARTITION.

• This was uncalled for:

  -   },
  +   unlike => {},
  

---

1fd1bd8 Transfer statistics during pg_upgrade.

• Is it okay to add case 18 after case 11? Should it be mapped down to case 12?

• Section If it has a statistics component that we want, then process that has double newline for no reason. Make it single.

• case 5:: is it safe to skip cases 2-4? Or does this need to be remapped to case 2?

---

40e27d0 Use attnum to identify index columns in pg_restore_attribute_stats().

Previous cherry-pick ae8c8b3080e7cab45a991ea772602ba51c794ec2 imported the latest version of this file already.

• You are referencing a yugabyte/yugabyte-db commit. Please refer to eea554f instead.

Cause: Missing 374c7a2: 'CREATE TABLE regress_pg_dump_table_part' test case was added as part of a new feature.

• That's an invalid excuse. The code change is adjacent to CREATE TABLE regress_pg_dump_table_part. Do the minor code change.

Previous version of the test file was invalid. The contents of the entire file have been updated to match the sate as of 40e27d0

• Please elaborate on what was "invalid".

• diff <(git show cd9ae07210c7b255c9e2e80550e8b5bd3350f6a7:src/test/regress/expected/stats_import.out) <(git show 40e27d04b4f643cfb78af8db42a1f2e700ec9876:src/test/regress/expected/stats_import.out) shows differences. This does not match your claim "the entire file have been updated to match". You show one diff afterwards, but please use English to say that you made further modifications after the entire update. There is also another diff regarding the output f that is not explained.

• s/sate/state/

---

650ab8a Stats: use schemaname/relname instead of regclass.

Differences found in documentation file

• so what action did you take?

catversion is not updated in pg15

• Please do not refer to "pg15". Use "yb-pg15". Ditto elsewhere

---

9c02e3a pg_dump: Retrieve attribute statistics in batches.

Cause: Missing 4694aed: relallfrozen column was added.
Resolution: Simple merge with skipping relallfrozen column.

• This conflict is concerning relallfrozen code adjacently added. You don't skip relallfrozen code: those were context lines.

---

5920cb4 Import 'Transfer statistics during pg_upgrade'

• This is a merge commit that should not exist.

*   5920cb4164f Import 'Transfer statistics during pg_upgrade' - Hari Krishna Sunder (BackupStats)
|\
| * ff9073a5202 pg_dump: Fix query for gathering attribute stats on older versions. - Nathan Bossart
| * c1fe52f64bb pg_dump: Retrieve attribute statistics in batches. - Nathan Bossart
| * ae866db06df pg_dump: Reduce memory usage of dumps with statistics. - Nathan Bossart
| * 509c84a6cfa Matview statistics depend on matview data. - Jeff Davis
| * a033888cd0c Add pg_dump --with-{schema|data|statistics} options. - Jeff Davis
| * da5faf1fdf7 Stats: use schemaname/relname instead of regclass. - Jeff Davis
| * 167ddc0bc98 Don't convert to and from floats in pg_dump. - Jeff Davis
| * 8ef62be4cb8 CREATE INDEX: don't update table stats if autovacuum=off. - Jeff Davis
| * 28fc7225dee Organize and deduplicate statistics import tests. - Jeff Davis
| * 2f834403150 Adjust pg_dump tag for relation stats. - Jeff Davis
| * cd9ae07210c Use attnum to identify index columns in pg_restore_attribute_stats(). - Tom Lane
| * d3116245b04 pg_dump: prepare attribute stats query. - Jeff Davis
| * b3aa4014557 Avoid unnecessary relation stats query in pg_dump. - Jeff Davis
| * 47f47fce245 Ignore hash's relallvisible when checking pg_upgrade from pre-v10. - Tom Lane
| * 2c18d3853fa Trial fix for old cross-version upgrades. - Jeff Davis
| * a236d972da3 Transfer statistics during pg_upgrade. - Jeff Davis
| * 676c7b8ebe5 Clean up some pg_dump tests - Peter Eisentraut
|/
*   12398eddbd5 Merge tag 'REL_15_12' into yb-pg15 - Jason Kim (yb-pg15)

Last commit should be ff9073a.

---

• Note: In your merge resolution notes, all your < and > are not clear on which side represents the original patch and which side represents your patch.

[hari90]

• Is it okay to add case 18 after case 11? Should it be mapped down to case 12?
• case 5:: is it safe to skip cases 2-4? Or does this need to be remapped to case 2?

Not sure which one you mean by case 18. For case 5 yes its safe to come after 1, and is preferred since in future we might selective backport 2-4.

[jasonyb]

Don't forget to push directly, not use GitHub interface to merge.

Clean up some pg_dump tests

• I previously said "You should add an explanation for section ALTER INDEX ... ATTACH PARTITION.", but this still doesn't appear to be done. If you had done it before, then I would have realized my misread of

  -   },
  +   unlike => {},
  

  Now that I look at it, it makes sense: all the items inside unlike were removed. Your new version removes the unlike entirely. Both versions were merged in a nontrivial way: the most trivial way would be to keep unlike => { and } as two separate lines. You can change the code to that, in which case an explanation is not needed. Or if you keep the current code, I suggest writing that you removed the entire unlike because it's completely empty (unlike the original patch which still has exclude_measurement).