Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce size of shared test tables #7591

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkindahl
Copy link
Contributor

@mkindahl mkindahl commented Jan 14, 2025

In order to reduce the runtime of the shared tests we reduce the number of rows in the tables used for the shared tests by increasing the interval between the time samples. This reduce the number of rows without reducing the upper and lower ranges. This changes the number of rows in the plans, so we have to update the tests accordingly.

Note: there is a lot of test changes, but they are just reducing the number of rows scanned. Important is that the new plans do not end up with zero rows where there should be some. Note that in some cases, there will be zero rows scanned for an Append node, but the corresponding ChunkAppend node is then not scanned at all, so that matches.

Disable-check: force-changelog-file

In order to reduce the runtime of the shared tests we reduce the number
of rows in the tables used for the shared tests by increasing the
interval between the time samples. This reduce the number of rows
without reducing the upper and lower ranges. This changes the number of
rows in the plans, so we have to update the tests accordingly.
@mkindahl mkindahl force-pushed the reduce-shared-test-size branch from c165de8 to c9c4eca Compare January 14, 2025 11:54
@mkindahl mkindahl marked this pull request as ready for review January 14, 2025 12:03
Copy link

@antekresic, @erimatnor: please review this pull request.

Powered by pull-review

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.29%. Comparing base (59f50f2) to head (c9c4eca).
Report is 692 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7591      +/-   ##
==========================================
+ Coverage   80.06%   82.29%   +2.22%     
==========================================
  Files         190      238      +48     
  Lines       37181    43745    +6564     
  Branches     9450    10981    +1531     
==========================================
+ Hits        29770    35999    +6229     
- Misses       2997     3401     +404     
+ Partials     4414     4345      -69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think we should adjust the global shared database. This set is used for many tests and only generated once per ci run and intentionally a bit bigger. If this set is too large for this specific test we should create a test local small table for these tests. Keep in mind though that the shared set also has other attributes e.g. chunks with different physical layouts,

@mkindahl
Copy link
Contributor Author

I dont think we should adjust the global shared database. This set is used for many tests and only generated once per ci run and intentionally a bit bigger. If this set is too large for this specific test we should create a test local small table for these tests. Keep in mind though that the shared set also has other attributes e.g. chunks with different physical layouts,

I am not sure we need a bigger size. The changes involves "thinning out" the samples so that there are fewer samples but the ranges are the same in all tests, which should keep the "physical layout" of the chunks (the chunk time intervals and the number of chunks). Looking at the changes, I see mostly changes in the rows count, but there are some changes from Merge Join to Nested Loop Joins as well. There is one case where a timestamp was an even multiple of 2, which I changed to an even multiple of 10, and it seems like the result is the same.

@akuzm
Copy link
Member

akuzm commented Jan 14, 2025

In my previous attempt to make the shared tests less flaky, I actually had to increase the table sizes to get meaningful plans: #6550
Not sure if we should touch the shared tables because it influences just too many things, maybe we could focus on individual very slow tests instead?

@akuzm
Copy link
Member

akuzm commented Jan 14, 2025

ci_stats=> select avg(test_duration/1000)::int seconds, test_name from test natural join job where job_name like 'PG16%Debug ubuntu%' group by 2 order by 1 desc limit 15;
 seconds │          test_name           
─────────┼──────────────────────────────
      51 │ compression_ddl
      43 │ bgw_job_stat_history
      33 │ pg_join
      30 │ bgw_custom
      27 │ vector_agg_grouping
      23 │ transparent_decompression-16
      20 │ compression_indexscan
      20 │ cagg_compression
      20 │ bgw_job_stat_history_errors
      20 │ insert_memory_usage
      18 │ plan_hashagg-16
      18 │ compression_merge
      15 │ deadlock_dropchunks_select
      15 │ parallel-16
      15 │ vector_agg_functions
(15 rows)

For the reference, here are the 15 slowest tests. Only cagg_compression is shared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants