From 7cc8c8c155ad07e8a9be7a844dc332a5c2a9ffda Mon Sep 17 00:00:00 2001 From: Hadi Moshayedi Date: Mon, 5 Oct 2020 10:27:58 -0700 Subject: [PATCH 1/3] Support VACUUM FULL --- Makefile | 2 +- cstore_tableam.c | 122 ++++++++++++++++++++++++++++++++++------- expected/am_vacuum.out | 89 ++++++++++++++++++++++++++++++ sql/am_vacuum.sql | 37 +++++++++++++ 4 files changed, 229 insertions(+), 21 deletions(-) create mode 100644 expected/am_vacuum.out create mode 100644 sql/am_vacuum.sql diff --git a/Makefile b/Makefile index 6ef8431c8..60d8855f8 100644 --- a/Makefile +++ b/Makefile @@ -53,7 +53,7 @@ ifeq ($(USE_TABLEAM),yes) PG_CFLAGS += -DUSE_TABLEAM OBJS += cstore_tableam.o REGRESS += am_create am_load am_query am_analyze am_data_types am_functions \ - am_drop am_insert am_copyto am_alter am_rollback am_truncate am_clean + am_drop am_insert am_copyto am_alter am_rollback am_truncate am_vacuum am_clean endif ifeq ($(enable_coverage),yes) diff --git a/cstore_tableam.c b/cstore_tableam.c index 0369ca15a..337dbe06f 100644 --- a/cstore_tableam.c +++ b/cstore_tableam.c @@ -19,6 +19,7 @@ #include "catalog/storage.h" #include "catalog/storage_xlog.h" #include "commands/progress.h" +#include "commands/vacuum.h" #include "executor/executor.h" #include "nodes/makefuncs.h" #include "optimizer/plancat.h" @@ -131,6 +132,36 @@ cstore_free_write_state() } +static List * +RelationColumnList(Relation rel) +{ + List *columnList = NIL; + TupleDesc tupdesc = RelationGetDescr(rel); + + for (int i = 0; i < tupdesc->natts; i++) + { + Index varno = 0; + AttrNumber varattno = i + 1; + Oid vartype = tupdesc->attrs[i].atttypid; + int32 vartypmod = 0; + Oid varcollid = 0; + Index varlevelsup = 0; + Var *var; + + if (tupdesc->attrs[i].attisdropped) + { + continue; + } + + var = makeVar(varno, varattno, vartype, vartypmod, + varcollid, varlevelsup); + columnList = lappend(columnList, var); + } + + return columnList; +} + + static const TupleTableSlotOps * cstore_slot_callbacks(Relation relation) { @@ -157,25 +188,7 @@ cstore_beginscan(Relation relation, Snapshot snapshot, scan->cs_base.rs_flags = flags; scan->cs_base.rs_parallel = parallel_scan; - for (int i = 0; i < tupdesc->natts; i++) - { - Index varno = 0; - AttrNumber varattno = i + 1; - Oid vartype = tupdesc->attrs[i].atttypid; - int32 vartypmod = 0; - Oid varcollid = 0; - Index varlevelsup = 0; - Var *var; - - if (tupdesc->attrs[i].attisdropped) - { - continue; - } - - var = makeVar(varno, varattno, vartype, vartypmod, - varcollid, varlevelsup); - columnList = lappend(columnList, var); - } + columnList = RelationColumnList(relation); readState = CStoreBeginRead(relation, tupdesc, columnList, NULL); @@ -497,6 +510,13 @@ cstore_relation_copy_data(Relation rel, const RelFileNode *newrnode) } +/* + * cstore_relation_copy_for_cluster is called on VACUUM FULL, at which + * we should copy data from OldHeap to NewHeap. + * + * In general TableAM case this can also be called for the CLUSTER command + * which is not applicable for cstore since it doesn't support indexes. + */ static void cstore_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap, Relation OldIndex, bool use_sort, @@ -507,7 +527,69 @@ cstore_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap, double *tups_vacuumed, double *tups_recently_dead) { - elog(ERROR, "cstore_relation_copy_for_cluster not implemented"); + TableWriteState *writeState = NULL; + TableReadState *readState = NULL; + CStoreOptions *cstoreOptions = NULL; + Datum *sourceValues = NULL; + bool *sourceNulls = NULL; + Datum *targetValues = NULL; + bool *targetNulls = NULL; + TupleDesc sourceDesc = RelationGetDescr(OldHeap); + TupleDesc targetDesc = RelationGetDescr(NewHeap); + + if (OldIndex != NULL || use_sort) + { + ereport(ERROR, (errmsg("cstore_am doesn't support indexes"))); + } + + /* + * copy_table_data in cluster.c assumes tuple descriptors are exactly + * the same. Even dropped columns exist and are marked as attisdropped + * in the target relation. + */ + Assert(sourceDesc->natts == targetDesc->natts); + + cstoreOptions = CStoreTableAMGetOptions(); + + writeState = CStoreBeginWrite(NewHeap, + cstoreOptions->compressionType, + cstoreOptions->stripeRowCount, + cstoreOptions->blockRowCount, + targetDesc); + + readState = CStoreBeginRead(OldHeap, sourceDesc, RelationColumnList(OldHeap), NULL); + + sourceValues = palloc0(sourceDesc->natts * sizeof(Datum)); + sourceNulls = palloc0(sourceDesc->natts * sizeof(bool)); + + targetValues = palloc0(targetDesc->natts * sizeof(Datum)); + targetNulls = palloc0(targetDesc->natts * sizeof(bool)); + + *num_tuples = 0; + + while (CStoreReadNextRow(readState, sourceValues, sourceNulls)) + { + memset(targetNulls, true, targetDesc->natts * sizeof(bool)); + + for (int attrIndex = 0; attrIndex < sourceDesc->natts; attrIndex++) + { + FormData_pg_attribute *sourceAttr = TupleDescAttr(sourceDesc, attrIndex); + + if (!sourceAttr->attisdropped) + { + targetNulls[attrIndex] = sourceNulls[attrIndex]; + targetValues[attrIndex] = sourceValues[attrIndex]; + } + } + + CStoreWriteRow(writeState, targetValues, targetNulls); + (*num_tuples)++; + } + + *tups_vacuumed = *num_tuples; + + CStoreEndWrite(writeState); + CStoreEndRead(readState); } diff --git a/expected/am_vacuum.out b/expected/am_vacuum.out new file mode 100644 index 000000000..d689be800 --- /dev/null +++ b/expected/am_vacuum.out @@ -0,0 +1,89 @@ +CREATE TABLE t(a int, b int) USING cstore_tableam; +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + count +------- + 0 +(1 row) + +INSERT INTO t SELECT i, i * i FROM generate_series(1, 10) i; +INSERT INTO t SELECT i, i * i FROM generate_series(11, 20) i; +INSERT INTO t SELECT i, i * i FROM generate_series(21, 30) i; +SELECT sum(a), sum(b) FROM t; + sum | sum +-----+------ + 465 | 9455 +(1 row) + +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + count +------- + 3 +(1 row) + +-- vacuum full should merge stripes together +VACUUM FULL t; +SELECT sum(a), sum(b) FROM t; + sum | sum +-----+------ + 465 | 9455 +(1 row) + +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + count +------- + 1 +(1 row) + +-- test the case when all data cannot fit into a single stripe +SET cstore.stripe_row_count TO 1000; +INSERT INTO t SELECT i, 2 * i FROM generate_series(1,2500) i; +SELECT sum(a), sum(b) FROM t; + sum | sum +---------+--------- + 3126715 | 6261955 +(1 row) + +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + count +------- + 4 +(1 row) + +VACUUM FULL t; +SELECT sum(a), sum(b) FROM t; + sum | sum +---------+--------- + 3126715 | 6261955 +(1 row) + +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + count +------- + 3 +(1 row) + +-- VACUUM FULL doesn't reclaim dropped columns, but converts them to NULLs +ALTER TABLE t DROP COLUMN a; +SELECT stripe, attr, block, minimum_value IS NULL, maximum_value IS NULL FROM cstore.cstore_skipnodes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t' ORDER BY 1, 2, 3; + stripe | attr | block | ?column? | ?column? +--------+------+-------+----------+---------- + 0 | 1 | 0 | f | f + 0 | 2 | 0 | f | f + 1 | 1 | 0 | f | f + 1 | 2 | 0 | f | f + 2 | 1 | 0 | f | f + 2 | 2 | 0 | f | f +(6 rows) + +VACUUM FULL t; +SELECT stripe, attr, block, minimum_value IS NULL, maximum_value IS NULL FROM cstore.cstore_skipnodes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t' ORDER BY 1, 2, 3; + stripe | attr | block | ?column? | ?column? +--------+------+-------+----------+---------- + 0 | 1 | 0 | t | t + 0 | 2 | 0 | f | f + 1 | 1 | 0 | t | t + 1 | 2 | 0 | f | f + 2 | 1 | 0 | t | t + 2 | 2 | 0 | f | f +(6 rows) + diff --git a/sql/am_vacuum.sql b/sql/am_vacuum.sql new file mode 100644 index 000000000..070a13b05 --- /dev/null +++ b/sql/am_vacuum.sql @@ -0,0 +1,37 @@ +CREATE TABLE t(a int, b int) USING cstore_tableam; +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + +INSERT INTO t SELECT i, i * i FROM generate_series(1, 10) i; +INSERT INTO t SELECT i, i * i FROM generate_series(11, 20) i; +INSERT INTO t SELECT i, i * i FROM generate_series(21, 30) i; + +SELECT sum(a), sum(b) FROM t; +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + +-- vacuum full should merge stripes together +VACUUM FULL t; + +SELECT sum(a), sum(b) FROM t; +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + +-- test the case when all data cannot fit into a single stripe +SET cstore.stripe_row_count TO 1000; +INSERT INTO t SELECT i, 2 * i FROM generate_series(1,2500) i; + +SELECT sum(a), sum(b) FROM t; +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + +VACUUM FULL t; + +SELECT sum(a), sum(b) FROM t; +SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; + +-- VACUUM FULL doesn't reclaim dropped columns, but converts them to NULLs +ALTER TABLE t DROP COLUMN a; + +SELECT stripe, attr, block, minimum_value IS NULL, maximum_value IS NULL FROM cstore.cstore_skipnodes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t' ORDER BY 1, 2, 3; + +VACUUM FULL t; + +SELECT stripe, attr, block, minimum_value IS NULL, maximum_value IS NULL FROM cstore.cstore_skipnodes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t' ORDER BY 1, 2, 3; + From eeb25aca856142b6395a515730863ea0f32a80fe Mon Sep 17 00:00:00 2001 From: Hadi Moshayedi Date: Mon, 5 Oct 2020 10:28:50 -0700 Subject: [PATCH 2/3] Add a test which checks for resource clean-up --- expected/am_vacuum.out | 16 ++++++++++++++++ sql/am_vacuum.sql | 10 ++++++++++ 2 files changed, 26 insertions(+) diff --git a/expected/am_vacuum.out b/expected/am_vacuum.out index d689be800..3abb3c668 100644 --- a/expected/am_vacuum.out +++ b/expected/am_vacuum.out @@ -1,3 +1,4 @@ +SELECT count(*) AS columnar_table_count FROM cstore.cstore_tables \gset CREATE TABLE t(a int, b int) USING cstore_tableam; SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; count @@ -87,3 +88,18 @@ SELECT stripe, attr, block, minimum_value IS NULL, maximum_value IS NULL FROM cs 2 | 2 | 0 | f | f (6 rows) +-- Make sure we cleaned-up the transient table metadata after VACUUM FULL commands +SELECT count(*) - :columnar_table_count FROM cstore.cstore_tables; + ?column? +---------- + 1 +(1 row) + +DROP TABLE t; +-- Make sure we cleaned the metadata for t too +SELECT count(*) - :columnar_table_count FROM cstore.cstore_tables; + ?column? +---------- + 0 +(1 row) + diff --git a/sql/am_vacuum.sql b/sql/am_vacuum.sql index 070a13b05..6a5e0687e 100644 --- a/sql/am_vacuum.sql +++ b/sql/am_vacuum.sql @@ -1,4 +1,7 @@ +SELECT count(*) AS columnar_table_count FROM cstore.cstore_tables \gset + CREATE TABLE t(a int, b int) USING cstore_tableam; + SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; INSERT INTO t SELECT i, i * i FROM generate_series(1, 10) i; @@ -35,3 +38,10 @@ VACUUM FULL t; SELECT stripe, attr, block, minimum_value IS NULL, maximum_value IS NULL FROM cstore.cstore_skipnodes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t' ORDER BY 1, 2, 3; +-- Make sure we cleaned-up the transient table metadata after VACUUM FULL commands +SELECT count(*) - :columnar_table_count FROM cstore.cstore_tables; + +DROP TABLE t; + +-- Make sure we cleaned the metadata for t too +SELECT count(*) - :columnar_table_count FROM cstore.cstore_tables; From aa3032cfdd90c1edab6393c234174828a8238007 Mon Sep 17 00:00:00 2001 From: Hadi Moshayedi Date: Mon, 5 Oct 2020 10:30:02 -0700 Subject: [PATCH 3/3] Address feedback --- cstore_tableam.c | 38 ++++++++++---------------------------- expected/am_vacuum.out | 6 +++--- sql/am_vacuum.sql | 6 +++--- 3 files changed, 16 insertions(+), 34 deletions(-) diff --git a/cstore_tableam.c b/cstore_tableam.c index 337dbe06f..39a0695e2 100644 --- a/cstore_tableam.c +++ b/cstore_tableam.c @@ -143,8 +143,8 @@ RelationColumnList(Relation rel) Index varno = 0; AttrNumber varattno = i + 1; Oid vartype = tupdesc->attrs[i].atttypid; - int32 vartypmod = 0; - Oid varcollid = 0; + int32 vartypmod = tupdesc->attrs[i].atttypmod; + Oid varcollid = tupdesc->attrs[i].attcollation; Index varlevelsup = 0; Var *var; @@ -530,16 +530,14 @@ cstore_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap, TableWriteState *writeState = NULL; TableReadState *readState = NULL; CStoreOptions *cstoreOptions = NULL; - Datum *sourceValues = NULL; - bool *sourceNulls = NULL; - Datum *targetValues = NULL; - bool *targetNulls = NULL; + Datum *values = NULL; + bool *nulls = NULL; TupleDesc sourceDesc = RelationGetDescr(OldHeap); TupleDesc targetDesc = RelationGetDescr(NewHeap); if (OldIndex != NULL || use_sort) { - ereport(ERROR, (errmsg("cstore_am doesn't support indexes"))); + ereport(ERROR, (errmsg(CSTORE_TABLEAM_NAME " doesn't support indexes"))); } /* @@ -559,34 +557,18 @@ cstore_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap, readState = CStoreBeginRead(OldHeap, sourceDesc, RelationColumnList(OldHeap), NULL); - sourceValues = palloc0(sourceDesc->natts * sizeof(Datum)); - sourceNulls = palloc0(sourceDesc->natts * sizeof(bool)); - - targetValues = palloc0(targetDesc->natts * sizeof(Datum)); - targetNulls = palloc0(targetDesc->natts * sizeof(bool)); + values = palloc0(sourceDesc->natts * sizeof(Datum)); + nulls = palloc0(sourceDesc->natts * sizeof(bool)); *num_tuples = 0; - while (CStoreReadNextRow(readState, sourceValues, sourceNulls)) + while (CStoreReadNextRow(readState, values, nulls)) { - memset(targetNulls, true, targetDesc->natts * sizeof(bool)); - - for (int attrIndex = 0; attrIndex < sourceDesc->natts; attrIndex++) - { - FormData_pg_attribute *sourceAttr = TupleDescAttr(sourceDesc, attrIndex); - - if (!sourceAttr->attisdropped) - { - targetNulls[attrIndex] = sourceNulls[attrIndex]; - targetValues[attrIndex] = sourceValues[attrIndex]; - } - } - - CStoreWriteRow(writeState, targetValues, targetNulls); + CStoreWriteRow(writeState, values, nulls); (*num_tuples)++; } - *tups_vacuumed = *num_tuples; + *tups_vacuumed = 0; CStoreEndWrite(writeState); CStoreEndRead(readState); diff --git a/expected/am_vacuum.out b/expected/am_vacuum.out index 3abb3c668..dbeddca2b 100644 --- a/expected/am_vacuum.out +++ b/expected/am_vacuum.out @@ -1,4 +1,4 @@ -SELECT count(*) AS columnar_table_count FROM cstore.cstore_tables \gset +SELECT count(*) AS columnar_table_count FROM cstore.cstore_data_files \gset CREATE TABLE t(a int, b int) USING cstore_tableam; SELECT count(*) FROM cstore.cstore_stripes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t'; count @@ -89,7 +89,7 @@ SELECT stripe, attr, block, minimum_value IS NULL, maximum_value IS NULL FROM cs (6 rows) -- Make sure we cleaned-up the transient table metadata after VACUUM FULL commands -SELECT count(*) - :columnar_table_count FROM cstore.cstore_tables; +SELECT count(*) - :columnar_table_count FROM cstore.cstore_data_files; ?column? ---------- 1 @@ -97,7 +97,7 @@ SELECT count(*) - :columnar_table_count FROM cstore.cstore_tables; DROP TABLE t; -- Make sure we cleaned the metadata for t too -SELECT count(*) - :columnar_table_count FROM cstore.cstore_tables; +SELECT count(*) - :columnar_table_count FROM cstore.cstore_data_files; ?column? ---------- 0 diff --git a/sql/am_vacuum.sql b/sql/am_vacuum.sql index 6a5e0687e..8cb70167d 100644 --- a/sql/am_vacuum.sql +++ b/sql/am_vacuum.sql @@ -1,4 +1,4 @@ -SELECT count(*) AS columnar_table_count FROM cstore.cstore_tables \gset +SELECT count(*) AS columnar_table_count FROM cstore.cstore_data_files \gset CREATE TABLE t(a int, b int) USING cstore_tableam; @@ -39,9 +39,9 @@ VACUUM FULL t; SELECT stripe, attr, block, minimum_value IS NULL, maximum_value IS NULL FROM cstore.cstore_skipnodes a, pg_class b WHERE a.relfilenode=b.relfilenode AND b.relname='t' ORDER BY 1, 2, 3; -- Make sure we cleaned-up the transient table metadata after VACUUM FULL commands -SELECT count(*) - :columnar_table_count FROM cstore.cstore_tables; +SELECT count(*) - :columnar_table_count FROM cstore.cstore_data_files; DROP TABLE t; -- Make sure we cleaned the metadata for t too -SELECT count(*) - :columnar_table_count FROM cstore.cstore_tables; +SELECT count(*) - :columnar_table_count FROM cstore.cstore_data_files;