From e520d50cd80141f4fc26472436837199965bcba6 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 21 Mar 2016 10:56:07 +0200 Subject: [PATCH] Remove unnecessary 'need_free' argument from defGetString(). All of the callers are in places where leaking a few bytes of memory to the current memory context will do no harm. Either parsing, or processing a DDL command, or planning. So let's simplify the callers by removing the argument. That makes the code match the upstream again, which makes merging easier. These changes were originally made to reduce the memory consumption when doing parse analysis on a heavily partitioned table, but the previous commit provided a more whole-sale solution for that, so we don't need to nickel-and-dime every allocation anymore. --- src/backend/access/common/reloptions.c | 15 +--- src/backend/commands/aggregatecmds.c | 5 +- src/backend/commands/define.c | 32 ++------- src/backend/commands/queue.c | 5 +- src/backend/commands/tablecmds.c | 18 ++--- src/backend/commands/test/Makefile | 2 +- src/backend/commands/test/define_test.c | 72 ------------------- src/backend/commands/typecmds.c | 9 ++- src/backend/gpopt/gpdbwrappers.cpp | 5 +- .../gpopt/translate/CTranslatorQueryToDXL.cpp | 6 +- src/backend/parser/analyze.c | 58 ++------------- src/backend/parser/parse_partition.c | 20 +----- src/include/commands/defrem.h | 2 +- src/include/gpopt/gpdbwrappers.h | 2 +- 14 files changed, 33 insertions(+), 218 deletions(-) delete mode 100644 src/backend/commands/test/define_test.c diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c index 2abeb92c2e..e433fb6ca3 100644 --- a/src/backend/access/common/reloptions.c +++ b/src/backend/access/common/reloptions.c @@ -298,29 +298,16 @@ transformRelOptions(Datum oldOptions, List *defList, * have just "name", assume "name=true" is meant. */ - bool need_free_value = false; if (def->arg != NULL) - { - value = defGetString(def, &need_free_value); - } + value = defGetString(def); else - { value = "true"; - } len = VARHDRSZ + strlen(def->defname) + 1 + strlen(value); /* +1 leaves room for sprintf's trailing null */ t = (text *) palloc(len + 1); SET_VARSIZE(t, len); sprintf(VARDATA(t), "%s=%s", def->defname, value); - if (need_free_value) - { - pfree(value); - value = NULL; - } - - AssertImply(need_free_value, NULL == value); - astate = accumArrayResult(astate, PointerGetDatum(t), false, TEXTOID, CurrentMemoryContext); diff --git a/src/backend/commands/aggregatecmds.c b/src/backend/commands/aggregatecmds.c index 8ec8443369..4085207a05 100644 --- a/src/backend/commands/aggregatecmds.c +++ b/src/backend/commands/aggregatecmds.c @@ -67,7 +67,6 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters, Oid transTypeId; ListCell *pl; Oid aggOid; - bool need_free_value = false; /* Convert list of names to a name and namespace */ aggNamespace = QualifiedNameGetCreationNamespace(name, &aggName); @@ -101,9 +100,9 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters, else if (pg_strcasecmp(defel->defname, "stype1") == 0) transType = defGetTypeName(defel); else if (pg_strcasecmp(defel->defname, "initcond") == 0) - initval = defGetString(defel, &need_free_value); + initval = defGetString(defel); else if (pg_strcasecmp(defel->defname, "initcond1") == 0) - initval = defGetString(defel, &need_free_value); + initval = defGetString(defel); else if (pg_strcasecmp(defel->defname, "prefunc") == 0) /* MPP */ prelimfuncName = defGetQualifiedName(defel); else diff --git a/src/backend/commands/define.c b/src/backend/commands/define.c index 882b91fa1c..981352be08 100644 --- a/src/backend/commands/define.c +++ b/src/backend/commands/define.c @@ -59,14 +59,13 @@ case_translate_language_name(const char *input) * Extract a string value (otherwise uninterpreted) from a DefElem. */ char * -defGetString(DefElem *def, bool *need_free) +defGetString(DefElem *def) { if (def->arg == NULL) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s requires a parameter", def->defname))); - Assert(NULL != need_free); switch (nodeTag(def->arg)) { case T_Integer: @@ -74,7 +73,6 @@ defGetString(DefElem *def, bool *need_free) char *str = palloc(32); snprintf(str, 32, "%ld", (long) intVal(def->arg)); - *need_free = true; return str; } case T_Float: @@ -83,16 +81,12 @@ defGetString(DefElem *def, bool *need_free) * T_Float values are kept in string form, so this type cheat * works (and doesn't risk losing precision) */ - *need_free = false; return strVal(def->arg); case T_String: - *need_free = false; return strVal(def->arg); case T_TypeName: - *need_free = true; return TypeNameToString((TypeName *) def->arg); case T_List: - *need_free = true; return NameListToString((List *) def->arg); default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(def->arg)); @@ -157,26 +151,13 @@ defGetBoolean(DefElem *def) break; default: { - bool need_free_value = false; - char *sval = defGetString(def, &need_free_value); - bool result = false; + char *sval = defGetString(def); + if (pg_strcasecmp(sval, "true") == 0) - { - result = true; - } + return true; if (pg_strcasecmp(sval, "false") == 0) - { - result = false; - } - if (need_free_value) - { - pfree(sval); - sval = NULL; - } - - AssertImply(need_free_value, NULL == sval); + return false; - return result; } break; } @@ -317,11 +298,10 @@ defGetTypeLength(DefElem *def) default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(def->arg)); } - bool need_free_value = false; ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("invalid argument for %s: \"%s\"", - def->defname, defGetString(def, &need_free_value)))); + def->defname, defGetString(def)))); return 0; /* keep compiler quiet */ } diff --git a/src/backend/commands/queue.c b/src/backend/commands/queue.c index b03ac3c341..9b3e47c1a7 100644 --- a/src/backend/commands/queue.c +++ b/src/backend/commands/queue.c @@ -347,10 +347,7 @@ AlterResqueueCapabilityEntry( /* WITHOUT clause value determined in pg_resourcetype */ if (!bWithout) - { - bool need_free_value = false; - pStrVal = makeString(defGetString(defel, &need_free_value)); - } + pStrVal = makeString(defGetString(defel)); else { pStrVal = NULL; /* if NULL, delete entry from diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index eb0adc8f08..d05fd73c64 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -12206,7 +12206,6 @@ new_rel_opts(Relation rel, List *lwith) { Datum newOptions = PointerGetDatum(NULL); bool make_heap = false; - bool need_free_value = false; if (lwith && list_length(lwith)) { ListCell *lc; @@ -12219,7 +12218,7 @@ new_rel_opts(Relation rel, List *lwith) { DefElem *e = lfirst(lc); if (pg_strcasecmp(e->defname, "appendonly") == 0 && - pg_strcasecmp(defGetString(e, &need_free_value), "false") == 0) + pg_strcasecmp(defGetString(e), "false") == 0) { make_heap = true; break; @@ -17922,8 +17921,7 @@ static Datum transformFormatOpts(char formattype, List *formatOpts, int numcols, { DefElem *defel = (DefElem *) lfirst(option); char *key = defel->defname; - bool need_free_value = false; - char *val = defGetString(defel, &need_free_value); + char *val = defGetString(defel); if (strcmp(key, "formatter") == 0) { @@ -17934,22 +17932,14 @@ static Datum transformFormatOpts(char formattype, List *formatOpts, int numcols, formatter = strVal(defel->arg); } - + /* MPP-14467 - replace any space chars with meta char */ resetStringInfo(&key_modified); appendStringInfoString(&key_modified, key); replaceStringInfoString(&key_modified, " ", ""); - + sprintf((char *) format_str + len, "%s '%s' ", key_modified.data, val); len += strlen(key_modified.data) + strlen(val) + 4; - - if (need_free_value) - { - pfree(val); - val = NULL; - } - - AssertImply(need_free_value, NULL == val); if (len > maxlen) ereport(ERROR, diff --git a/src/backend/commands/test/Makefile b/src/backend/commands/test/Makefile index cefe444c3a..e0da0c50d7 100644 --- a/src/backend/commands/test/Makefile +++ b/src/backend/commands/test/Makefile @@ -2,6 +2,6 @@ subdir=src/backend/commands top_builddir=../../../.. include $(top_builddir)/src/Makefile.global -TARGETS=tablecmds define +TARGETS=tablecmds include $(top_builddir)/src/backend/mock.mk diff --git a/src/backend/commands/test/define_test.c b/src/backend/commands/test/define_test.c deleted file mode 100644 index 80f31188ca..0000000000 --- a/src/backend/commands/test/define_test.c +++ /dev/null @@ -1,72 +0,0 @@ -#include -#include -#include -#include "cmockery.h" - -#include "../define.c" - -/* ==================== FreeStrFromDefGetString ==================== */ - -/* - * Tests that need_free flag correctly set in defGetString method. - */ -void -test__DefGetString_NeedFreeFlag(void **state) -{ - bool need_free = false; - char *value = NULL; - - /* case: T_String expected value: false */ - need_free = true; - DefElem *e1 = makeDefElem("def_string", (Node *) makeString("none")); - value = defGetString(e1, &need_free); - assert_false(need_free); - - /* case: T_Integer expected value: true */ - need_free = false; - DefElem *e2 = makeDefElem("def_int", (Node *) makeInteger(0)); - value = defGetString(e2, &need_free); - assert_true(need_free); - - /* case: T_Float expected value: false */ - need_free = true; - DefElem *e3 = makeDefElem("def_float", (Node *) makeFloat("3.14")); - value = defGetString(e3, &need_free); - assert_false(need_free); - - /* case: T_TypeName expected value: true */ - need_free = false; - TypeName *tName = makeNode(TypeName); - tName->names = list_make2(makeString("pg_catalog"), makeString("unknown")); - tName->typemod = -1; - tName->location = -1; - DefElem *e4 = makeDefElem("def_typename", (Node *) tName); - value = defGetString(e4, &need_free); - assert_true(need_free); - - /* case: T_List expected value: true */ - need_free = false; - List *list = NIL; - list = lappend(list, makeString("str1")); - list = lappend(list, makeString("str2")); - DefElem *e5 = makeDefElem("def_list", (Node *) list); - value = defGetString(e5, &need_free); - assert_true(need_free); - -} - -/* ==================== main ==================== */ -int -main(int argc, char* argv[]) -{ - cmockery_parse_arguments(argc, argv); - - const UnitTest tests[] = { - unit_test(test__DefGetString_NeedFreeFlag) - }; - - MemoryContextInit(); - - return run_tests(tests); -} - diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index c788bee488..4b7c1e156a 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -133,7 +133,6 @@ DefineType(List *names, List *parameters, Oid newOid, Oid shadowOid) Oid resulttype; Datum typoptions = 0; List *encoding = NIL; - bool need_free_value = false; Relation pg_type; /* Convert list of names to a name and namespace */ @@ -236,7 +235,7 @@ DefineType(List *names, List *parameters, Oid newOid, Oid shadowOid) analyzeName = defGetQualifiedName(defel); else if (pg_strcasecmp(defel->defname, "delimiter") == 0) { - char *p = defGetString(defel, &need_free_value); + char *p = defGetString(defel); delimiter = p[0]; } @@ -251,12 +250,12 @@ DefineType(List *names, List *parameters, Oid newOid, Oid shadowOid) format_type_be(elemType)))); } else if (pg_strcasecmp(defel->defname, "default") == 0) - defaultValue = defGetString(defel, &need_free_value); + defaultValue = defGetString(defel); else if (pg_strcasecmp(defel->defname, "passedbyvalue") == 0) byValue = defGetBoolean(defel); else if (pg_strcasecmp(defel->defname, "alignment") == 0) { - char *a = defGetString(defel, &need_free_value); + char *a = defGetString(defel); /* * Note: if argument was an unquoted identifier, parser will have @@ -283,7 +282,7 @@ DefineType(List *names, List *parameters, Oid newOid, Oid shadowOid) } else if (pg_strcasecmp(defel->defname, "storage") == 0) { - char *a = defGetString(defel, &need_free_value); + char *a = defGetString(defel); if (pg_strcasecmp(a, "plain") == 0) storage = 'p'; diff --git a/src/backend/gpopt/gpdbwrappers.cpp b/src/backend/gpopt/gpdbwrappers.cpp index 0d27d6fd4e..36eebc9dd6 100644 --- a/src/backend/gpopt/gpdbwrappers.cpp +++ b/src/backend/gpopt/gpdbwrappers.cpp @@ -2969,13 +2969,12 @@ gpdb::FInterpretOidsOption char * gpdb::SzDefGetString ( - DefElem *pdefelem, - bool *fNeedFree + DefElem *pdefelem ) { GP_WRAP_START; { - return defGetString(pdefelem, fNeedFree); + return defGetString(pdefelem); } GP_WRAP_END; return NULL; diff --git a/src/backend/gpopt/translate/CTranslatorQueryToDXL.cpp b/src/backend/gpopt/translate/CTranslatorQueryToDXL.cpp index 11904f096b..f4d15185a5 100644 --- a/src/backend/gpopt/translate/CTranslatorQueryToDXL.cpp +++ b/src/backend/gpopt/translate/CTranslatorQueryToDXL.cpp @@ -1005,14 +1005,10 @@ CTranslatorQueryToDXL::PstrExtractOptionValue GPOS_ASSERT(NULL != pdefelem); BOOL fNeedsFree = false; - CHAR *szValue = gpdb::SzDefGetString(pdefelem, &fNeedsFree); + CHAR *szValue = gpdb::SzDefGetString(pdefelem); CWStringDynamic *pstrResult = CDXLUtils::PstrFromSz(m_pmp, szValue); - if (fNeedsFree) - { - gpdb::GPDBFree(szValue); - } return pstrResult; } diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index fa5d0182ad..c7a3a32af0 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1298,8 +1298,7 @@ co_explicitly_disabled(List *opts) continue; } - bool need_free_arg = false; - arg = defGetString(el, &need_free_arg); + arg = defGetString(el); bool result = false; if (pg_strcasecmp("appendonly", el->defname) == 0 && pg_strcasecmp("false", arg) == 0) @@ -1311,13 +1310,6 @@ co_explicitly_disabled(List *opts) { result = true; } - if (need_free_arg) - { - pfree(arg); - arg = NULL; - } - - AssertImply(need_free_arg, NULL == arg); if (result) { @@ -1352,8 +1344,7 @@ is_aocs(List *opts) continue; } - bool need_free_arg = false; - arg = defGetString(el, &need_free_arg); + arg = defGetString(el); if (pg_strcasecmp("appendonly", el->defname) == 0) { @@ -1366,13 +1357,6 @@ is_aocs(List *opts) found_cs = true; csvalue = pg_strcasecmp("column", arg) == 0; } - if (need_free_arg) - { - pfree(arg); - arg = NULL; - } - - AssertImply(need_free_arg, NULL == arg); } if (!found_ao) aovalue = isDefaultAO(); @@ -1411,30 +1395,11 @@ encodings_overlap(List *a, List *b, bool test_conflicts) } else { - bool need_free_ela = false; - bool need_free_elb = false; - char *ela_str = defGetString(ela, &need_free_ela); - char *elb_str = defGetString(elb, &need_free_elb); - int result = pg_strcasecmp(ela_str,elb_str); - // free ela_str, elb_str if it is initialized via TypeNameToString - if (need_free_ela) - { - pfree(ela_str); - ela_str = NULL; - } - if (need_free_elb) - { - pfree(elb_str); - elb_str = NULL; - } + char *ela_str = defGetString(ela); + char *elb_str = defGetString(elb); - AssertImply(need_free_ela, NULL == ela_str); - AssertImply(need_free_elb, NULL == elb_str); - - if (result != 0) - { + if (pg_strcasecmp(ela_str, elb_str) != 0) return true; - } } } else @@ -3160,7 +3125,6 @@ fillin_encoding(List *list) bool foundCompressType = false; bool foundCompressTypeNone = false; char *cmplevel = NULL; - bool need_free_cmplevel = false; bool foundBlockSize = false; char *arg; List *retList = list; @@ -3175,21 +3139,13 @@ fillin_encoding(List *list) if (pg_strcasecmp("compresstype", el->defname) == 0) { foundCompressType = true; - bool need_free_arg = false; - arg = defGetString(el, &need_free_arg); + arg = defGetString(el); if (pg_strcasecmp("none", arg) == 0) foundCompressTypeNone = true; - if (need_free_arg) - { - pfree(arg); - arg = NULL; - } - - AssertImply(need_free_arg, NULL == arg); } else if (pg_strcasecmp("compresslevel", el->defname) == 0) { - cmplevel = defGetString(el, &need_free_cmplevel); + cmplevel = defGetString(el); } else if (pg_strcasecmp("blocksize", el->defname) == 0) foundBlockSize = true; diff --git a/src/backend/parser/parse_partition.c b/src/backend/parser/parse_partition.c index c5e009ef67..1dfa8327b9 100644 --- a/src/backend/parser/parse_partition.c +++ b/src/backend/parser/parse_partition.c @@ -717,17 +717,9 @@ transformPartitionBy(ParseState *pstate, CreateStmtContext *cxt, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("invalid tablename specification"))); - bool need_free_value = false; - char *relname_str = defGetString(pDef, &need_free_value); + char *relname_str = defGetString(pDef); relname = pstrdup(relname_str); - if (need_free_value) - { - pfree(relname_str); - relname_str = NULL; - } - - AssertImply(need_free_value, NULL == relname_str); prtstr[0] = '\0'; pWithList = list_delete_cell(pWithList, def_lc, prev_lc); @@ -3610,17 +3602,9 @@ partition_range_every(ParseState *pstate, PartitionBy *pBy, List *coltypes, errmsg("invalid tablename specification"))); bTablename = true; - bool need_free_value = false; - char *widthname_str = defGetString(pDef, &need_free_value); + char *widthname_str = defGetString(pDef); pBSpec->pWithTnameStr = pstrdup(widthname_str); - if (need_free_value) - { - pfree(widthname_str); - widthname_str = NULL; - } - - AssertImply(need_free_value, NULL == widthname_str); pWithList = list_delete_cell(pWithList, def_lc, prev_lc); ((AlterPartitionCmd *) pStoreAttr)->arg1 = diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index 188b83a61b..1062ee407c 100644 --- a/src/include/commands/defrem.h +++ b/src/include/commands/defrem.h @@ -101,7 +101,7 @@ extern void AlterOpFamilyOwner(List *name, const char *access_method, Oid newOwn extern char *case_translate_language_name(const char *input); -extern char *defGetString(DefElem *def, bool *need_free); +extern char *defGetString(DefElem *def); extern double defGetNumeric(DefElem *def); extern bool defGetBoolean(DefElem *def); extern int64 defGetInt64(DefElem *def); diff --git a/src/include/gpopt/gpdbwrappers.h b/src/include/gpopt/gpdbwrappers.h index 7473fe1f36..ee64f5ad53 100644 --- a/src/include/gpopt/gpdbwrappers.h +++ b/src/include/gpopt/gpdbwrappers.h @@ -596,7 +596,7 @@ namespace gpdb { bool FInterpretOidsOption(List *plOptions); // extract string value from defelem's value - char *SzDefGetString(DefElem *pdefelem, bool *fNeedFree); + char *SzDefGetString(DefElem *pdefelem); // fold array expression constant values Node *PnodeFoldArrayexprConstants(ArrayExpr *parrayexpr); -- GitLab