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

Add test for repair table script #2065

Closed
wants to merge 1 commit into from

Conversation

mkindahl
Copy link
Contributor

@mkindahl mkindahl commented Jul 7, 2020

This commit copies the repair query into a separate file and turns
it into a procedure. This will allow the repair query to be tested
separately using pg_regress. It also adds a test script for the
repair procedure and will remove different sorts of dimensions and see
that they can be properly repaired.

It also contain a fix for the timestamp parsing query that prevented it
from handling all timestamp formats and a fix so that if many chunks refer to the same dimension slice, it might generate duplicate rows.

@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #2065 into master will increase coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2065      +/-   ##
==========================================
+ Coverage   90.01%   90.24%   +0.23%     
==========================================
  Files         210      211       +1     
  Lines       33255    33643     +388     
==========================================
+ Hits        29933    30361     +428     
+ Misses       3322     3282      -40     
Impacted Files Coverage Δ
tsl/src/fdw/shippable.c 82.85% <0.00%> (-11.43%) ⬇️
tsl/src/fdw/data_node_scan_exec.c 82.69% <0.00%> (-1.63%) ⬇️
src/loader/bgw_message_queue.c 84.51% <0.00%> (-1.02%) ⬇️
tsl/src/debug.c 39.89% <0.00%> (-0.33%) ⬇️
tsl/src/compression/create.c 95.16% <0.00%> (-0.26%) ⬇️
src/plan_agg_bookend.c 92.34% <0.00%> (-0.26%) ⬇️
src/hypertable.c 90.84% <0.00%> (-0.11%) ⬇️
src/chunk.c 95.29% <0.00%> (-0.05%) ⬇️
src/compat.h 100.00% <0.00%> (ø)
tsl/test/src/remote/stmt_params.c 100.00% <0.00%> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41c89b5...f81ce58. Read the comment docs.

@mkindahl mkindahl force-pushed the repair_script_tests branch 2 times, most recently from 8940810 to 51aea09 Compare July 7, 2020 12:20
@mkindahl mkindahl marked this pull request as ready for review July 7, 2020 12:20
@mkindahl mkindahl requested a review from a team as a code owner July 7, 2020 12:20
@mkindahl mkindahl requested review from k-rus, svenklemm, gayyappan and erimatnor and removed request for a team and k-rus July 7, 2020 12:20
@mkindahl
Copy link
Contributor Author

mkindahl commented Jul 7, 2020

There is one unfortunate situation in that we need to have two copies of the repair script. I fear that might result in mistakes when one script is updated but not the other. Include commands do not work with the update scripts, but we could have version-specific pre- and post-scripts and use these to generate the actual scripts in a similar way to have latest-dev.sql is used.

@mkindahl mkindahl marked this pull request as draft July 7, 2020 13:28
@mkindahl mkindahl added the upgrade Issue is related to upgrading the extension or the PostgreSQL version. label Jul 7, 2020
@mkindahl mkindahl self-assigned this Jul 7, 2020
This commit copies out the repair query into a separate file and turns
it into a procedure. This will allow the repair query to be tested
separately using `pg_regress`. It also adds a test script for the
repair procedure and will remove different sorts of dimensions and see
that they can be properly repaired.

It also contain a fix for the timestamp parsing query that prevented it
from handling all timestamp formats and a fix so that if many chunks
refer to the same dimension slice, it might generate duplicate rows.
Copy link
Contributor

@erimatnor erimatnor left a comment

Choose a reason for hiding this comment

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

It doesn't seem right to me that the update script is tested in a regression test. Why isn't it part of the update test?

The odd thing is that this is a script that needs to run only once on upgrades of specific versions of the database, so it doesn't seem right that this has to run as part of our standard regression suite. It also doesn't test that the update script actually works during an update to a new version of the extension.

@mkindahl
Copy link
Contributor Author

mkindahl commented Jul 8, 2020

It doesn't seem right to me that the update script is tested in a regression test. Why isn't it part of the update test?

The odd thing is that this is a script that needs to run only once on upgrades of specific versions of the database, so it doesn't seem right that this has to run as part of our standard regression suite. It also doesn't test that the update script actually works during an update to a new version of the extension.

Not sure I follow. Pg_regress will start a clean server and test that the repair script will repair tables, so it's perfect for testing the repair script. It is not a complete test that the update script works, it just test that it can repair the dimension slice table.

We already have update tests, but they cannot catch all kind of problems. They just test that a correct installation can be updated.

@svenklemm
Copy link
Member

I agree with @erimatnor the script is run when upgrading from 1.7.1 to 1.7.2 so there is no need in maintaining it and adjusting it to catalog changes because it needs to run in the 1.7.1 environment therefore regression tests are not the right place.

@mkindahl
Copy link
Contributor Author

mkindahl commented Sep 8, 2020

Closing.

@mkindahl mkindahl closed this Sep 8, 2020
@mkindahl mkindahl deleted the repair_script_tests branch September 8, 2020 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upgrade Issue is related to upgrading the extension or the PostgreSQL version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants