From 7fcecde20302a966aabdd85329c9e431a79549c2 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Thu, 5 Aug 2021 18:48:26 +0300 Subject: [PATCH 1/3] Use init_columnar_read_state instead of lower level func Funtionally, this doesn't change anything. This is just a preparation before next commit. --- src/backend/columnar/columnar_tableam.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/backend/columnar/columnar_tableam.c b/src/backend/columnar/columnar_tableam.c index 532613dd4..8b6151b29 100644 --- a/src/backend/columnar/columnar_tableam.c +++ b/src/backend/columnar/columnar_tableam.c @@ -795,10 +795,12 @@ columnar_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap, /* we need all columns */ int natts = OldHeap->rd_att->natts; Bitmapset *attr_needed = bms_add_range(NULL, 0, natts - 1); - List *projectedColumnList = NeededColumnsList(sourceDesc, attr_needed); - ColumnarReadState *readState = ColumnarBeginRead(OldHeap, sourceDesc, - projectedColumnList, - NULL); + + /* no quals for table rewrite */ + List *scanQual = NIL; + + ColumnarReadState *readState = init_columnar_read_state(OldHeap, sourceDesc, + attr_needed, scanQual); Datum *values = palloc0(sourceDesc->natts * sizeof(Datum)); bool *nulls = palloc0(sourceDesc->natts * sizeof(bool)); From b3d9fc91f804c3f3cd96ce2af5bae7a46993b235 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Thu, 5 Aug 2021 19:27:10 +0300 Subject: [PATCH 2/3] Always use right mem cxt when creating ColumnarReadState All the callers except columnar_relation_copy_for_cluster were already switching to right memory context when creating ColumnarReadState. With this commit, we embed that logic into init_columnar_read_state to avoid further such bugs. That way, we start using the right memory context for columnar_relation_copy_for_cluster too. --- src/backend/columnar/columnar_tableam.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/backend/columnar/columnar_tableam.c b/src/backend/columnar/columnar_tableam.c index 8b6151b29..f167ec41b 100644 --- a/src/backend/columnar/columnar_tableam.c +++ b/src/backend/columnar/columnar_tableam.c @@ -253,12 +253,16 @@ CreateColumnarScanMemoryContext(void) */ static ColumnarReadState * init_columnar_read_state(Relation relation, TupleDesc tupdesc, Bitmapset *attr_needed, - List *scanQual) + List *scanQual, MemoryContext scanContext) { + MemoryContext oldContext = MemoryContextSwitchTo(scanContext); + List *neededColumnList = NeededColumnsList(tupdesc, attr_needed); ColumnarReadState *readState = ColumnarBeginRead(relation, tupdesc, neededColumnList, scanQual); + MemoryContextSwitchTo(oldContext); + return readState; } @@ -302,11 +306,10 @@ columnar_getnextslot(TableScanDesc sscan, ScanDirection direction, TupleTableSlo */ if (scan->cs_readState == NULL) { - MemoryContext oldContext = MemoryContextSwitchTo(scan->scanContext); scan->cs_readState = init_columnar_read_state(scan->cs_base.rs_rd, slot->tts_tupleDescriptor, - scan->attr_needed, scan->scanQual); - MemoryContextSwitchTo(oldContext); + scan->attr_needed, scan->scanQual, + scan->scanContext); } ExecClearTuple(slot); @@ -490,8 +493,6 @@ columnar_index_fetch_tuple(struct IndexFetchTableData *sscan, /* initialize read state for the first row */ if (scan->cs_readState == NULL) { - MemoryContext oldContext = MemoryContextSwitchTo(scan->scanContext); - /* we need all columns */ int natts = columnarRelation->rd_att->natts; Bitmapset *attr_needed = bms_add_range(NULL, 0, natts - 1); @@ -501,8 +502,8 @@ columnar_index_fetch_tuple(struct IndexFetchTableData *sscan, scan->cs_readState = init_columnar_read_state(columnarRelation, slot->tts_tupleDescriptor, - attr_needed, scanQual); - MemoryContextSwitchTo(oldContext); + attr_needed, scanQual, + scan->scanContext); } uint64 rowNumber = tid_to_row_number(*tid); @@ -799,8 +800,10 @@ columnar_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap, /* no quals for table rewrite */ List *scanQual = NIL; + MemoryContext scanContext = CreateColumnarScanMemoryContext(); ColumnarReadState *readState = init_columnar_read_state(OldHeap, sourceDesc, - attr_needed, scanQual); + attr_needed, scanQual, + scanContext); Datum *values = palloc0(sourceDesc->natts * sizeof(Datum)); bool *nulls = palloc0(sourceDesc->natts * sizeof(bool)); From 68f46c5dc9081a28fd5566829a3a2ca1a04bc624 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Thu, 5 Aug 2021 19:43:13 +0300 Subject: [PATCH 3/3] Use scan context for intermediate mem allocs too --- src/backend/columnar/columnar_reader.c | 19 ++++++++++++++++++- src/backend/columnar/columnar_tableam.c | 2 +- src/include/columnar/columnar.h | 3 ++- src/test/regress/expected/columnar_alter.out | 15 +++++++++++++++ src/test/regress/sql/columnar_alter.sql | 10 ++++++++++ 5 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/backend/columnar/columnar_reader.c b/src/backend/columnar/columnar_reader.c index cfd052ac2..96efdbffc 100644 --- a/src/backend/columnar/columnar_reader.c +++ b/src/backend/columnar/columnar_reader.c @@ -81,6 +81,13 @@ struct ColumnarReadState MemoryContext stripeReadContext; int64 chunkGroupsFiltered; + + /* + * Memory context guaranteed to be not freed during scan so we can + * safely use for any memory allocations regarding ColumnarReadState + * itself. + */ + MemoryContext scanContext; }; /* static function declarations */ @@ -159,7 +166,8 @@ static Datum ColumnDefaultValue(TupleConstr *tupleConstraints, */ ColumnarReadState * ColumnarBeginRead(Relation relation, TupleDesc tupleDescriptor, - List *projectedColumnList, List *whereClauseList) + List *projectedColumnList, List *whereClauseList, + MemoryContext scanContext) { /* * We allocate all stripe specific data in the stripeReadContext, and reset @@ -180,6 +188,7 @@ ColumnarBeginRead(Relation relation, TupleDesc tupleDescriptor, readState->currentStripeMetadata = FindNextStripeByRowNumber(relation, COLUMNAR_INVALID_ROW_NUMBER, GetTransactionSnapshot()); + readState->scanContext = scanContext; return readState; } @@ -432,11 +441,15 @@ HasUnreadStripe(ColumnarReadState *readState) void ColumnarRescan(ColumnarReadState *readState) { + MemoryContext oldContext = MemoryContextSwitchTo(readState->scanContext); + ColumnarResetRead(readState); readState->currentStripeMetadata = FindNextStripeByRowNumber(readState->relation, COLUMNAR_INVALID_ROW_NUMBER, GetTransactionSnapshot()); readState->chunkGroupsFiltered = 0; + + MemoryContextSwitchTo(oldContext); } @@ -518,6 +531,8 @@ BeginStripeRead(StripeMetadata *stripeMetadata, Relation rel, TupleDesc tupleDes static void AdvanceStripeRead(ColumnarReadState *readState) { + MemoryContext oldContext = MemoryContextSwitchTo(readState->scanContext); + readState->chunkGroupsFiltered += readState->stripeReadState->chunkGroupsFiltered; @@ -528,6 +543,8 @@ AdvanceStripeRead(ColumnarReadState *readState) GetTransactionSnapshot()); readState->stripeReadState = NULL; MemoryContextReset(readState->stripeReadContext); + + MemoryContextSwitchTo(oldContext); } diff --git a/src/backend/columnar/columnar_tableam.c b/src/backend/columnar/columnar_tableam.c index f167ec41b..6630d0430 100644 --- a/src/backend/columnar/columnar_tableam.c +++ b/src/backend/columnar/columnar_tableam.c @@ -259,7 +259,7 @@ init_columnar_read_state(Relation relation, TupleDesc tupdesc, Bitmapset *attr_n List *neededColumnList = NeededColumnsList(tupdesc, attr_needed); ColumnarReadState *readState = ColumnarBeginRead(relation, tupdesc, neededColumnList, - scanQual); + scanQual, scanContext); MemoryContextSwitchTo(oldContext); diff --git a/src/include/columnar/columnar.h b/src/include/columnar/columnar.h index eaeb9fa21..a44072ffe 100644 --- a/src/include/columnar/columnar.h +++ b/src/include/columnar/columnar.h @@ -212,7 +212,8 @@ extern MemoryContext ColumnarWritePerTupleContext(ColumnarWriteState *state); extern ColumnarReadState * ColumnarBeginRead(Relation relation, TupleDesc tupleDescriptor, List *projectedColumnList, - List *qualConditions); + List *qualConditions, + MemoryContext scanContext); extern bool ColumnarReadNextRow(ColumnarReadState *state, Datum *columnValues, bool *columnNulls, uint64 *rowNumber); extern void ColumnarRescan(ColumnarReadState *readState); diff --git a/src/test/regress/expected/columnar_alter.out b/src/test/regress/expected/columnar_alter.out index 707107568..2cf31a86b 100644 --- a/src/test/regress/expected/columnar_alter.out +++ b/src/test/regress/expected/columnar_alter.out @@ -218,6 +218,21 @@ SELECT * FROM domain_test; 1 | 2 | x (2 rows) +-- similar to "add column c str_domain DEFAULT 'x'", both were getting +-- stucked before fixing https://github.com/citusdata/citus/issues/5164 +BEGIN; + ALTER TABLE domain_test ADD COLUMN d INT DEFAULT random(); +ROLLBACK; +BEGIN; + ALTER TABLE domain_test ADD COLUMN d SERIAL; + SELECT * FROM domain_test ORDER BY 1,2,3,4; + a | b | c | d +--------------------------------------------------------------------- + 1 | 2 | x | 1 + 1 | 2 | x | 2 +(2 rows) + +ROLLBACK; set default_table_access_method TO 'columnar'; CREATE TABLE has_volatile AS SELECT * FROM generate_series(1,10) id; diff --git a/src/test/regress/sql/columnar_alter.sql b/src/test/regress/sql/columnar_alter.sql index ecd647fe1..7872773d7 100644 --- a/src/test/regress/sql/columnar_alter.sql +++ b/src/test/regress/sql/columnar_alter.sql @@ -116,6 +116,16 @@ alter table domain_test add column c str_domain; alter table domain_test add column c str_domain DEFAULT 'x'; SELECT * FROM domain_test; +-- similar to "add column c str_domain DEFAULT 'x'", both were getting +-- stucked before fixing https://github.com/citusdata/citus/issues/5164 +BEGIN; + ALTER TABLE domain_test ADD COLUMN d INT DEFAULT random(); +ROLLBACK; +BEGIN; + ALTER TABLE domain_test ADD COLUMN d SERIAL; + SELECT * FROM domain_test ORDER BY 1,2,3,4; +ROLLBACK; + set default_table_access_method TO 'columnar'; CREATE TABLE has_volatile AS SELECT * FROM generate_series(1,10) id;