From f21cbe68f8c7ab9f730de3bbf775c292b9134ac9 Mon Sep 17 00:00:00 2001 From: Ying Xu <32597660+yxu2162@users.noreply.github.com> Date: Wed, 5 Oct 2022 11:42:09 -0700 Subject: [PATCH] [Columnar] Bugfix for Columnar: options ignored during ALTER TABLE rewrite (#6337) DESCRIPTION: Fixes a bug that prevents retaining columnar table options after a table-rewrite A fix for this issue: Columnar: options ignored during ALTER TABLE rewrite #5927 The OID for the temporary table created during ALTER TABLE was not the same as the original table's OID so the columnar options were not being applied during rewrite. The change is that I applied the original table's columnar options to the new table so that it has the correct options during write. I also added a test. --- src/backend/columnar/columnar_tableam.c | 8 ++++++ src/backend/columnar/write_state_management.c | 12 ++++++++- src/include/columnar/columnar.h | 1 + .../expected/columnar_alter_set_type.out | 25 +++++++++++++++++++ .../regress/sql/columnar_alter_set_type.sql | 14 +++++++++++ 5 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/backend/columnar/columnar_tableam.c b/src/backend/columnar/columnar_tableam.c index b8e94b774..4f1815103 100644 --- a/src/backend/columnar/columnar_tableam.c +++ b/src/backend/columnar/columnar_tableam.c @@ -739,7 +739,9 @@ columnar_tuple_insert(Relation relation, TupleTableSlot *slot, CommandId cid, */ ColumnarWriteState *writeState = columnar_init_write_state(relation, RelationGetDescr(relation), + slot->tts_tableOid, GetCurrentSubTransactionId()); + MemoryContext oldContext = MemoryContextSwitchTo(ColumnarWritePerTupleContext( writeState)); @@ -781,8 +783,14 @@ columnar_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, { CheckCitusColumnarVersion(ERROR); + /* + * The callback to .multi_insert is table_multi_insert() and this is only used for the COPY + * command, so slot[i]->tts_tableoid will always be equal to relation->id. Thus, we can send + * RelationGetRelid(relation) as the tupSlotTableOid + */ ColumnarWriteState *writeState = columnar_init_write_state(relation, RelationGetDescr(relation), + RelationGetRelid(relation), GetCurrentSubTransactionId()); ColumnarCheckLogicalReplication(relation); diff --git a/src/backend/columnar/write_state_management.c b/src/backend/columnar/write_state_management.c index 5ad849cac..e3bfc9260 100644 --- a/src/backend/columnar/write_state_management.c +++ b/src/backend/columnar/write_state_management.c @@ -113,6 +113,7 @@ CleanupWriteStateMap(void *arg) ColumnarWriteState * columnar_init_write_state(Relation relation, TupleDesc tupdesc, + Oid tupSlotRelationId, SubTransactionId currentSubXid) { bool found; @@ -176,7 +177,16 @@ columnar_init_write_state(Relation relation, TupleDesc tupdesc, MemoryContext oldContext = MemoryContextSwitchTo(WriteStateContext); ColumnarOptions columnarOptions = { 0 }; - ReadColumnarOptions(relation->rd_id, &columnarOptions); + + /* + * In case of a table rewrite, we need to fetch table options based on the + * relation id of the source tuple slot. + * + * For this reason, we always pass tupSlotRelationId here; which should be + * same as the target table if the write operation is not related to a table + * rewrite etc. + */ + ReadColumnarOptions(tupSlotRelationId, &columnarOptions); SubXidWriteState *stackEntry = palloc0(sizeof(SubXidWriteState)); stackEntry->writeState = ColumnarBeginWrite(relation->rd_node, diff --git a/src/include/columnar/columnar.h b/src/include/columnar/columnar.h index 7b2924576..13195b1d6 100644 --- a/src/include/columnar/columnar.h +++ b/src/include/columnar/columnar.h @@ -314,6 +314,7 @@ extern Datum columnar_relation_storageid(PG_FUNCTION_ARGS); /* write_state_management.c */ extern ColumnarWriteState * columnar_init_write_state(Relation relation, TupleDesc tupdesc, + Oid tupSlotRelationId, SubTransactionId currentSubXid); extern void FlushWriteStateForRelfilenode(Oid relfilenode, SubTransactionId currentSubXid); diff --git a/src/test/regress/expected/columnar_alter_set_type.out b/src/test/regress/expected/columnar_alter_set_type.out index 9368c5850..9672a8e3c 100644 --- a/src/test/regress/expected/columnar_alter_set_type.out +++ b/src/test/regress/expected/columnar_alter_set_type.out @@ -50,3 +50,28 @@ SELECT * FROM test_alter_table ORDER BY a; (4 rows) DROP TABLE test_alter_table; +-- Make sure that the correct table options are used when rewriting the table. +-- This is reflected by the VACUUM VERBOSE output right after a rewrite showing +-- that all chunks are compressed with the configured compression algorithm +-- https://github.com/citusdata/citus/issues/5927 +CREATE TABLE test(i int) USING columnar; +ALTER TABLE test SET (columnar.compression = lz4); +INSERT INTO test VALUES(1); +VACUUM VERBOSE test; +INFO: statistics for "test": +storage id: xxxxx +total file size: 24576, total data size: 6 +compression rate: 0.83x +total row count: 1, stripe count: 1, average rows per stripe: 1 +chunk count: 1, containing data for dropped columns: 0, lz4 compressed: 1 + +ALTER TABLE test ALTER COLUMN i TYPE int8; +VACUUM VERBOSE test; +INFO: statistics for "test": +storage id: xxxxx +total file size: 24576, total data size: 10 +compression rate: 0.90x +total row count: 1, stripe count: 1, average rows per stripe: 1 +chunk count: 1, containing data for dropped columns: 0, lz4 compressed: 1 + +DROP TABLE test; diff --git a/src/test/regress/sql/columnar_alter_set_type.sql b/src/test/regress/sql/columnar_alter_set_type.sql index f6bafbc4f..c7a8e7ea0 100644 --- a/src/test/regress/sql/columnar_alter_set_type.sql +++ b/src/test/regress/sql/columnar_alter_set_type.sql @@ -29,3 +29,17 @@ ALTER TABLE test_alter_table ALTER COLUMN b TYPE float USING (b::float + 0.5); SELECT * FROM test_alter_table ORDER BY a; DROP TABLE test_alter_table; + +-- Make sure that the correct table options are used when rewriting the table. +-- This is reflected by the VACUUM VERBOSE output right after a rewrite showing +-- that all chunks are compressed with the configured compression algorithm +-- https://github.com/citusdata/citus/issues/5927 +CREATE TABLE test(i int) USING columnar; +ALTER TABLE test SET (columnar.compression = lz4); +INSERT INTO test VALUES(1); +VACUUM VERBOSE test; + +ALTER TABLE test ALTER COLUMN i TYPE int8; +VACUUM VERBOSE test; + +DROP TABLE test;