From 30ab5a3b8aa140444cafab5d0f86c86b1b53b1ae Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Wed, 10 Jan 2018 23:26:47 +0100
Subject: [PATCH 2/2] Avoid silently changing volatilty in create function

A function cannot be declared both immutable and volatile, the
command will error out on conflicting options:

  create function int42(cstring) returns int42 AS 'int4in'
  language internal strict immutable volatile;

The following statement would however silently create an immutable
function due to iscachable in the with() clause being handled last
and overwriting the previous volatility setting.

  create function int42(cstring) returns int42 AS 'int4in'
  language internal strict volatile with (isstrict, iscachable);

With this patch, compute_attributes_with_style() checks if volatility
has been defined elsewhere in the statement and errors out in case
conflicting options are detected.
---
 src/backend/commands/functioncmds.c             | 27 ++++++++++++++++++++-----
 src/test/regress/expected/create_function_3.out |  3 +++
 src/test/regress/sql/create_function_3.sql      |  2 ++
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 8eed5a0166..2291b00180 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -650,7 +650,8 @@ compute_attributes_sql_style(ParseState *pstate,
 							 ArrayType **proconfig,
 							 float4 *procost,
 							 float4 *prorows,
-							 char *parallel_p)
+							 char *parallel_p,
+							 bool *has_volatile)
 {
 	ListCell   *option;
 	DefElem    *as_item = NULL;
@@ -758,7 +759,12 @@ compute_attributes_sql_style(ParseState *pstate,
 	if (windowfunc_item)
 		*windowfunc_p = intVal(windowfunc_item->arg);
 	if (volatility_item)
+	{
+		*has_volatile = true;
 		*volatility_p = interpret_func_volatility(volatility_item);
+	}
+	else
+		*has_volatile = false;
 	if (strict_item)
 		*strict_p = intVal(strict_item->arg);
 	if (security_item)
@@ -790,7 +796,8 @@ compute_attributes_sql_style(ParseState *pstate,
 
 /*-------------
  *	 Interpret the parameters *parameters and return their contents via
- *	 *isStrict_p and *volatility_p.
+ *	 *isStrict_p and *volatility_p. If has_volatile is true then volatility
+ *	 has been defined already in the syntax.
  *
  *	These parameters supply optional information about a function.
  *	All have defaults if not specified. Parameters:
@@ -803,7 +810,7 @@ compute_attributes_sql_style(ParseState *pstate,
  *------------
  */
 static void
-compute_attributes_with_style(ParseState *pstate, bool is_procedure, List *parameters, bool *isStrict_p, char *volatility_p)
+compute_attributes_with_style(ParseState *pstate, bool is_procedure, List *parameters, bool *isStrict_p, char *volatility_p, bool has_volatile)
 {
 	ListCell   *pl;
 
@@ -834,8 +841,15 @@ compute_attributes_with_style(ParseState *pstate, bool is_procedure, List *param
 						(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
 						 errmsg("invalid attribute in procedure definition"),
 						 parser_errposition(pstate, param->location)));
+
 			if (defGetBoolean(param))
+			{
+				if (has_volatile && *volatility_p != PROVOLATILE_IMMUTABLE)
+					ereport(ERROR,
+							(errcode(ERRCODE_SYNTAX_ERROR),
+							 errmsg("conflicting or redundant options")));
 				*volatility_p = PROVOLATILE_IMMUTABLE;
+			}
 		}
 		else
 			ereport(WARNING,
@@ -943,6 +957,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
 				isStrict,
 				security,
 				isLeakProof;
+	bool		hasVolatile;
 	char		volatility;
 	ArrayType  *proconfig;
 	float4		procost;
@@ -980,7 +995,8 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
 								 &as_clause, &language, &transformDefElem,
 								 &isWindowFunc, &volatility,
 								 &isStrict, &security, &isLeakProof,
-								 &proconfig, &procost, &prorows, &parallel);
+								 &proconfig, &procost, &prorows, &parallel,
+								 &hasVolatile);
 
 	/* Look up the language and validate permissions */
 	languageTuple = SearchSysCache1(LANGNAME, PointerGetDatum(language));
@@ -1112,7 +1128,8 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
 		trftypes = NULL;
 	}
 
-	compute_attributes_with_style(pstate, stmt->is_procedure, stmt->withClause, &isStrict, &volatility);
+	compute_attributes_with_style(pstate, stmt->is_procedure, stmt->withClause,
+								  &isStrict, &volatility, hasVolatile);
 
 	interpret_AS_clause(languageOid, language, funcname, as_clause,
 						&prosrc_str, &probin_str);
diff --git a/src/test/regress/expected/create_function_3.out b/src/test/regress/expected/create_function_3.out
index b5e19485e5..3d522c974c 100644
--- a/src/test/regress/expected/create_function_3.out
+++ b/src/test/regress/expected/create_function_3.out
@@ -66,6 +66,9 @@ SELECT proname, provolatile FROM pg_proc
  functest_b_4 | v
 (4 rows)
 
+CREATE FUNCTION functest_B_5(int) RETURNS bool LANGUAGE 'sql'
+		VOLATILE AS 'SELECT $1 < 0' WITH (iscachable);
+ERROR:  conflicting or redundant options
 --
 -- SECURITY DEFINER | INVOKER
 --
diff --git a/src/test/regress/sql/create_function_3.sql b/src/test/regress/sql/create_function_3.sql
index 0a0e407aab..187454b643 100644
--- a/src/test/regress/sql/create_function_3.sql
+++ b/src/test/regress/sql/create_function_3.sql
@@ -48,6 +48,8 @@ SELECT proname, provolatile FROM pg_proc
                      'functest_B_2'::regproc,
                      'functest_B_3'::regproc,
 		     'functest_B_4'::regproc) ORDER BY proname;
+CREATE FUNCTION functest_B_5(int) RETURNS bool LANGUAGE 'sql'
+		VOLATILE AS 'SELECT $1 < 0' WITH (iscachable);
 
 --
 -- SECURITY DEFINER | INVOKER
-- 
2.14.1.145.gb3622a4ee

