From d3757ff15dce157f72586093af64a75cd59063ac Mon Sep 17 00:00:00 2001 From: Ying Xu <32597660+yxu2162@users.noreply.github.com> Date: Sun, 9 Oct 2022 22:30:59 -0700 Subject: [PATCH] [Columnar] 11.0 Cherry-Pick Bugfix for Columnar: options ignored during ALTER TABLE (#6410) 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. cherry-pick from commit f21cbe68f8c7ab9f730de3bbf775c292b9134ac9 --- 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 | 30 +++++++++++++++++++ .../regress/sql/columnar_alter_set_type.sql | 14 +++++++++ 5 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/backend/columnar/columnar_tableam.c b/src/backend/columnar/columnar_tableam.c index 4a3ce8387..635731eb8 100644 --- a/src/backend/columnar/columnar_tableam.c +++ b/src/backend/columnar/columnar_tableam.c @@ -742,7 +742,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)); @@ -784,8 +786,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 201a1a479..609489294 100644 --- a/src/backend/columnar/write_state_management.c +++ b/src/backend/columnar/write_state_management.c @@ -117,6 +117,7 @@ CleanupWriteStateMap(void *arg) ColumnarWriteState * columnar_init_write_state(Relation relation, TupleDesc tupdesc, + Oid tupSlotRelationId, SubTransactionId currentSubXid) { bool found; @@ -180,7 +181,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 5195cbfee..4a9696d4f 100644 --- a/src/include/columnar/columnar.h +++ b/src/include/columnar/columnar.h @@ -302,6 +302,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..be092e26d 100644 --- a/src/test/regress/expected/columnar_alter_set_type.out +++ b/src/test/regress/expected/columnar_alter_set_type.out @@ -50,3 +50,33 @@ 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; +SELECT alter_columnar_table_set('test', compression => 'lz4'); + alter_columnar_table_set +--------------------------------------------------------------------- + +(1 row) + +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..d3494f466 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; +SELECT alter_columnar_table_set('test', compression => 'lz4'); +INSERT INTO test VALUES(1); +VACUUM VERBOSE test; + +ALTER TABLE test ALTER COLUMN i TYPE int8; +VACUUM VERBOSE test; + +DROP TABLE test;