From d37c717e143fdd07275393f5e81ebbc6780fc069 Mon Sep 17 00:00:00 2001 From: Hadi Moshayedi Date: Sat, 26 Sep 2020 12:39:16 -0700 Subject: [PATCH] Clean-up resources on drop --- cstore.c | 80 ++++++++++++++++++++++++++++++++++++++++ cstore.h | 1 + cstore_fdw.c | 50 ------------------------- cstore_metadata_tables.c | 11 ++++++ expected/am_drop.out | 15 ++++++++ expected/fdw_drop.out | 15 ++++++++ sql/am_drop.sql | 8 ++++ sql/fdw_drop.sql | 8 ++++ 8 files changed, 138 insertions(+), 50 deletions(-) diff --git a/cstore.c b/cstore.c index d6b6751e2..1d6e414ae 100644 --- a/cstore.c +++ b/cstore.c @@ -16,9 +16,13 @@ #include #include +#include "access/heapam.h" +#include "catalog/objectaccess.h" +#include "catalog/storage.h" #include "miscadmin.h" #include "utils/guc.h" #include "utils/rel.h" +#include "utils/relcache.h" #include "cstore.h" @@ -38,6 +42,11 @@ static const struct config_enum_entry cstore_compression_options[] = { NULL, 0, false } }; +static object_access_hook_type prevObjectAccess = NULL; + +static void ObjectAccess(ObjectAccessType access, Oid classId, Oid objectId, int subId, + void *arg); + void cstore_init() { @@ -78,6 +87,9 @@ cstore_init() NULL, NULL, NULL); + + prevObjectAccess = object_access_hook; + object_access_hook = ObjectAccess; } @@ -110,3 +122,71 @@ InitializeCStoreTableFile(Oid relNode, CStoreOptions *cstoreOptions) { InitCStoreTableMetadata(relNode, cstoreOptions->blockRowCount); } + + +/* + * Implements object_access_hook. One of the places this is called is just + * before dropping an object, which allows us to clean-up resources for + * cstore tables while the pg_class record for the table is still there. + */ +static void +ObjectAccess(ObjectAccessType access, Oid classId, Oid objectId, int subId, void *arg) +{ + if (prevObjectAccess) + { + prevObjectAccess(access, classId, objectId, subId, arg); + } + + /* + * Do nothing if this is not a DROP relation command. + */ + if (access != OAT_DROP || classId != RelationRelationId || OidIsValid(subId)) + { + return; + } + + if (IsCStoreFdwTable(objectId)) + { + /* + * Drop both metadata and storage. We need to drop storage here since + * we manage relfilenode for FDW tables in the extension. + */ + Relation rel = cstore_fdw_open(objectId, AccessExclusiveLock); + RelationOpenSmgr(rel); + RelationDropStorage(rel); + DeleteTableMetadataRowIfExists(rel->rd_node.relNode); + + /* keep the lock since we did physical changes to the relation */ + relation_close(rel, NoLock); + } + else + { + Oid relNode = InvalidOid; + Relation rel = try_relation_open(objectId, AccessExclusiveLock); + if (rel == NULL) + { + return; + } + + relNode = rel->rd_node.relNode; + if (IsCStoreStorage(relNode)) + { + /* + * Drop only metadata for table am cstore tables. Postgres manages + * storage for these tables, so we don't need to drop that. + */ + DeleteTableMetadataRowIfExists(relNode); + + /* keep the lock since we did physical changes to the relation */ + relation_close(rel, NoLock); + } + else + { + /* + * For non-cstore tables, we do nothing. + * Release the lock since we haven't changed the relation. + */ + relation_close(rel, AccessExclusiveLock); + } + } +} diff --git a/cstore.h b/cstore.h index dd5f9e6e1..919352c6c 100644 --- a/cstore.h +++ b/cstore.h @@ -283,6 +283,7 @@ extern bool CompressBuffer(StringInfo inputBuffer, StringInfo outputBuffer, extern StringInfo DecompressBuffer(StringInfo buffer, CompressionType compressionType); /* cstore_metadata_tables.c */ +extern bool IsCStoreStorage(Oid relfilenode); extern void DeleteTableMetadataRowIfExists(Oid relfilenode); extern void InitCStoreTableMetadata(Oid relfilenode, int blockRowCount); extern void InsertStripeMetadataRow(Oid relfilenode, StripeMetadata *stripe); diff --git a/cstore_fdw.c b/cstore_fdw.c index f9f886f79..d4c5c1ec1 100644 --- a/cstore_fdw.c +++ b/cstore_fdw.c @@ -126,7 +126,6 @@ static uint64 CopyIntoCStoreTable(const CopyStmt *copyStatement, const char *queryString); static uint64 CopyOutCStoreTable(CopyStmt *copyStatement, const char *queryString); static void CStoreProcessAlterTableCommand(AlterTableStmt *alterStatement); -static List * DroppedCStoreRelidList(DropStmt *dropStatement); static List * FindCStoreTables(List *tableList); static List * OpenRelationsForTruncate(List *cstoreTableList); static void FdwNewRelFileNode(Relation relation); @@ -315,25 +314,6 @@ CStoreProcessUtility(Node * parseTree, const char * queryString, destReceiver, completionTag); } } - else if (nodeTag(parseTree) == T_DropStmt) - { - List *dropRelids = DroppedCStoreRelidList((DropStmt *) parseTree); - ListCell *lc = NULL; - - /* drop smgr storage */ - foreach(lc, dropRelids) - { - Oid relid = lfirst_oid(lc); - Relation relation = cstore_fdw_open(relid, AccessExclusiveLock); - - RelationOpenSmgr(relation); - RelationDropStorage(relation); - heap_close(relation, AccessExclusiveLock); - } - - CALL_PREVIOUS_UTILITY(parseTree, queryString, context, paramListInfo, - destReceiver, completionTag); - } else if (nodeTag(parseTree) == T_TruncateStmt) { TruncateStmt *truncateStatement = (TruncateStmt *) parseTree; @@ -723,36 +703,6 @@ CStoreProcessAlterTableCommand(AlterTableStmt *alterStatement) } -/* - * DropppedCStoreRelidList extracts and returns the list of cstore relids - * from DROP table statement - */ -static List * -DroppedCStoreRelidList(DropStmt *dropStatement) -{ - List *droppedCStoreRelidList = NIL; - - if (dropStatement->removeType == OBJECT_FOREIGN_TABLE) - { - ListCell *dropObjectCell = NULL; - foreach(dropObjectCell, dropStatement->objects) - { - List *tableNameList = (List *) lfirst(dropObjectCell); - RangeVar *rangeVar = makeRangeVarFromNameList(tableNameList); - - Oid relationId = RangeVarGetRelid(rangeVar, AccessShareLock, true); - if (CStoreTable(relationId)) - { - droppedCStoreRelidList = lappend_oid(droppedCStoreRelidList, - relationId); - } - } - } - - return droppedCStoreRelidList; -} - - /* FindCStoreTables returns list of CStore tables from given table list */ static List * FindCStoreTables(List *tableList) diff --git a/cstore_metadata_tables.c b/cstore_metadata_tables.c index 4459d3009..e1f1caedf 100644 --- a/cstore_metadata_tables.c +++ b/cstore_metadata_tables.c @@ -94,6 +94,17 @@ static Datum ByteaToDatum(bytea *bytes, Form_pg_attribute attrForm); #define Anum_cstore_skipnodes_value_compression_type 12 +/* + * IsCStoreStorage returns if relfilenode belongs to a cstore table. + */ +bool +IsCStoreStorage(Oid relfilenode) +{ + uint64 blockRowCount = 0; + return ReadCStoreTables(relfilenode, &blockRowCount); +} + + /* * InitCStoreTableMetadata adds a record for the given relation in cstore_table. */ diff --git a/expected/am_drop.out b/expected/am_drop.out index e1c634d7f..c1fc60519 100644 --- a/expected/am_drop.out +++ b/expected/am_drop.out @@ -12,14 +12,29 @@ -- 'postgres' directory is excluded from comparison to have the same result. -- store postgres database oid SELECT oid postgres_oid FROM pg_database WHERE datname = 'postgres' \gset +SELECT count(*) AS cstore_tables_before_drop FROM cstore.cstore_tables \gset -- DROP cstore_fdw tables DROP TABLE contestant; DROP TABLE contestant_compressed; +-- make sure DROP deletes metadata +SELECT :cstore_tables_before_drop - count(*) FROM cstore.cstore_tables; + ?column? +---------- + 2 +(1 row) + -- Create a cstore_fdw table under a schema and drop it. CREATE SCHEMA test_schema; CREATE TABLE test_schema.test_table(data int) USING cstore_tableam; +SELECT count(*) AS cstore_tables_before_drop FROM cstore.cstore_tables \gset DROP SCHEMA test_schema CASCADE; NOTICE: drop cascades to table test_schema.test_table +SELECT :cstore_tables_before_drop - count(*) FROM cstore.cstore_tables; + ?column? +---------- + 1 +(1 row) + SELECT current_database() datname \gset CREATE DATABASE db_to_drop; \c db_to_drop diff --git a/expected/fdw_drop.out b/expected/fdw_drop.out index 926f69337..24c0f518d 100644 --- a/expected/fdw_drop.out +++ b/expected/fdw_drop.out @@ -12,14 +12,29 @@ -- 'postgres' directory is excluded from comparison to have the same result. -- store postgres database oid SELECT oid postgres_oid FROM pg_database WHERE datname = 'postgres' \gset +SELECT count(*) AS cstore_tables_before_drop FROM cstore.cstore_tables \gset -- DROP cstore_fdw tables DROP FOREIGN TABLE contestant; DROP FOREIGN TABLE contestant_compressed; +-- make sure DROP deletes metadata +SELECT :cstore_tables_before_drop - count(*) FROM cstore.cstore_tables; + ?column? +---------- + 2 +(1 row) + -- Create a cstore_fdw table under a schema and drop it. CREATE SCHEMA test_schema; CREATE FOREIGN TABLE test_schema.test_table(data int) SERVER cstore_server; +SELECT count(*) AS cstore_tables_before_drop FROM cstore.cstore_tables \gset DROP SCHEMA test_schema CASCADE; NOTICE: drop cascades to foreign table test_schema.test_table +SELECT :cstore_tables_before_drop - count(*) FROM cstore.cstore_tables; + ?column? +---------- + 1 +(1 row) + SELECT current_database() datname \gset CREATE DATABASE db_to_drop; \c db_to_drop diff --git a/sql/am_drop.sql b/sql/am_drop.sql index f92f90b9d..06873aa6e 100644 --- a/sql/am_drop.sql +++ b/sql/am_drop.sql @@ -15,14 +15,22 @@ -- store postgres database oid SELECT oid postgres_oid FROM pg_database WHERE datname = 'postgres' \gset +SELECT count(*) AS cstore_tables_before_drop FROM cstore.cstore_tables \gset + -- DROP cstore_fdw tables DROP TABLE contestant; DROP TABLE contestant_compressed; +-- make sure DROP deletes metadata +SELECT :cstore_tables_before_drop - count(*) FROM cstore.cstore_tables; + -- Create a cstore_fdw table under a schema and drop it. CREATE SCHEMA test_schema; CREATE TABLE test_schema.test_table(data int) USING cstore_tableam; + +SELECT count(*) AS cstore_tables_before_drop FROM cstore.cstore_tables \gset DROP SCHEMA test_schema CASCADE; +SELECT :cstore_tables_before_drop - count(*) FROM cstore.cstore_tables; SELECT current_database() datname \gset diff --git a/sql/fdw_drop.sql b/sql/fdw_drop.sql index c64b5c99b..7c6dd5c6e 100644 --- a/sql/fdw_drop.sql +++ b/sql/fdw_drop.sql @@ -15,14 +15,22 @@ -- store postgres database oid SELECT oid postgres_oid FROM pg_database WHERE datname = 'postgres' \gset +SELECT count(*) AS cstore_tables_before_drop FROM cstore.cstore_tables \gset + -- DROP cstore_fdw tables DROP FOREIGN TABLE contestant; DROP FOREIGN TABLE contestant_compressed; +-- make sure DROP deletes metadata +SELECT :cstore_tables_before_drop - count(*) FROM cstore.cstore_tables; + -- Create a cstore_fdw table under a schema and drop it. CREATE SCHEMA test_schema; CREATE FOREIGN TABLE test_schema.test_table(data int) SERVER cstore_server; + +SELECT count(*) AS cstore_tables_before_drop FROM cstore.cstore_tables \gset DROP SCHEMA test_schema CASCADE; +SELECT :cstore_tables_before_drop - count(*) FROM cstore.cstore_tables; SELECT current_database() datname \gset