-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fix update duplication after crashfix #69
Conversation
Issue: with the fix in Percona-Lab#67, pgtde decrypts tuples during update into a new memory region, and changes the t_data pointer to this new region. Because of this, later updates to tuple flags also happen in the new data, and the original persisted tuple flags are never updated. Fix: after the update statement is done with the decrypted data, restore the t_data pointer to the original. This way, flag changes happen where they should. Fixes Percona-Lab#68
src/access/pg_tdeam.c
Outdated
@@ -3012,6 +3012,7 @@ pg_tde_update(Relation relation, ItemPointer otid, HeapTuple newtup, | |||
ItemId lp; | |||
HeapTupleData oldtup; | |||
HeapTupleData oldtup2; | |||
void* oldtupptr; |
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.
Tiny suggestion. I think we should use more descriptive name for oldtupptr (may be oldtup_data). since the variable refers to the tuple data instead of the tuple. Similarly not related to this particular PR, but oldtup2 may be renamed as decrypted_tup or something to make it easier to understand when we revisit the code in the future.
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.
Not related to this particular PR. But I figured out we are copying the complete tuple data in new_ptr in the destination tuple before decrypting the data in that.
/* decrypt the old tuple */
{
char* new_ptr = NULL;
new_ptr = MemoryContextAlloc(CurTransactionContext, oldtup.t_len);
memcpy(new_ptr, oldtup.t_data, oldtup.t_len); <==== Here
// only neccessary field
oldtup2.t_data = (HeapTupleHeader)new_ptr;
}
This is fine for small size tuples, but for large tuples we can gain some performance by saving the memory copy by only copying the tuple header ( since that is all what we need for destination tuple)
/* decrypt the old tuple */
{
char* new_ptr = NULL;
new_ptr = MemoryContextAlloc(CurTransactionContext, oldtup.t_len);
memcpy(new_ptr, oldtup.t_data, oldtup.t_data->t_hoff); <==== Here
// only neccessary field
oldtup2.t_data = (HeapTupleHeader)new_ptr;
}
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 perfect
Issue: with the fix in #67, pgtde decrypts tuples during update into a new memory region, and changes the t_data pointer to this new region.
Because of this, later updates to tuple flags also happen in the new data, and the original persisted tuple flags are never updated.
Fix: after the update statement is done with the decrypted data, restore the t_data pointer to the original. This way, flag changes happen where they should.
Fixes #68