PG-1447 Test and fix support for template databases #270
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your project status has failed because the head coverage (79.03%) 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 #270 +/- ##
=====================================================
+ Coverage 78.93% 79.03% +0.10%
=====================================================
Files 22 22
Lines 2464 2533 +69
Branches 385 397 +12
=====================================================
+ Hits 1945 2002 +57
- Misses 444 452 +8
- Partials 75 79 +4
🚀 New features to boost your workflow:
|
jeltz
force-pushed
the
tde/template-dbs
branch
7 times, most recently
from
7ea8a82 to
c3c8917
Compare
| (1 row) | ||
|
|
||
| CREATE DATABASE new_db TEMPLATE template_db; | ||
| ERROR: database "new_db" already exists |
There was a problem hiding this comment.
Something seems to be wrong in this testcase - this database already exists.
The description also said it only works if we have a default key, but we didn't have one during the previous create db.
There was a problem hiding this comment.
This seems possibly like a bug unrelated to the PR. And since it only happens when when TDE is enabled for the whole test suite it will not directly be an issue now that that has been changed but I think we should take a look and try to figure out if I accidentally found a bug here.
There was a problem hiding this comment.
No, the bug was in my test script. Doh. The issue is that we cannot delete the current default principal key. These tests used to run before the default key provider tests.
There was a problem hiding this comment.
Fixed by adding a comment and commenting out the code.
jeltz
force-pushed
the
tde/template-dbs
branch
4 times, most recently
from
d2b6a47 to
33509c4
Compare
contrib/pg_tde/Makefile
Outdated
| @@ -8,7 +8,7 @@ DATA = pg_tde--1.0-rc.sql | |||
| # Since meson supports skipping test suites this is a make only feature | |||
| ifndef TDE_MODE | |||
| REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_tde/pg_tde.conf | |||
| # default_principal_key needs to run after key_provider. | |||
| # default_principal_key needs and create_database to run after key_provider. | |||
There was a problem hiding this comment.
This sentence makes no sense to me, but looking at the code it seems like you want to say that default_principal_key needs to run after key_provider, and create_database needs to run after default_principal_key?
There was a problem hiding this comment.
Yes, I was lazy when writing it. Should fix it.
There was a problem hiding this comment.
The issue is that there is currently no way to delete default principal keys so these tests becom order dependent.
Using a database with encrypted relations as template works works with the WAL_LOG strateg as long as you have configured a default principal key. It is a bit hackish but works and since encrypted objects in a template database is likely to be an uncommon use case this is fine for now. The test file is put last in the test run order since there is still no good way to clean up default principal keys.
The FILE_COPY strategy of CREATE DATABASE does not use the SMGR so it cannot properly copy and re-encrypt encrypted relations. So we check for such relations and error out if any are found. We look for encrypted relations simply by counting the number of keys in the key map file of the database. This simple approach is fine even with the risk of a left-over key from a crash or a bug due to the FILE_COPY method being rare and that we therefore want to keep this code minimal.
Using a database with encrypted relations as template works with the
WAL_LOGstrategy as long as you have configured a default principal key. It is a bit hackish but works and since encrypted objects in a template database is likely to be an uncommon use case this is fine for now.The test file is put last in the test run order since there is still no good way to clean up default principal keys.
Additionally we assert there are no encrypted relations when using
FILE_COPY.The
FILE_COPYstrategy ofCREATE DATABASEdoes not use the SMGR so it cannot properly copy and re-encrypt encrypted relations. So we check for such relations and error out if any are found.We look for encrypted relations simply by counting the number of keys in the key map file of the database. This simple approach is fine even with the risk of a left-over key from a crash or a bug due to the
FILE_COPYstrategy being rare and that we therefore want to keep this code minimal.