Don't rewrite _map files on the save_principal_key redo #217
Conversation
|
I am a bit uncertain about this since I feel it goes against the spirit of what a redo is supposed to do. Like wouldn't this cause issues when replayed on the replica when we have WAL encryption enabled on it? I need to think on this a bit. |
We create a new WAL key during the extension init, which happens before the redo. This means that in case of a crash, pg_tde_save_principal_key_redo was rewriting a WAL _map file and destroying a newly created key. Since we emit an XLog record after the key was successfully written to the file (the file was created), we can safely assume that we should not change the file if it exists.
dAdAbird
force-pushed
the
dont_rewrite_map
branch
from
2aa1c21 to
aad61b5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❌ Your project status has failed because the head coverage (78.14%) 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 #217 +/- ##
=====================================================
+ Coverage 77.14% 78.14% +0.99%
=====================================================
Files 22 22
Lines 2490 2489 -1
Branches 395 394 -1
=====================================================
+ Hits 1921 1945 +24
+ Misses 490 466 -24
+ Partials 79 78 -1
🚀 New features to boost your workflow:
|
dAdAbird
force-pushed
the
dont_rewrite_map
branch
from
aad61b5 to
24b4832
Compare
| $node->kill9; | ||
| PGTDE::append_to_result_file("-- server start"); | ||
| PGTDE::append_to_result_file( | ||
| "-- check the recovery redo and the fix in PR #217 ('Failed to verify principal key header...' error)" |
There was a problem hiding this comment.
I would prefer not to have a reference to a PR here but rather an explanation for what we are actually testing.
|
|
||
| my $node = PostgreSQL::Test::Cluster->new('main'); | ||
| $node->init; | ||
| $node->append_conf('postgresql.conf', "shared_preload_libraries = 'pg_tde'"); |
There was a problem hiding this comment.
To do a recovery tests shouldn't you set the checkpoint timeout to like 60 minutes to make it clear? Or is that overkill?
There was a problem hiding this comment.
yes, it makes sense to be sure
Tests to trigger redo routines after the server crash. It mostly checks invariants when different redo functions might rewrite WAL keys created on the init stage. For PG-1539, PG-1541, PG-1468, PG-1413
dAdAbird
force-pushed
the
dont_rewrite_map
branch
from
24b4832 to
014e977
Compare
We create a new WAL key during the extension init, which happens before the redo. This means that in case of a crash,
pg_tde_save_principal_key_redo was rewriting a WAL _map file and destroying a newly created key.
Since we emit an XLog record after the key was successfully written to the file (the file was created), we can safely assume that we should not change the file if it exists.