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

@@ -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);

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

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 */
 }
 

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

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, " ", "<gpx20>");
-			
+
 			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,

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

src/backend/commands/test/define_test.c

-#include <stdarg.h>
-#include <stddef.h>
-#include <setjmp.h>
-#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);
-}
-

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';

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;

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;
 }

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;

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 =

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);

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);