Repository: incubator-hawq Updated Branches: refs/heads/master f0e95fc4e -> aaa2c8cdf
HAWQ-1618. Segment panic at workfile_mgr_close_file() when transaction ROLLBACK Project: http://git-wip-us.apache.org/repos/asf/incubator-hawq/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-hawq/commit/aaa2c8cd Tree: http://git-wip-us.apache.org/repos/asf/incubator-hawq/tree/aaa2c8cd Diff: http://git-wip-us.apache.org/repos/asf/incubator-hawq/diff/aaa2c8cd Branch: refs/heads/master Commit: aaa2c8cdf289d0cf50e025cf5eef2197871f4951 Parents: f0e95fc Author: interma <inte...@outlook.com> Authored: Wed May 30 14:26:01 2018 +0800 Committer: rlei <r...@pivotal.io> Committed: Wed Jun 6 16:53:37 2018 +0800 ---------------------------------------------------------------------- src/backend/executor/execWorkfile.c | 53 ++++----- src/backend/executor/test/Makefile | 2 +- src/backend/executor/test/execWorkfile_test.c | 119 --------------------- src/backend/utils/sort/tuplestorenew.c | 90 +++++++--------- 4 files changed, 59 insertions(+), 205 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/aaa2c8cd/src/backend/executor/execWorkfile.c ---------------------------------------------------------------------- diff --git a/src/backend/executor/execWorkfile.c b/src/backend/executor/execWorkfile.c index 2e68efe..7e8ec58 100644 --- a/src/backend/executor/execWorkfile.c +++ b/src/backend/executor/execWorkfile.c @@ -60,8 +60,8 @@ ExecWorkFile_Create(const char *fileName, bool delOnClose, int compressType) { - ExecWorkFile *workfile = NULL; - void *file = NULL; + ExecWorkFile *workfile; + void *file; /* Before creating a new file, let's check the limit on number of workfile created */ if (!WorkfileQueryspace_AddWorkfile()) @@ -70,14 +70,6 @@ ExecWorkFile_Create(const char *fileName, workfile_mgr_report_error(); } - /* - * Create ExecWorkFile in the TopMemoryContext since this memory context - * is still available when calling the transaction callback at the - * time when the transaction aborts. - */ - MemoryContext oldContext = MemoryContextSwitchTo(TopMemoryContext); - - switch(fileType) { case BUFFILE: @@ -88,10 +80,10 @@ ExecWorkFile_Create(const char *fileName, file = (void *)bfz_create(fileName, delOnClose, compressType); break; default: - ereport(LOG, + ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("invalid work file type: %d", fileType))); - Assert(false); + return NULL; /* keep compiler quiet */ } workfile = palloc0(sizeof(ExecWorkFile)); @@ -102,8 +94,6 @@ ExecWorkFile_Create(const char *fileName, workfile->size = 0; ExecWorkFile_SetFlags(workfile, delOnClose, true /* created */); - MemoryContextSwitchTo(oldContext); - return workfile; } @@ -158,9 +148,9 @@ ExecWorkFile_Open(const char *fileName, bool delOnClose, int compressType) { - ExecWorkFile *workfile = NULL; - void *file = NULL; - int64 file_size = 0; + ExecWorkFile *workfile; + void *file; + int64 file_size; switch(fileType) { @@ -170,32 +160,30 @@ ExecWorkFile_Open(const char *fileName, delOnClose, true /* interXact */ ); if (!file) - { - elog(ERROR, "could not open temporary file \"%s\": %m", fileName); - } + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not open temporary file \"%s\": %m", + fileName))); + BufFileSetWorkfile(file); file_size = BufFileGetSize(file); - break; + case BFZ: file = (void *)bfz_open(fileName, delOnClose, compressType); if (!file) - { - elog(ERROR, "could not open temporary file \"%s\": %m", fileName); - } + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not open temporary file \"%s\": %m", + fileName))); file_size = bfz_totalbytes((bfz_t *)file); break; + default: - ereport(LOG, + ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("invalid work file type: %d", fileType))); - Assert(false); - } - - /* Failed opening existing workfile. Inform the caller */ - if (NULL == file) - { - return NULL; + return NULL; /* keep compiler quiet */ } workfile = palloc0(sizeof(ExecWorkFile)); @@ -219,6 +207,7 @@ ExecWorkFile_Open(const char *fileName, */ bool ExecWorkFile_Write(ExecWorkFile *workfile, + void *data, uint64 size) { http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/aaa2c8cd/src/backend/executor/test/Makefile ---------------------------------------------------------------------- diff --git a/src/backend/executor/test/Makefile b/src/backend/executor/test/Makefile index 8f84984..a7d4124 100644 --- a/src/backend/executor/test/Makefile +++ b/src/backend/executor/test/Makefile @@ -18,7 +18,7 @@ subdir=src/backend/executor top_builddir=../../../.. -TARGETS=nodeSubplan nodeShareInputScan execAmi execWorkfile execHHashagg +TARGETS=nodeSubplan nodeShareInputScan execAmi execHHashagg # Objects from backend, which don't need to be mocked but need to be linked. nodeSubplan_REAL_OBJS=\ http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/aaa2c8cd/src/backend/executor/test/execWorkfile_test.c ---------------------------------------------------------------------- diff --git a/src/backend/executor/test/execWorkfile_test.c b/src/backend/executor/test/execWorkfile_test.c deleted file mode 100644 index 5a279df..0000000 --- a/src/backend/executor/test/execWorkfile_test.c +++ /dev/null @@ -1,119 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -#include <stdarg.h> -#include <stddef.h> -#include <setjmp.h> -#include "cmockery.h" - -#include "c.h" -#include "postgres.h" -#include "storage/buffile.h" -#include "storage/bfz.h" -#include "executor/execWorkfile.h" -#include "utils/memutils.h" - -/* Ignore elog */ -#include "utils/elog.h" - -#undef elog -#define elog - -/* Provide specialized mock implementations for memory allocation functions */ -#undef palloc0 -#define palloc0 execWorkfile_palloc0_mock -void *execWorkfile_palloc0_mock(Size size); - -#undef pstrdup -#define pstrdup execWorkfile_pstrdup_mock -char *execWorkfile_pstrdup_mock(const char *string); - -#include "../execWorkfile.c" - -/* - * This is a mocked version of palloc0 to be used in ExecWorkFile_Create. - * It asserts that it is executed in the TopMemoryContext. - */ -void * -execWorkfile_palloc0_mock(Size size) -{ - assert_int_equal(CurrentMemoryContext, TopMemoryContext); - return MemoryContextAllocZero(CurrentMemoryContext, size); -} - -/* - * This is a mocked version of pstrdup to be used in ExecWorkFile_Create. - * It asserts that it is executed in the TopMemoryContext. - */ -char *execWorkfile_pstrdup_mock(const char *string) -{ - assert_int_equal(CurrentMemoryContext, TopMemoryContext); - return MemoryContextStrdup(CurrentMemoryContext, string); -} - - -/* ==================== ExecWorkFile_Create ==================== */ -/* - * Test that the ExecWorkFile struct is allocated in TopMemoryContext - */ -void -test__ExecWorkFile_Create__InTopMemContext(void **state) -{ - - char *test_filename = "foo"; - - will_return(WorkfileQueryspace_AddWorkfile, true); - - expect_value(BufFileCreateFile, fileName, test_filename); - expect_value(BufFileCreateFile, delOnClose, true); - expect_value(BufFileCreateFile, interXact, false); - will_return(BufFileCreateFile, NULL); - - expect_value(BufFileSetWorkfile, buffile, NULL); - will_be_called(BufFileSetWorkfile); - - /* - * All the memory context stuff is mocked, so the TopMemoryContext is NULL - * at this point. Set it to something specific so we can distinguish it from - * the CurrentMemoryContext. - */ - TopMemoryContext = (MemoryContext) 0xdeadbeef; - CurrentMemoryContext = (MemoryContext) 0xfeadbead; - - /* - * ExecWorkFile_Create will call our mocked palloc0 function execWorkfile__palloc0_mock - * and our mocked pstrdup function execWorkfile_pstrdup_mock. - * These functions will assert that the allocation of the result happens - * in the TopMemoryContext. - */ - ExecWorkFile *ewf = ExecWorkFile_Create(test_filename, BUFFILE, true /* delOnClose */, 0 /* compressType */); - -} - -/* ==================== main ==================== */ -int -main(int argc, char* argv[]) -{ - cmockery_parse_arguments(argc, argv); - - const UnitTest tests[] = { - unit_test(test__ExecWorkFile_Create__InTopMemContext) - }; - - return run_tests(tests); -} http://git-wip-us.apache.org/repos/asf/incubator-hawq/blob/aaa2c8cd/src/backend/utils/sort/tuplestorenew.c ---------------------------------------------------------------------- diff --git a/src/backend/utils/sort/tuplestorenew.c b/src/backend/utils/sort/tuplestorenew.c index f88a3e2..7d3cb4a 100644 --- a/src/backend/utils/sort/tuplestorenew.c +++ b/src/backend/utils/sort/tuplestorenew.c @@ -191,6 +191,8 @@ struct NTupleStore List *accessors; /* all current accessors of the store */ bool fwacc; /* if I had already has a write acc */ + MemoryContext mcxt; /* memory context holding this tuplestore, and the page structs */ + /* instrumentation for explain analyze */ Instrumentation *instrument; }; @@ -383,21 +385,9 @@ static void nts_return_free_page(NTupleStore *nts, NTupleStorePage *page) nts->first_free_page = NTS_PREPEND_1(nts->first_free_page, page); } -static inline void *check_malloc(int size) -{ - void *ptr = gp_malloc(size); - if(!ptr) - ereport(ERROR, - (errcode(ERRCODE_OUT_OF_MEMORY), - errmsg("NTupleStore failed to malloc: out of memory"))); - return ptr; -} - /* Get a free page. Will shrink if necessary. * The page returned still belong to the store (accounted by nts->page_cnt), but it is * not on any list. Caller is responsible to put it back onto a list. - * - * Page is allocated/freed with gcc malloc/free, not palloc/pfree. */ static NTupleStorePage *nts_get_free_page(NTupleStore *nts) { @@ -464,7 +454,7 @@ static NTupleStorePage *nts_get_free_page(NTupleStore *nts) if(nts->page_cnt >= page_max) { - gp_free2(page_next, sizeof(NTupleStorePage)); + pfree(page_next); --nts->page_cnt; } else @@ -486,7 +476,7 @@ static NTupleStorePage *nts_get_free_page(NTupleStore *nts) } Assert(page_next == NULL && nts->page_cnt < page_max); - page = (NTupleStorePage *) check_malloc(sizeof(NTupleStorePage)); + page = (NTupleStorePage *) MemoryContextAlloc(nts->mcxt, sizeof(NTupleStorePage)); init_page(page); ++nts->page_cnt; @@ -614,30 +604,24 @@ static NTupleStorePage *nts_load_prev_page(NTupleStore *store, NTupleStorePage * } } -static void ntuplestore_cleanup(NTupleStore *ts, bool fNormal, bool canReportError) +void +ntuplestore_destroy(NTupleStore *ts) { NTupleStorePage *p = ts->first_page; + ListCell *cell; - /* normal case: for each accessor, we mark it has no owning store. - * This do not need to, actually, cannot be called in error out case, - * because the memory context of ts->accessor has already been - * cleaned up - */ - if(fNormal) + /* For each accessor, we mark it has no owning store. */ + foreach(cell, ts->accessors) { - ListCell *cell; - foreach(cell, ts->accessors) - { - NTupleStoreAccessor *acc = (NTupleStoreAccessor *) lfirst(cell); - acc->store = NULL; - acc->page = NULL; - } + NTupleStoreAccessor *acc = (NTupleStoreAccessor *) lfirst(cell); + acc->store = NULL; + acc->page = NULL; } while(p) { NTupleStorePage *pnext = nts_page_next(p); - gp_free2(p, sizeof(NTupleStorePage)); + pfree(p); p = pnext; } @@ -645,18 +629,18 @@ static void ntuplestore_cleanup(NTupleStore *ts, bool fNormal, bool canReportErr while(p) { NTupleStorePage *pnext = nts_page_next(p); - gp_free2(p, sizeof(NTupleStorePage)); + pfree(p); p = pnext; } if(ts->pfile) { - workfile_mgr_close_file(ts->work_set, ts->pfile, canReportError); + workfile_mgr_close_file(ts->work_set, ts->pfile, false); ts->pfile = NULL; } if(ts->plobfile) { - workfile_mgr_close_file(ts->work_set, ts->plobfile, canReportError); + workfile_mgr_close_file(ts->work_set, ts->plobfile, false); ts->plobfile = NULL; } @@ -666,18 +650,14 @@ static void ntuplestore_cleanup(NTupleStore *ts, bool fNormal, bool canReportErr ts->work_set = NULL; } - gp_free2(ts, sizeof(NTupleStore)); -} - -static void XCallBack_NTS(XactEvent event, void *nts) -{ - ntuplestore_cleanup((NTupleStore *)nts, false, (event!=XACT_EVENT_ABORT)); + pfree(ts); } NTupleStore * ntuplestore_create(int maxBytes) { - NTupleStore *store = (NTupleStore *) check_malloc(sizeof(NTupleStore)); + NTupleStore *store = (NTupleStore *) palloc(sizeof(NTupleStore)); + store->mcxt = CurrentMemoryContext; store->pfile = NULL; store->first_ondisk_blockn = 0; @@ -696,7 +676,7 @@ ntuplestore_create(int maxBytes) if(store->page_max < 16) store->page_max = 16; - store->first_page = (NTupleStorePage *) check_malloc(sizeof(NTupleStorePage)); + store->first_page = (NTupleStorePage *) MemoryContextAlloc(store->mcxt, sizeof(NTupleStorePage)); init_page(store->first_page); nts_page_set_blockn(store->first_page, 0); @@ -715,7 +695,6 @@ ntuplestore_create(int maxBytes) store->instrument = NULL; - RegisterXactCallbackOnce(XCallBack_NTS, (void *) store); return store; } @@ -749,7 +728,8 @@ ntuplestore_create_readerwriter(const char *filename, int maxBytes, bool isWrite } else { - store = (NTupleStore *) check_malloc(sizeof(NTupleStore)); + store = (NTupleStore *) palloc(sizeof(NTupleStore)); + store->mcxt = CurrentMemoryContext; store->work_set = NULL; store->cached_workfiles_found = false; store->cached_workfiles_loaded = false; @@ -764,7 +744,6 @@ ntuplestore_create_readerwriter(const char *filename, int maxBytes, bool isWrite 0 /* compressType */); ntuplestore_init_reader(store, maxBytes); - RegisterXactCallbackOnce(XCallBack_NTS, (void *) store); } return store; } @@ -791,7 +770,7 @@ ntuplestore_init_reader(NTupleStore *store, int maxBytes) if(store->page_max < 16) store->page_max = 16; - store->first_page = (NTupleStorePage *) check_malloc(sizeof(NTupleStorePage)); + store->first_page = (NTupleStorePage *) MemoryContextAlloc(store->mcxt, sizeof(NTupleStorePage)); init_page(store->first_page); bool fOK = ntsReadBlock(store, 0, store->first_page); @@ -923,17 +902,19 @@ ntuplestore_flush(NTupleStore *ts) } } -void -ntuplestore_destroy(NTupleStore *ts) -{ - UnregisterXactCallbackOnce(XCallBack_NTS, (void *) ts); - ntuplestore_cleanup(ts, true, true); -} - NTupleStoreAccessor* ntuplestore_create_accessor(NTupleStore *ts, bool isWriter) { - NTupleStoreAccessor *acc = (NTupleStoreAccessor *) palloc(sizeof(NTupleStoreAccessor)); + NTupleStoreAccessor *acc; + MemoryContext oldcxt; + + /* + * The accessor is allocated in the same memory context as the tuplestore + * itself. + */ + oldcxt = MemoryContextSwitchTo(ts->mcxt); + + acc = (NTupleStoreAccessor *) palloc(sizeof(NTupleStoreAccessor)); acc->store = ts; acc->isWriter = isWriter; @@ -952,6 +933,8 @@ ntuplestore_create_accessor(NTupleStore *ts, bool isWriter) if(isWriter) ts->fwacc = true; + MemoryContextSwitchTo(oldcxt); + return acc; } @@ -1338,6 +1321,7 @@ bool ntuplestore_acc_current_tupleslot(NTupleStoreAccessor *tsa, TupleTableSlot { NTupleStoreLobRef *plobref = (NTupleStoreLobRef *) tuple; Assert(len == -(int)sizeof(NTupleStoreLobRef)); + tuple = (MemTuple) palloc(plobref->size); len = ntuplestore_get_lob(tsa->store, tuple, plobref); @@ -1370,7 +1354,7 @@ bool ntuplestore_acc_current_data(NTupleStoreAccessor *tsa, void **data, int *le { if (tsa->tmp_lob) pfree(tsa->tmp_lob); - tsa->tmp_lob = palloc(plobref->size); + tsa->tmp_lob = MemoryContextAlloc(tsa->store->mcxt, plobref->size); tsa->tmp_len = plobref->size; }
