-
Notifications
You must be signed in to change notification settings - Fork 8
PG-1504 Make partitions inherit encryption status #253
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-1504 Make partitions inherit encryption status #253
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❌ Your project status has failed because the head coverage (78.45%) 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 #253 +/- ##
=====================================================
+ Coverage 78.32% 78.45% +0.13%
=====================================================
Files 22 22
Lines 2468 2479 +11
Branches 385 387 +2
=====================================================
+ Hits 1933 1945 +12
Misses 459 459
+ Partials 76 75 -1
🚀 New features to boost your workflow:
|
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 think it has a bug, but even if not I think the tests should also involve the GUC.
I'm not 100% sure how to handle At least the testcase could include this behavior. |
bcb9f9a
to
f1beaf2
Compare
Agreed! I included the current behavior in the test, we can change it later if we would want to allow creation of unencrypted partitions even if the GUC is set. |
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 think there is one case with the default GUC which is still not properly handled but I could be wrong.
f1beaf2
to
1041d55
Compare
Should be fixed now! |
1041d55
to
134bf2e
Compare
parentOid = RangeVarGetRelid(linitial(stmt->inhRelations), | ||
AccessExclusiveLock, | ||
false); | ||
parentAmOid = get_rel_relam(parentOid); |
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.
You could accomplish the same with relation_open_rv()
but I am not sure which is cleaner.
AccessExclusiveLock, | ||
false); | ||
parentAmOid = get_rel_relam(parentOid); | ||
foundAccessMethod = (bool) parentAmOid; |
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 think PostgreSQL style for this is foundAccessMethod = parentAmOid != InvalidOid
.
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.
Yes! That's better. I didn't know about InvalidOid
so i felt this was just as good as randomly comparing to 0
Pushed a fixup and also added a noop to the if
just below to make it slightly clearer what's going on.
Since the access method isn't included in the statement we'll have to look it up on the parent in the same way DefineRelation() does.
6a2d44f
to
050d87e
Compare
Since the access method isn't included in the statement we'll have to look it up on the parent in the same way DefineRelation() does.