-
Notifications
You must be signed in to change notification settings - Fork 11
PG-1870 Run postgresql TAP suite with pg_tde enabled #535
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
PG-1870 Run postgresql TAP suite with pg_tde enabled #535
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, especially with Zsolt's proposed changes if they can be implemented reaosnably which generates the files and copies them.
3aac647
to
0e93481
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (82.22%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## TDE_REL_17_STABLE #535 +/- ##
==================================================
Coverage 82.22% 82.22%
==================================================
Files 25 25
Lines 3246 3246
Branches 512 512
==================================================
Hits 2669 2669
Misses 465 465
Partials 112 112
🚀 New features to boost your workflow:
|
615c60f
to
4ccb15e
Compare
b3b7e89
to
415cb8d
Compare
4ccb15e
to
cdd6aa1
Compare
cdd6aa1
to
3647e68
Compare
3647e68
to
512a3a1
Compare
512a3a1
to
82e6728
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice changes! Nice also to see some use of single user mode.
my ($self) = @_; | ||
|
||
my $tde_template_dir = $ENV{TDE_TEMPLATE_DIR} | ||
if defined($ENV{TDE_TEMPLATE_DIR}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we have not implemented it for make? Just lack of time? The reason should either be in the commit message or a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just didn't care enough about make to do it. Which is probably not a great reason. I am honestly on the fence if we should do it for meson either. Might not be worth the extra complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't say I care either way but it should be explained in the commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you emasured the speedup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a hard time doing any type of even preliminary performance testing on my laptop right now because the stupid s1-agent
eats so much CPU and it's quite random.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what more to say about i in a commit message. I already mention that it does it for meson but not for make. Should I just remove it from meson if it's confusing?
Or do you mean that I should mention that there is no technical reason for not doing it in make, just that this commit doesn't do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do you mean that I should mention that there is no technical reason for not doing it in make, just that this commit doesn't do that?
Yes, that is what I meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do you mean that I should mention that there is no technical reason for not doing it in make, just that this commit doesn't do that?
Yes, that is what I meant.
Pushed new version of the branch that does the meson setup in a separate commit with a more verbose commit message.
8d7f347
to
c757bde
Compare
This makes sure pg_tde is loaded and keys are setup when running postgresql TAP suite. No TDE features are enabled at this point. Single user mode is used to generate a template of pg_tde setup files which are then copied to each created cluster's data directory.
This enables WAL encryption by default when the TAP tests are run with TDE_MODE=1. Use TDE_MODE_WAL=0 to disable wal encryption while still having pg_tde enabled.
This enables table encryption by default in TAP tests when TDE_MODE=1. Use TDE_MODE_SMGR=0 to turn off table encryption when running with pg_tde loaded. The setup for running regress with tde turned on has been slightly modified to match what is done for TAP tests to let tests that run the regress suite under TAP work.
This is currently only added for meson, but could also be added for make. This has to be setup before the actual TAP tests are run as they are run in parallel and as such would all try to setup the template at the same time if we let them use the same folder for it without it being pre-generated. This seems to shorten the test suite run-time by ~25% on my laptop, so it seems worth doing.
c757bde
to
d976449
Compare
This enables wal encryption and table encryption by default when running postgres TAP suite with
TDE_MODE=1
.A few tests had to be skipped for various reasons, and some of these should most likely be looked into as they might be actual bugs or test setup issues we can fix.
Use
TDE_MODE=1
to enable pg_tde with both wal and relation encryption. UseTDE_MODE_WAL=0
to disable wal encryption andTDE_MODE_SMGR=0
to disable relation encryption.For example to run the postgres suite with WAL encryption turned on, but not SMGR:
TDE_MODE=1 TDE_MODE_SMGR=0 meson test --no-suite pg_tde
There is also
TDE_MODE_NOSKIP=1
which will force all tests to run. This is useful when investigating the test failures that made it necessary to skip some tests.