PG-1093 Fix pending deletes for subtransactions (#302)

* PG-1093 Fix pending deletes for subtransactions

* PG-1093 Add dedicated test cases for tde_heap and tde_heap_basic

Makefile.in

@@ -14,6 +14,7 @@ update_compare_indexes_basic \
 pg_tde_is_encrypted_basic \
 test_issue_153_fix_basic \
 multi_insert_basic \
+subtransaction_basic \
 trigger_on_view_basic \
 change_access_method_basic \
 insert_update_delete_basic \

expected/subtransaction.out

@@ -0,0 +1,32 @@
+\set tde_am tde_heap
+\i sql/subtransaction.inc
+CREATE EXTENSION pg_tde;
+SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');
+ pg_tde_add_key_provider_file 
+------------------------------
+                            1
+(1 row)
+
+SELECT pg_tde_set_principal_key('test-db-principal-key','file-vault');
+ pg_tde_set_principal_key 
+--------------------------
+ t
+(1 row)
+
+BEGIN;                      -- Nesting level 1
+SAVEPOINT sp;
+CREATE TABLE foo(s TEXT);   -- Nesting level 2
+RELEASE SAVEPOINT sp;
+SAVEPOINT sp;
+CREATE TABLE bar(s TEXT);   -- Nesting level 2
+ROLLBACK TO sp;             -- Rollback should not affect first subtransaction
+COMMIT;
+BEGIN;                      -- Nesting level 1
+SAVEPOINT sp;
+DROP TABLE foo;             -- Nesting level 2
+RELEASE SAVEPOINT sp;
+SAVEPOINT sp;
+CREATE TABLE bar(s TEXT);   -- Nesting level 2
+ROLLBACK TO sp;             -- Rollback should not affect first subtransaction
+COMMIT;
+DROP EXTENSION pg_tde;

expected/subtransaction_basic.out

@@ -0,0 +1,32 @@
+\set tde_am tde_heap_basic
+\i sql/subtransaction.inc
+CREATE EXTENSION pg_tde;
+SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');
+ pg_tde_add_key_provider_file 
+------------------------------
+                            1
+(1 row)
+
+SELECT pg_tde_set_principal_key('test-db-principal-key','file-vault');
+ pg_tde_set_principal_key 
+--------------------------
+ t
+(1 row)
+
+BEGIN;                      -- Nesting level 1
+SAVEPOINT sp;
+CREATE TABLE foo(s TEXT);   -- Nesting level 2
+RELEASE SAVEPOINT sp;
+SAVEPOINT sp;
+CREATE TABLE bar(s TEXT);   -- Nesting level 2
+ROLLBACK TO sp;             -- Rollback should not affect first subtransaction
+COMMIT;
+BEGIN;                      -- Nesting level 1
+SAVEPOINT sp;
+DROP TABLE foo;             -- Nesting level 2
+RELEASE SAVEPOINT sp;
+SAVEPOINT sp;
+CREATE TABLE bar(s TEXT);   -- Nesting level 2
+ROLLBACK TO sp;             -- Rollback should not affect first subtransaction
+COMMIT;
+DROP EXTENSION pg_tde;

sql/subtransaction.inc

@@ -0,0 +1,25 @@
+CREATE EXTENSION pg_tde;
+
+SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');
+SELECT pg_tde_set_principal_key('test-db-principal-key','file-vault');
+
+
+BEGIN;                      -- Nesting level 1
+SAVEPOINT sp;
+CREATE TABLE foo(s TEXT);   -- Nesting level 2
+RELEASE SAVEPOINT sp;
+SAVEPOINT sp;
+CREATE TABLE bar(s TEXT);   -- Nesting level 2
+ROLLBACK TO sp;             -- Rollback should not affect first subtransaction
+COMMIT;
+
+BEGIN;                      -- Nesting level 1
+SAVEPOINT sp;
+DROP TABLE foo;             -- Nesting level 2
+RELEASE SAVEPOINT sp;
+SAVEPOINT sp;
+CREATE TABLE bar(s TEXT);   -- Nesting level 2
+ROLLBACK TO sp;             -- Rollback should not affect first subtransaction
+COMMIT;
+
+DROP EXTENSION pg_tde;

sql/subtransaction.sql

@@ -0,0 +1,2 @@
+\set tde_am tde_heap
+\i sql/subtransaction.inc

sql/subtransaction_basic.sql

@@ -0,0 +1,2 @@
+\set tde_am tde_heap_basic
+\i sql/subtransaction.inc

src/access/pg_tde_tdemap.c

@@ -530,6 +530,7 @@ pg_tde_delete_key_map_entry(const RelFileLocator *rlocator)
 	/* Get the file paths */
 	pg_tde_set_db_file_paths(rlocator, db_map_path, db_keydata_path);
 
+	errno = 0;
 	/* Remove the map entry if found */
 	LWLockAcquire(lock_files, LW_EXCLUSIVE);
 	key_index = pg_tde_process_map_entry(rlocator, db_map_path, &offset, false);

src/transam/pg_tde_xact_handler.c

@@ -30,6 +30,7 @@ typedef struct PendingMapEntryDelete
 static PendingMapEntryDelete *pendingDeletes = NULL; /* head of linked list */
 
 static void do_pending_deletes(bool isCommit);
+static void reassign_pending_deletes_to_parent_xact(void);
 static void pending_delete_cleanup(void);
 
 /* Transaction Callbacks from Backend*/
@@ -64,6 +65,11 @@ pg_tde_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
         ereport(DEBUG2,
                 (errmsg("pg_tde_subxact_callback: aborting subtransaction")));
         do_pending_deletes(false);
+    } else if (event == SUBXACT_EVENT_COMMIT_SUB)
+    {
+        ereport(DEBUG2,
+                (errmsg("pg_tde_subxact_callback: committing subtransaction")));
+        reassign_pending_deletes_to_parent_xact();
     }
 }
 
@@ -99,31 +105,53 @@ do_pending_deletes(bool isCommit)
     for (pending = pendingDeletes; pending != NULL; pending = next)
     {
         next = pending->next;
-        if (pending->nestLevel < nestLevel)
+        if (pending->nestLevel != nestLevel)
         {
             /* outer-level entries should not be processed yet */
             prev = pending;
+            continue;
         }
+
+        /* unlink list entry first, so we don't retry on failure */
+        if (prev)
+            prev->next = next;
         else
+            pendingDeletes = next;
+        /* do deletion if called for */
+        if (pending->atCommit == isCommit)
         {
-            /* unlink list entry first, so we don't retry on failure */
+            ereport(LOG,
-            if (prev)
+                    (errmsg("pg_tde_xact_callback: deleting entry at offset %d",
-                prev->next = next;
+                            (int)(pending->map_entry_offset))));
-            else
+            pg_tde_free_key_map_entry(&pending->rlocator, pending->map_entry_offset);
-                pendingDeletes = next;
-            /* do deletion if called for */
-            if (pending->atCommit == isCommit)
-            {
-                ereport(LOG,
-                        (errmsg("pg_tde_xact_callback: deleting entry at offset %d",
-                                (int)(pending->map_entry_offset))));
-
-                pg_tde_free_key_map_entry(&pending->rlocator, pending->map_entry_offset);
-            }
-
-            pfree(pending);
-            /* prev does not change */
         }
+        pfree(pending);
+        /* prev does not change */
+
+    }
+}
+
+
+/*
+  *  reassign_pending_deletes_to_parent_xact() -- Adjust nesting level of pending deletes.
+  *
+  * There are several cases to consider:
+  * 1. Only top level transaction can perform on-commit deletes.
+  * 2. Subtransaction and top level transaction can perform on-abort deletes.
+  * So we have to decrement the nesting level of pending deletes to reassing them to the parent transaction
+  * if subtransaction was not self aborted. In other words if subtransaction state is commited all its pending 
+  * deletes are reassigned to the parent transaction.
+  */
+static void 
+reassign_pending_deletes_to_parent_xact(void)
+{
+    PendingMapEntryDelete *pending;
+    int nestLevel = GetCurrentTransactionNestLevel();
+
+    for (pending = pendingDeletes; pending != NULL; pending = pending->next)
+    {
+        if (pending->nestLevel == nestLevel)
+            pending->nestLevel--;
     }
 }