PG-1444 Implement deletion of relation keys when dropping relations

contrib/pg_tde/Makefile

@@ -21,7 +21,6 @@ partition_table \
 pg_tde_is_encrypted \
 recreate_storage \
 relocate \
-subtransaction \
 tablespace \
 vault_v2_test \
 version \
@@ -33,7 +32,6 @@ src/encryption/enc_aes.o \
 src/access/pg_tde_tdemap.o \
 src/access/pg_tde_xlog.o \
 src/access/pg_tde_xlog_encrypt.o \
-src/transam/pg_tde_xact_handler.o \
 src/keyring/keyring_curl.o \
 src/keyring/keyring_file.o \
 src/keyring/keyring_vault.o \

contrib/pg_tde/meson.build

@@ -21,7 +21,6 @@ pg_tde_sources = files(
   'src/pg_tde_event_capture.c',
   'src/pg_tde_guc.c',
   'src/smgr/pg_tde_smgr.c',
-  'src/transam/pg_tde_xact_handler.c',
 )
 
 tde_frontend_sources = files(
@@ -97,7 +96,6 @@ sql_tests = [
   'pg_tde_is_encrypted',
   'relocate',
   'recreate_storage',
-  'subtransaction',
   'tablespace',
   'vault_v2_test',
   'version',

contrib/pg_tde/src/access/pg_tde_tdemap.c

@@ -13,7 +13,6 @@
 #include "postgres.h"
 #include "access/pg_tde_tdemap.h"
 #include "common/file_perm.h"
-#include "transam/pg_tde_xact_handler.h"
 #include "storage/fd.h"
 #include "utils/wait_event.h"
 #include "utils/memutils.h"
@@ -129,7 +128,6 @@ static int	pg_tde_file_header_write(const char *tde_filename, int fd, const TDES
 static void pg_tde_sign_principal_key_info(TDESignedPrincipalKeyInfo *signed_key_info, const TDEPrincipalKey *principal_key);
 static off_t pg_tde_write_one_map_entry(int fd, const TDEMapEntry *map_entry, off_t *offset, const char *db_map_path);
 static void pg_tde_write_key_map_entry(const RelFileLocator *rlocator, InternalKey *rel_key_data, TDEPrincipalKey *principal_key, bool write_xlog);
-static bool pg_tde_delete_map_entry(const RelFileLocator *rlocator, char *db_map_path, off_t offset);
 static int	keyrotation_init_file(const TDESignedPrincipalKeyInfo *signed_key_info, char *rotated_filename, const char *filename, off_t *curr_pos);
 static void finalize_key_rotation(const char *path_old, const char *path_new);
 static int	pg_tde_open_file_write(const char *tde_filename, const TDESignedPrincipalKeyInfo *signed_key_info, bool truncate, off_t *curr_pos);
@@ -486,9 +484,6 @@ pg_tde_write_key_map_entry(const RelFileLocator *rlocator, InternalKey *rel_key_
 
 	/* Let's close the file. */
 	close(map_fd);
-
-	/* Register the entry to be freed in case the transaction aborts */
-	RegisterEntryForDeletion(rlocator, curr_pos, false);
 }
 
 /*
@@ -548,51 +543,40 @@ pg_tde_write_key_map_entry_redo(const TDEMapEntry *write_map_entry, TDESignedPri
 	LWLockRelease(tde_lwlock_enc_keys());
 }
 
-static bool
-pg_tde_delete_map_entry(const RelFileLocator *rlocator, char *db_map_path, off_t offset)
+/*
+ * Mark relation map entry as free and overwrite the key
+ *
+ * This fucntion is called by the pg_tde SMGR when storage is unlinked on
+ * transaction commit/abort.
+ */
+void
+pg_tde_free_key_map_entry(const RelFileLocator *rlocator)
 {
+	char		db_map_path[MAXPGPATH];
 	File		map_fd;
-	bool		found = false;
 	off_t		curr_pos = 0;
 
-	/* Open and validate file for basic correctness. */
-	map_fd = pg_tde_open_file_write(db_map_path, NULL, false, &curr_pos);
+	Assert(rlocator);
 
-	/*
-	 * If we need to delete an entry, we expect an offset value to the start
-	 * of the entry to speed up the operation. Otherwise, we'd be sequentially
-	 * scanning the entire map file.
-	 */
-	if (offset > 0)
-	{
-		curr_pos = lseek(map_fd, offset, SEEK_SET);
+	pg_tde_set_db_file_path(rlocator->dbOid, db_map_path);
 
-		if (curr_pos == -1)
-		{
-			ereport(ERROR,
-					errcode_for_file_access(),
-					errmsg("could not seek in tde map file \"%s\": %m",
-						   db_map_path));
-		}
-	}
+	LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE);
+
+	/* Open and validate file for basic correctness. */
+	map_fd = pg_tde_open_file_write(db_map_path, NULL, false, &curr_pos);
 
-	/*
-	 * Read until we find an empty slot. Otherwise, read until end. This seems
-	 * to be less frequent than vacuum. So let's keep this function here
-	 * rather than overloading the vacuum process.
-	 */
 	while (1)
 	{
 		TDEMapEntry read_map_entry;
 		off_t		prev_pos = curr_pos;
+		bool		found;
 
 		found = pg_tde_read_one_map_entry(map_fd, rlocator, MAP_ENTRY_VALID, &read_map_entry, &curr_pos);
 
 		/* We've reached EOF */
 		if (curr_pos == prev_pos)
 			break;
 
-		/* We found a valid entry for the relation */
 		if (found)
 		{
 			TDEMapEntry empty_map_entry = {
@@ -607,52 +591,9 @@ pg_tde_delete_map_entry(const RelFileLocator *rlocator, char *db_map_path, off_t
 		}
 	}
 
-	/* Let's close the file. */
 	close(map_fd);
 
-	/* Return -1 indicating that no entry was removed */
-	return found;
-}
-
-/*
- * Called when transaction is being completed; either committed or aborted.
- * By default, when a transaction creates an entry, we mark it as MAP_ENTRY_VALID.
- * Only during the abort phase of the transaction that we are proceed on with
- * marking the entry as MAP_ENTRY_FREE. This optimistic strategy that assumes
- * that transaction will commit more often then getting aborted avoids
- * unnecessary locking.
- *
- * The offset allows us to simply seek to the desired location and mark the entry
- * as MAP_ENTRY_FREE without needing any further processing.
- */
-void
-pg_tde_free_key_map_entry(const RelFileLocator *rlocator, off_t offset)
-{
-	bool		found;
-	char		db_map_path[MAXPGPATH] = {0};
-
-	Assert(rlocator);
-
-	/* Get the file paths */
-	pg_tde_set_db_file_path(rlocator->dbOid, db_map_path);
-
-	LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE);
-
-	/* Remove the map entry if found */
-	found = pg_tde_delete_map_entry(rlocator, db_map_path, offset);
-
 	LWLockRelease(tde_lwlock_enc_keys());
-
-	if (!found)
-	{
-		ereport(WARNING,
-				errcode(ERRCODE_NO_DATA_FOUND),
-				errmsg("could not find the required map entry for deletion of relation %d in tablespace %d in tde map file \"%s\": %m",
-					   rlocator->relNumber,
-					   rlocator->spcOid,
-					   db_map_path));
-
-	}
 }
 
 /*

contrib/pg_tde/src/access/pg_tde_xlog.c

@@ -54,12 +54,6 @@ tdeheap_rmgr_redo(XLogReaderState *record)
 
 		pg_tde_write_key_map_entry_redo(&xlrec->mapEntry, &xlrec->pkInfo);
 	}
-	else if (info == XLOG_TDE_REMOVE_RELATION_KEY)
-	{
-		RelFileLocator *xlrec = (RelFileLocator *) XLogRecGetData(record);
-
-		pg_tde_free_key_map_entry(xlrec, 0);
-	}
 	else if (info == XLOG_TDE_ADD_PRINCIPAL_KEY)
 	{
 		TDESignedPrincipalKeyInfo *mkey = (TDESignedPrincipalKeyInfo *) XLogRecGetData(record);
@@ -101,12 +95,6 @@ tdeheap_rmgr_desc(StringInfo buf, XLogReaderState *record)
 
 		appendStringInfo(buf, "rel: %u/%u/%u", xlrec->mapEntry.spcOid, xlrec->pkInfo.data.databaseId, xlrec->mapEntry.relNumber);
 	}
-	else if (info == XLOG_TDE_REMOVE_RELATION_KEY)
-	{
-		RelFileLocator *xlrec = (RelFileLocator *) XLogRecGetData(record);
-
-		appendStringInfo(buf, "rel: %u/%u/%u", xlrec->spcOid, xlrec->dbOid, xlrec->relNumber);
-	}
 	else if (info == XLOG_TDE_ADD_PRINCIPAL_KEY)
 	{
 		TDEPrincipalKeyInfo *xlrec = (TDEPrincipalKeyInfo *) XLogRecGetData(record);
@@ -140,8 +128,6 @@ tdeheap_rmgr_identify(uint8 info)
 	{
 		case XLOG_TDE_ADD_RELATION_KEY:
 			return "ADD_RELATION_KEY";
-		case XLOG_TDE_REMOVE_RELATION_KEY:
-			return "REMOVE_RELATION_KEY";
 		case XLOG_TDE_ADD_PRINCIPAL_KEY:
 			return "ADD_PRINCIPAL_KEY";
 		case XLOG_TDE_ROTATE_PRINCIPAL_KEY:

contrib/pg_tde/src/include/access/pg_tde_tdemap.h

@@ -101,7 +101,7 @@ extern void pg_tde_wal_last_key_set_lsn(XLogRecPtr lsn, const char *keyfile_path
 
 extern InternalKey *pg_tde_create_smgr_key(const RelFileLocatorBackend *newrlocator);
 extern void pg_tde_create_wal_key(InternalKey *rel_key_data, const RelFileLocator *newrlocator, uint32 flags);
-extern void pg_tde_free_key_map_entry(const RelFileLocator *rlocator, off_t offset);
+extern void pg_tde_free_key_map_entry(const RelFileLocator *rlocator);
 extern void pg_tde_write_key_map_entry_redo(const TDEMapEntry *write_map_entry, TDESignedPrincipalKeyInfo *signed_key_info);
 
 #define PG_TDE_MAP_FILENAME			"pg_tde_%d_map"

contrib/pg_tde/src/include/access/pg_tde_xlog.h

@@ -15,11 +15,10 @@
 
 /* TDE XLOG record types */
 #define XLOG_TDE_ADD_RELATION_KEY		0x00
-#define XLOG_TDE_REMOVE_RELATION_KEY	0x10
-#define XLOG_TDE_ADD_PRINCIPAL_KEY		0x20
-#define XLOG_TDE_ROTATE_PRINCIPAL_KEY	0x30
-#define XLOG_TDE_WRITE_KEY_PROVIDER 	0x40
-#define XLOG_TDE_INSTALL_EXTENSION		0x50
+#define XLOG_TDE_ADD_PRINCIPAL_KEY		0x10
+#define XLOG_TDE_ROTATE_PRINCIPAL_KEY	0x20
+#define XLOG_TDE_WRITE_KEY_PROVIDER 	0x30
+#define XLOG_TDE_INSTALL_EXTENSION		0x40
 
 /* ID 140 is registered for Percona TDE extension: https://wiki.postgresql.org/wiki/CustomWALResourceManagers */
 #define RM_TDERMGR_ID	140

contrib/pg_tde/src/pg_tde.c

@@ -13,7 +13,6 @@
 #include "postgres.h"
 #include "funcapi.h"
 #include "pg_tde.h"
-#include "transam/pg_tde_xact_handler.h"
 #include "miscadmin.h"
 #include "storage/ipc.h"
 #include "storage/lwlock.h"
@@ -121,7 +120,6 @@ _PG_init(void)
 	prev_shmem_startup_hook = shmem_startup_hook;
 	shmem_startup_hook = tde_shmem_startup;
 
-	RegisterTdeXactCallbacks();
 	InstallFileKeyring();
 	InstallVaultV2Keyring();
 	InstallKmipKeyring();

contrib/pg_tde/src/smgr/pg_tde_smgr.c

@@ -115,6 +115,28 @@ tde_mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
 	}
 }
 
+static void
+tde_mdunlink(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo)
+{
+	mdunlink(rlocator, forknum, isRedo);
+
+	/*
+	 * As of PostgreSQL 17 we are called once per forks, no matter if they
+	 * exist or not, from smgrdounlinkall() so deleting the relation key on
+	 * attempting to delete the main fork is safe. Additionally since we
+	 * unlink the files after commit/abort we do not need to care about
+	 * concurrent accesses.
+	 *
+	 * We support InvalidForkNumber to be similar to mdunlink() but it can
+	 * actually never happen.
+	 */
+	if (forknum == MAIN_FORKNUM || forknum == InvalidForkNumber)
+	{
+		if (!RelFileLocatorBackendIsTemp(rlocator) && GetSMGRRelationKey(rlocator))
+			pg_tde_free_key_map_entry(&rlocator.locator);
+	}
+}
+
 static void
 tde_mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
 			 const void *buffer, bool skipFsync)
@@ -274,7 +296,7 @@ static const struct f_smgr tde_smgr = {
 	.smgr_close = mdclose,
 	.smgr_create = tde_mdcreate,
 	.smgr_exists = mdexists,
-	.smgr_unlink = mdunlink,
+	.smgr_unlink = tde_mdunlink,
 	.smgr_extend = tde_mdextend,
 	.smgr_zeroextend = mdzeroextend,
 	.smgr_prefetch = mdprefetch,