public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v5] SQL-standard function body
16+ messages / 5 participants
[nested] [flat]
* [PATCH v5] SQL-standard function body
@ 2020-10-27 13:40 Peter Eisentraut <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Peter Eisentraut @ 2020-10-27 13:40 UTC (permalink / raw)
This adds support for writing CREATE FUNCTION and CREATE PROCEDURE
statements for language SQL with a function body that conforms to the
SQL standard and is portable to other implementations.
Instead of the PostgreSQL-specific AS $$ string literal $$ syntax,
this allows writing out the SQL statements making up the body
unquoted, either as a single statement:
CREATE FUNCTION add(a integer, b integer) RETURNS integer
LANGUAGE SQL
RETURN a + b;
or as a block
CREATE PROCEDURE insert_data(a integer, b integer)
LANGUAGE SQL
BEGIN ATOMIC
INSERT INTO tbl VALUES (a);
INSERT INTO tbl VALUES (b);
END;
The function body is parsed at function definition time and stored as
expression nodes in a new pg_proc column prosqlbody. So at run time,
no further parsing is required.
However, this form does not support polymorphic arguments, because
there is no more parse analysis done at call time.
Dependencies between the function and the objects it uses are fully
tracked.
A new RETURN statement is introduced. This can only be used inside
function bodies. Internally, it is treated much like a SELECT
statement.
psql needs some new intelligence to keep track of function body
boundaries so that it doesn't send off statements when it sees
semicolons that are inside a function body.
Also, per SQL standard, LANGUAGE SQL is the default, so it does not
need to be specified anymore.
Discussion: https://www.postgresql.org/message-id/flat/[email protected]
---
doc/src/sgml/catalogs.sgml | 10 +
doc/src/sgml/ref/create_function.sgml | 126 +++++++++--
doc/src/sgml/ref/create_procedure.sgml | 62 ++++-
src/backend/catalog/pg_aggregate.c | 1 +
src/backend/catalog/pg_proc.c | 116 ++++++----
src/backend/commands/aggregatecmds.c | 2 +
src/backend/commands/functioncmds.c | 119 ++++++++--
src/backend/commands/typecmds.c | 1 +
src/backend/executor/functions.c | 79 ++++---
src/backend/nodes/copyfuncs.c | 15 ++
src/backend/nodes/equalfuncs.c | 13 ++
src/backend/nodes/outfuncs.c | 12 +
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/util/clauses.c | 126 +++++++----
src/backend/parser/analyze.c | 35 +++
src/backend/parser/gram.y | 129 ++++++++---
src/backend/tcop/postgres.c | 3 +-
src/backend/utils/adt/ruleutils.c | 106 ++++++++-
src/bin/pg_dump/pg_dump.c | 45 +++-
src/bin/psql/describe.c | 15 +-
src/fe_utils/psqlscan.l | 23 +-
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_proc.h | 6 +-
src/include/commands/defrem.h | 2 +
src/include/executor/functions.h | 15 ++
src/include/fe_utils/psqlscan_int.h | 2 +
src/include/nodes/nodes.h | 1 +
src/include/nodes/parsenodes.h | 13 ++
src/include/parser/kwlist.h | 2 +
src/include/tcop/tcopprot.h | 1 +
src/interfaces/ecpg/preproc/ecpg.addons | 6 +
src/interfaces/ecpg/preproc/ecpg.trailer | 4 +-
.../regress/expected/create_function_3.out | 212 +++++++++++++++++-
.../regress/expected/create_procedure.out | 58 +++++
src/test/regress/sql/create_function_3.sql | 99 ++++++++
src/test/regress/sql/create_procedure.sql | 26 +++
36 files changed, 1286 insertions(+), 204 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 5bd54cb218..a8ae9594e6 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -5972,6 +5972,16 @@ <title><structname>pg_proc</structname> Columns</title>
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>prosqlbody</structfield> <type>pg_node_tree</type>
+ </para>
+ <para>
+ Pre-parsed SQL function body. This will be used for language SQL
+ functions if the body is not specified as a string constant.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>proconfig</structfield> <type>text[]</type>
diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml
index 3c1eaea651..1b5b9420db 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -38,6 +38,7 @@
| SET <replaceable class="parameter">configuration_parameter</replaceable> { TO <replaceable class="parameter">value</replaceable> | = <replaceable class="parameter">value</replaceable> | FROM CURRENT }
| AS '<replaceable class="parameter">definition</replaceable>'
| AS '<replaceable class="parameter">obj_file</replaceable>', '<replaceable class="parameter">link_symbol</replaceable>'
+ | <replaceable class="parameter">sql_body</replaceable>
} ...
</synopsis>
</refsynopsisdiv>
@@ -257,8 +258,9 @@ <title>Parameters</title>
The name of the language that the function is implemented in.
It can be <literal>sql</literal>, <literal>c</literal>,
<literal>internal</literal>, or the name of a user-defined
- procedural language, e.g., <literal>plpgsql</literal>. Enclosing the
- name in single quotes is deprecated and requires matching case.
+ procedural language, e.g., <literal>plpgsql</literal>. The default is
+ <literal>sql</literal>. Enclosing the name in single quotes is
+ deprecated and requires matching case.
</para>
</listitem>
</varlistentry>
@@ -577,6 +579,44 @@ <title>Parameters</title>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><replaceable class="parameter">sql_body</replaceable></term>
+
+ <listitem>
+ <para>
+ The body of a <literal>LANGUAGE SQL</literal> function. This can
+ either be a single statement
+<programlisting>
+RETURN <replaceable>expression</replaceable>
+</programlisting>
+ or a block
+<programlisting>
+BEGIN ATOMIC
+ <replaceable>statement</replaceable>;
+ <replaceable>statement</replaceable>;
+ ...
+ <replaceable>statement</replaceable>;
+END
+</programlisting>
+ </para>
+
+ <para>
+ This is similar to writing the text of the function body as a string
+ constant (see <replaceable>definition</replaceable> above), but there
+ are some differences: This form only works for <literal>LANGUAGE
+ SQL</literal>, the string constant form works for all languages. This
+ form is parsed at function definition time, the string constant form is
+ parsed at execution time; therefore this form cannot support
+ polymorphic argument types and other constructs that are not resolvable
+ at function definition time. This form tracks dependencies between the
+ function and objects used in the function body, so <literal>DROP
+ ... CASCADE</literal> will work correctly, whereas the form using
+ string literals may leave dangling functions. Finally, this form is
+ more compatible with the SQL standard and other SQL implementations.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
<para>
@@ -669,6 +709,15 @@ <title>Examples</title>
LANGUAGE SQL
IMMUTABLE
RETURNS NULL ON NULL INPUT;
+</programlisting>
+ The same function written in a more SQL-conforming style, using argument
+ names and an unquoted body:
+<programlisting>
+CREATE FUNCTION add(a integer, b integer) RETURNS integer
+ LANGUAGE SQL
+ IMMUTABLE
+ RETURNS NULL ON NULL INPUT
+ RETURN a + b;
</programlisting>
</para>
@@ -799,23 +848,74 @@ <title>Writing <literal>SECURITY DEFINER</literal> Functions Safely</title>
<title>Compatibility</title>
<para>
- A <command>CREATE FUNCTION</command> command is defined in the SQL standard.
- The <productname>PostgreSQL</productname> version is similar but
- not fully compatible. The attributes are not portable, neither are the
- different available languages.
+ A <command>CREATE FUNCTION</command> command is defined in the SQL
+ standard. The <productname>PostgreSQL</productname> implementation can be
+ used in a compatible way but has many extensions. Conversely, the SQL
+ standard specifies a number of optional features that are not implemented
+ in <productname>PostgreSQL</productname>.
</para>
<para>
- For compatibility with some other database systems,
- <replaceable class="parameter">argmode</replaceable> can be written
- either before or after <replaceable class="parameter">argname</replaceable>.
- But only the first way is standard-compliant.
+ The following are important compatibility issues:
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ <literal>OR REPLACE</literal> is a PostgreSQL extension.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ For compatibility with some other database systems, <replaceable
+ class="parameter">argmode</replaceable> can be written either before or
+ after <replaceable class="parameter">argname</replaceable>. But only
+ the first way is standard-compliant.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ For parameter defaults, the SQL standard specifies only the syntax with
+ the <literal>DEFAULT</literal> key word. The syntax with
+ <literal>=</literal> is used in T-SQL and Firebird.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ The <literal>SETOF</literal> modifier is a PostgreSQL extension.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ Only <literal>SQL</literal> is standardized as a language.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ All other attributes except <literal>CALLED ON NULL INPUT</literal> and
+ <literal>RETURNS NULL ON NULL INPUT</literal> are not standardized.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ For the body of <literal>LANGUAGE SQL</literal> functions, the SQL
+ standard only specifies the <replaceable>sql_body</replaceable> form.
+ </para>
+ </listitem>
+ </itemizedlist>
</para>
<para>
- For parameter defaults, the SQL standard specifies only the syntax with
- the <literal>DEFAULT</literal> key word. The syntax
- with <literal>=</literal> is used in T-SQL and Firebird.
+ Simple <literal>LANGUAGE SQL</literal> functions can be written in a way
+ that is both standard-conforming and portable to other implementations.
+ More complex functions using advanced features, optimization attributes, or
+ other languages will necessarily be specific to PostgreSQL in a significant
+ way.
</para>
</refsect1>
diff --git a/doc/src/sgml/ref/create_procedure.sgml b/doc/src/sgml/ref/create_procedure.sgml
index e258eca5ce..ecdeac1629 100644
--- a/doc/src/sgml/ref/create_procedure.sgml
+++ b/doc/src/sgml/ref/create_procedure.sgml
@@ -29,6 +29,7 @@
| SET <replaceable class="parameter">configuration_parameter</replaceable> { TO <replaceable class="parameter">value</replaceable> | = <replaceable class="parameter">value</replaceable> | FROM CURRENT }
| AS '<replaceable class="parameter">definition</replaceable>'
| AS '<replaceable class="parameter">obj_file</replaceable>', '<replaceable class="parameter">link_symbol</replaceable>'
+ | <replaceable class="parameter">sql_body</replaceable>
} ...
</synopsis>
</refsynopsisdiv>
@@ -162,8 +163,9 @@ <title>Parameters</title>
The name of the language that the procedure is implemented in.
It can be <literal>sql</literal>, <literal>c</literal>,
<literal>internal</literal>, or the name of a user-defined
- procedural language, e.g., <literal>plpgsql</literal>. Enclosing the
- name in single quotes is deprecated and requires matching case.
+ procedural language, e.g., <literal>plpgsql</literal>. The default is
+ <literal>sql</literal>. Enclosing the name in single quotes is
+ deprecated and requires matching case.
</para>
</listitem>
</varlistentry>
@@ -299,6 +301,41 @@ <title>Parameters</title>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="parameter">sql_body</replaceable></term>
+
+ <listitem>
+ <para>
+ The body of a <literal>LANGUAGE SQL</literal> procedure. This should
+ be a block
+<programlisting>
+BEGIN ATOMIC
+ <replaceable>statement</replaceable>;
+ <replaceable>statement</replaceable>;
+ ...
+ <replaceable>statement</replaceable>;
+END
+</programlisting>
+ </para>
+
+ <para>
+ This is similar to writing the text of the procedure body as a string
+ constant (see <replaceable>definition</replaceable> above), but there
+ are some differences: This form only works for <literal>LANGUAGE
+ SQL</literal>, the string constant form works for all languages. This
+ form is parsed at procedure definition time, the string constant form is
+ parsed at execution time; therefore this form cannot support
+ polymorphic argument types and other constructs that are not resolvable
+ at procedure definition time. This form tracks dependencies between the
+ procedure and objects used in the procedure body, so <literal>DROP
+ ... CASCADE</literal> will work correctly, whereas the form using
+ string literals may leave dangling procedures. Finally, this form is
+ more compatible with the SQL standard and other SQL implementations.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect1>
@@ -318,6 +355,7 @@ <title>Notes</title>
<refsect1 id="sql-createprocedure-examples">
<title>Examples</title>
+ <para>
<programlisting>
CREATE PROCEDURE insert_data(a integer, b integer)
LANGUAGE SQL
@@ -325,9 +363,21 @@ <title>Examples</title>
INSERT INTO tbl VALUES (a);
INSERT INTO tbl VALUES (b);
$$;
-
+</programlisting>
+ or
+<programlisting>
+CREATE PROCEDURE insert_data(a integer, b integer)
+LANGUAGE SQL
+BEGIN ATOMIC
+ INSERT INTO tbl VALUES (a);
+ INSERT INTO tbl VALUES (b);
+END;
+</programlisting>
+ and call like this:
+<programlisting>
CALL insert_data(1, 2);
</programlisting>
+ </para>
</refsect1>
<refsect1 id="sql-createprocedure-compat">
@@ -335,9 +385,9 @@ <title>Compatibility</title>
<para>
A <command>CREATE PROCEDURE</command> command is defined in the SQL
- standard. The <productname>PostgreSQL</productname> version is similar but
- not fully compatible. For details see
- also <xref linkend="sql-createfunction"/>.
+ standard. The <productname>PostgreSQL</productname> implementation can be
+ used in a compatible way but has many extensions. For details see also
+ <xref linkend="sql-createfunction"/>.
</para>
</refsect1>
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index 0cf1da6ebb..9d6032bf88 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -622,6 +622,7 @@ AggregateCreate(const char *aggName,
InvalidOid, /* no validator */
"aggregate_dummy", /* placeholder proc */
NULL, /* probin */
+ NULL, /* prosqlbody */
PROKIND_AGGREGATE,
false, /* security invoker (currently not
* definable for agg) */
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 1dd9ecc063..808d48949a 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -32,6 +32,7 @@
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
+#include "parser/analyze.h"
#include "parser/parse_coerce.h"
#include "parser/parse_type.h"
#include "tcop/pquery.h"
@@ -76,6 +77,7 @@ ProcedureCreate(const char *procedureName,
Oid languageValidator,
const char *prosrc,
const char *probin,
+ Node *prosqlbody,
char prokind,
bool security_definer,
bool isLeakProof,
@@ -119,8 +121,6 @@ ProcedureCreate(const char *procedureName,
/*
* sanity checks
*/
- Assert(PointerIsValid(prosrc));
-
parameterCount = parameterTypes->dim1;
if (parameterCount < 0 || parameterCount > FUNC_MAX_ARGS)
ereport(ERROR,
@@ -334,11 +334,18 @@ ProcedureCreate(const char *procedureName,
values[Anum_pg_proc_protrftypes - 1] = trftypes;
else
nulls[Anum_pg_proc_protrftypes - 1] = true;
- values[Anum_pg_proc_prosrc - 1] = CStringGetTextDatum(prosrc);
+ if (prosrc)
+ values[Anum_pg_proc_prosrc - 1] = CStringGetTextDatum(prosrc);
+ else
+ nulls[Anum_pg_proc_prosrc - 1] = true;
if (probin)
values[Anum_pg_proc_probin - 1] = CStringGetTextDatum(probin);
else
nulls[Anum_pg_proc_probin - 1] = true;
+ if (prosqlbody)
+ values[Anum_pg_proc_prosqlbody - 1] = CStringGetTextDatum(nodeToString(prosqlbody));
+ else
+ nulls[Anum_pg_proc_prosqlbody - 1] = true;
if (proconfig != PointerGetDatum(NULL))
values[Anum_pg_proc_proconfig - 1] = proconfig;
else
@@ -638,6 +645,10 @@ ProcedureCreate(const char *procedureName,
record_object_address_dependencies(&myself, addrs, DEPENDENCY_NORMAL);
free_object_addresses(addrs);
+ /* dependency on SQL routine body */
+ if (languageObjectId == SQLlanguageId && prosqlbody)
+ recordDependencyOnExpr(&myself, prosqlbody, NIL, DEPENDENCY_NORMAL);
+
/* dependency on parameter default expressions */
if (parameterDefaults)
recordDependencyOnExpr(&myself, (Node *) parameterDefaults,
@@ -861,61 +872,81 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
/* Postpone body checks if !check_function_bodies */
if (check_function_bodies)
{
- tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosrc, &isnull);
- if (isnull)
- elog(ERROR, "null prosrc");
-
- prosrc = TextDatumGetCString(tmp);
-
/*
* Setup error traceback support for ereport().
*/
callback_arg.proname = NameStr(proc->proname);
- callback_arg.prosrc = prosrc;
+ callback_arg.prosrc = NULL;
sqlerrcontext.callback = sql_function_parse_error_callback;
sqlerrcontext.arg = (void *) &callback_arg;
sqlerrcontext.previous = error_context_stack;
error_context_stack = &sqlerrcontext;
- /*
- * We can't do full prechecking of the function definition if there
- * are any polymorphic input types, because actual datatypes of
- * expression results will be unresolvable. The check will be done at
- * runtime instead.
- *
- * We can run the text through the raw parser though; this will at
- * least catch silly syntactic errors.
- */
- raw_parsetree_list = pg_parse_query(prosrc);
+ tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosrc, &isnull);
+ if (isnull)
+ {
+ Node *n;
- if (!haspolyarg)
+ tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosqlbody, &isnull);
+ if (isnull)
+ elog(ERROR, "null prosrc and prosqlbody");
+
+ n = stringToNode(TextDatumGetCString(tmp));
+ if (IsA(n, List))
+ querytree_list = castNode(List, n);
+ else
+ querytree_list = list_make1(list_make1(n));
+ }
+ else
{
+ prosrc = TextDatumGetCString(tmp);
+
+ callback_arg.prosrc = prosrc;
+
/*
- * OK to do full precheck: analyze and rewrite the queries, then
- * verify the result type.
+ * We can't do full prechecking of the function definition if there
+ * are any polymorphic input types, because actual datatypes of
+ * expression results will be unresolvable. The check will be done at
+ * runtime instead.
+ *
+ * We can run the text through the raw parser though; this will at
+ * least catch silly syntactic errors.
*/
- SQLFunctionParseInfoPtr pinfo;
- Oid rettype;
- TupleDesc rettupdesc;
+ raw_parsetree_list = pg_parse_query(prosrc);
- /* But first, set up parameter information */
- pinfo = prepare_sql_fn_parse_info(tuple, NULL, InvalidOid);
-
- querytree_list = NIL;
- foreach(lc, raw_parsetree_list)
+ if (!haspolyarg)
{
- RawStmt *parsetree = lfirst_node(RawStmt, lc);
- List *querytree_sublist;
-
- querytree_sublist = pg_analyze_and_rewrite_params(parsetree,
- prosrc,
- (ParserSetupHook) sql_fn_parser_setup,
- pinfo,
- NULL);
- querytree_list = lappend(querytree_list,
- querytree_sublist);
+ /*
+ * OK to do full precheck: analyze and rewrite the queries, then
+ * verify the result type.
+ */
+ SQLFunctionParseInfoPtr pinfo;
+
+ /* But first, set up parameter information */
+ pinfo = prepare_sql_fn_parse_info(tuple, NULL, InvalidOid);
+
+ querytree_list = NIL;
+ foreach(lc, raw_parsetree_list)
+ {
+ RawStmt *parsetree = lfirst_node(RawStmt, lc);
+ List *querytree_sublist;
+
+ querytree_sublist = pg_analyze_and_rewrite_params(parsetree,
+ prosrc,
+ (ParserSetupHook) sql_fn_parser_setup,
+ pinfo,
+ NULL);
+ querytree_list = lappend(querytree_list,
+ querytree_sublist);
+ }
}
+ }
+
+ if (!haspolyarg)
+ {
+ Oid rettype;
+ TupleDesc rettupdesc;
check_sql_fn_statements(querytree_list);
@@ -968,6 +999,9 @@ function_parse_error_transpose(const char *prosrc)
int newerrposition;
const char *queryText;
+ if (!prosrc)
+ return false;
+
/*
* Nothing to do unless we are dealing with a syntax error that has a
* cursor position.
diff --git a/src/backend/commands/aggregatecmds.c b/src/backend/commands/aggregatecmds.c
index 6bf54e64f8..26b3fa27de 100644
--- a/src/backend/commands/aggregatecmds.c
+++ b/src/backend/commands/aggregatecmds.c
@@ -313,9 +313,11 @@ DefineAggregate(ParseState *pstate,
InvalidOid,
OBJECT_AGGREGATE,
¶meterTypes,
+ NULL,
&allParameterTypes,
¶meterModes,
¶meterNames,
+ NULL,
¶meterDefaults,
&variadicArgType,
&requiredResultType);
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index c3ce480c8f..c48d3744a4 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -53,9 +53,11 @@
#include "commands/proclang.h"
#include "executor/execdesc.h"
#include "executor/executor.h"
+#include "executor/functions.h"
#include "funcapi.h"
#include "miscadmin.h"
#include "optimizer/optimizer.h"
+#include "parser/analyze.h"
#include "parser/parse_coerce.h"
#include "parser/parse_collate.h"
#include "parser/parse_expr.h"
@@ -186,9 +188,11 @@ interpret_function_parameter_list(ParseState *pstate,
Oid languageOid,
ObjectType objtype,
oidvector **parameterTypes,
+ List **parameterTypes_list,
ArrayType **allParameterTypes,
ArrayType **parameterModes,
ArrayType **parameterNames,
+ List **inParameterNames_list,
List **parameterDefaults,
Oid *variadicArgType,
Oid *requiredResultType)
@@ -283,7 +287,11 @@ interpret_function_parameter_list(ParseState *pstate,
/* handle input parameters */
if (fp->mode != FUNC_PARAM_OUT && fp->mode != FUNC_PARAM_TABLE)
+ {
isinput = true;
+ if (parameterTypes_list)
+ *parameterTypes_list = lappend_oid(*parameterTypes_list, toid);
+ }
/* handle signature parameters */
if (fp->mode == FUNC_PARAM_IN || fp->mode == FUNC_PARAM_INOUT ||
@@ -372,6 +380,9 @@ interpret_function_parameter_list(ParseState *pstate,
have_names = true;
}
+ if (inParameterNames_list)
+ *inParameterNames_list = lappend(*inParameterNames_list, makeString(fp->name ? fp->name : pstrdup("")));
+
if (fp->defexpr)
{
Node *def;
@@ -786,28 +797,10 @@ compute_function_attributes(ParseState *pstate,
defel->defname);
}
- /* process required items */
if (as_item)
*as = (List *) as_item->arg;
- else
- {
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("no function body specified")));
- *as = NIL; /* keep compiler quiet */
- }
-
if (language_item)
*language = strVal(language_item->arg);
- else
- {
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("no language specified")));
- *language = NULL; /* keep compiler quiet */
- }
-
- /* process optional items */
if (transform_item)
*transform = transform_item->arg;
if (windowfunc_item)
@@ -856,10 +849,21 @@ compute_function_attributes(ParseState *pstate,
*/
static void
interpret_AS_clause(Oid languageOid, const char *languageName,
- char *funcname, List *as,
- char **prosrc_str_p, char **probin_str_p)
+ char *funcname, List *as, Node *sql_body_in,
+ List *parameterTypes, List *inParameterNames,
+ char **prosrc_str_p, char **probin_str_p, Node **sql_body_out)
{
- Assert(as != NIL);
+ if (sql_body_in && as)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("duplicate function body specified")));
+
+ if (sql_body_in && languageOid != SQLlanguageId)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("inline SQL function body only valid for language SQL")));
+
+ *sql_body_out = NULL;
if (languageOid == ClanguageId)
{
@@ -881,6 +885,66 @@ interpret_AS_clause(Oid languageOid, const char *languageName,
*prosrc_str_p = funcname;
}
}
+ else if (sql_body_in)
+ {
+ SQLFunctionParseInfoPtr pinfo;
+
+ pinfo = (SQLFunctionParseInfoPtr) palloc0(sizeof(SQLFunctionParseInfo));
+
+ pinfo->fname = funcname;
+ pinfo->nargs = list_length(parameterTypes);
+ pinfo->argtypes = (Oid *) palloc(pinfo->nargs * sizeof(Oid));
+ pinfo->argnames = (char **) palloc(pinfo->nargs * sizeof(char *));
+ for (int i = 0; i < list_length(parameterTypes); i++)
+ {
+ char *s = strVal(list_nth(inParameterNames, i));
+
+ pinfo->argtypes[i] = list_nth_oid(parameterTypes, i);
+ if (IsPolymorphicType(pinfo->argtypes[i]))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("SQL function with unquoted function body cannot have polymorphic arguments")));
+
+ if (s[0] != '\0')
+ pinfo->argnames[i] = s;
+ else
+ pinfo->argnames[i] = NULL;
+ }
+
+ if (IsA(sql_body_in, List))
+ {
+ List *stmts = linitial_node(List, castNode(List, sql_body_in));
+ ListCell *lc;
+ List *transformed_stmts = NIL;
+
+ foreach(lc, stmts)
+ {
+ Node *stmt = lfirst(lc);
+ Query *q;
+ ParseState *pstate = make_parsestate(NULL);
+
+ sql_fn_parser_setup(pstate, pinfo);
+ q = transformStmt(pstate, stmt);
+ transformed_stmts = lappend(transformed_stmts, q);
+ free_parsestate(pstate);
+ }
+
+ *sql_body_out = (Node *) list_make1(transformed_stmts);
+ }
+ else
+ {
+ Query *q;
+ ParseState *pstate = make_parsestate(NULL);
+
+ sql_fn_parser_setup(pstate, pinfo);
+ q = transformStmt(pstate, sql_body_in);
+
+ *sql_body_out = (Node *) q;
+ }
+
+ *probin_str_p = NULL;
+ *prosrc_str_p = NULL;
+ }
else
{
/* Everything else wants the given string in prosrc. */
@@ -919,6 +983,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
{
char *probin_str;
char *prosrc_str;
+ Node *prosqlbody;
Oid prorettype;
bool returnsSet;
char *language;
@@ -929,9 +994,11 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
Oid namespaceId;
AclResult aclresult;
oidvector *parameterTypes;
+ List *parameterTypes_list = NIL;
ArrayType *allParameterTypes;
ArrayType *parameterModes;
ArrayType *parameterNames;
+ List *inParameterNames_list = NIL;
List *parameterDefaults;
Oid variadicArgType;
List *trftypes_list = NIL;
@@ -962,6 +1029,8 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
get_namespace_name(namespaceId));
/* Set default attributes */
+ as_clause = NULL;
+ language = "sql";
isWindowFunc = false;
isStrict = false;
security = false;
@@ -1053,9 +1122,11 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
languageOid,
stmt->is_procedure ? OBJECT_PROCEDURE : OBJECT_FUNCTION,
¶meterTypes,
+ ¶meterTypes_list,
&allParameterTypes,
¶meterModes,
¶meterNames,
+ &inParameterNames_list,
¶meterDefaults,
&variadicArgType,
&requiredResultType);
@@ -1112,8 +1183,9 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
trftypes = NULL;
}
- interpret_AS_clause(languageOid, language, funcname, as_clause,
- &prosrc_str, &probin_str);
+ interpret_AS_clause(languageOid, language, funcname, as_clause, stmt->sql_body,
+ parameterTypes_list, inParameterNames_list,
+ &prosrc_str, &probin_str, &prosqlbody);
/*
* Set default values for COST and ROWS depending on other parameters;
@@ -1155,6 +1227,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
languageValidator,
prosrc_str, /* converted to text later */
probin_str, /* converted to text later */
+ prosqlbody,
stmt->is_procedure ? PROKIND_PROCEDURE : (isWindowFunc ? PROKIND_WINDOW : PROKIND_FUNCTION),
security,
isLeakProof,
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 483bb65ddc..65aa91d80f 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1588,6 +1588,7 @@ makeRangeConstructors(const char *name, Oid namespace,
F_FMGR_INTERNAL_VALIDATOR, /* language validator */
prosrc[i], /* prosrc */
NULL, /* probin */
+ NULL, /* prosqlbody */
PROKIND_FUNCTION,
false, /* security_definer */
false, /* leakproof */
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 459a33375b..3ca7e89d44 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -26,6 +26,7 @@
#include "parser/parse_coerce.h"
#include "parser/parse_collate.h"
#include "parser/parse_func.h"
+#include "rewrite/rewriteHandler.h"
#include "storage/proc.h"
#include "tcop/utility.h"
#include "utils/builtins.h"
@@ -128,21 +129,6 @@ typedef struct
typedef SQLFunctionCache *SQLFunctionCachePtr;
-/*
- * Data structure needed by the parser callback hooks to resolve parameter
- * references during parsing of a SQL function's body. This is separate from
- * SQLFunctionCache since we sometimes do parsing separately from execution.
- */
-typedef struct SQLFunctionParseInfo
-{
- char *fname; /* function's name */
- int nargs; /* number of input arguments */
- Oid *argtypes; /* resolved types of input arguments */
- char **argnames; /* names of input arguments; NULL if none */
- /* Note that argnames[i] can be NULL, if some args are unnamed */
- Oid collation; /* function's input collation, if known */
-} SQLFunctionParseInfo;
-
/* non-export function prototypes */
static Node *sql_fn_param_ref(ParseState *pstate, ParamRef *pref);
@@ -607,7 +593,6 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
HeapTuple procedureTuple;
Form_pg_proc procedureStruct;
SQLFunctionCachePtr fcache;
- List *raw_parsetree_list;
List *queryTree_list;
List *resulttlist;
ListCell *lc;
@@ -682,9 +667,6 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
procedureTuple,
Anum_pg_proc_prosrc,
&isNull);
- if (isNull)
- elog(ERROR, "null prosrc for function %u", foid);
- fcache->src = TextDatumGetCString(tmp);
/*
* Parse and rewrite the queries in the function text. Use sublists to
@@ -695,20 +677,55 @@ init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
* but we'll not worry about it until the module is rewritten to use
* plancache.c.
*/
- raw_parsetree_list = pg_parse_query(fcache->src);
-
queryTree_list = NIL;
- foreach(lc, raw_parsetree_list)
+ if (isNull)
{
- RawStmt *parsetree = lfirst_node(RawStmt, lc);
- List *queryTree_sublist;
-
- queryTree_sublist = pg_analyze_and_rewrite_params(parsetree,
- fcache->src,
- (ParserSetupHook) sql_fn_parser_setup,
- fcache->pinfo,
- NULL);
- queryTree_list = lappend(queryTree_list, queryTree_sublist);
+ Node *n;
+ List *stored_query_list;
+
+ tmp = SysCacheGetAttr(PROCOID,
+ procedureTuple,
+ Anum_pg_proc_prosqlbody,
+ &isNull);
+ if (isNull)
+ elog(ERROR, "null prosrc and prosqlbody for function %u", foid);
+
+ n = stringToNode(TextDatumGetCString(tmp));
+ if (IsA(n, List))
+ stored_query_list = linitial_node(List, castNode(List, n));
+ else
+ stored_query_list = list_make1(n);
+
+ foreach(lc, stored_query_list)
+ {
+ Query *parsetree = lfirst_node(Query, lc);
+ List *queryTree_sublist;
+
+ AcquireRewriteLocks(parsetree, true, false);
+ queryTree_sublist = pg_rewrite_query(parsetree);
+ queryTree_list = lappend(queryTree_list, queryTree_sublist);
+ }
+ }
+ else
+ {
+ List *raw_parsetree_list;
+
+ fcache->src = TextDatumGetCString(tmp);
+
+ raw_parsetree_list = pg_parse_query(fcache->src);
+
+ foreach(lc, raw_parsetree_list)
+ {
+ RawStmt *parsetree = lfirst_node(RawStmt, lc);
+ List *queryTree_sublist;
+
+ queryTree_sublist = pg_analyze_and_rewrite_params(parsetree,
+ fcache->src,
+ (ParserSetupHook) sql_fn_parser_setup,
+ fcache->pinfo,
+ NULL);
+ queryTree_list = lappend(queryTree_list, queryTree_sublist);
+ }
}
/*
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 2b4d7654cc..b973b3b967 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3067,6 +3067,7 @@ _copyQuery(const Query *from)
COPY_SCALAR_FIELD(hasModifyingCTE);
COPY_SCALAR_FIELD(hasForUpdate);
COPY_SCALAR_FIELD(hasRowSecurity);
+ COPY_SCALAR_FIELD(isReturn);
COPY_NODE_FIELD(cteList);
COPY_NODE_FIELD(rtable);
COPY_NODE_FIELD(jointree);
@@ -3195,6 +3196,16 @@ _copySetOperationStmt(const SetOperationStmt *from)
return newnode;
}
+static ReturnStmt *
+_copyReturnStmt(const ReturnStmt *from)
+{
+ ReturnStmt *newnode = makeNode(ReturnStmt);
+
+ COPY_NODE_FIELD(returnval);
+
+ return newnode;
+}
+
static AlterTableStmt *
_copyAlterTableStmt(const AlterTableStmt *from)
{
@@ -3571,6 +3582,7 @@ _copyCreateFunctionStmt(const CreateFunctionStmt *from)
COPY_NODE_FIELD(parameters);
COPY_NODE_FIELD(returnType);
COPY_NODE_FIELD(options);
+ COPY_NODE_FIELD(sql_body);
return newnode;
}
@@ -5223,6 +5235,9 @@ copyObjectImpl(const void *from)
case T_SetOperationStmt:
retval = _copySetOperationStmt(from);
break;
+ case T_ReturnStmt:
+ retval = _copyReturnStmt(from);
+ break;
case T_AlterTableStmt:
retval = _copyAlterTableStmt(from);
break;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index e2d1b987bf..0051b1a88b 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -966,6 +966,7 @@ _equalQuery(const Query *a, const Query *b)
COMPARE_SCALAR_FIELD(hasModifyingCTE);
COMPARE_SCALAR_FIELD(hasForUpdate);
COMPARE_SCALAR_FIELD(hasRowSecurity);
+ COMPARE_SCALAR_FIELD(isReturn);
COMPARE_NODE_FIELD(cteList);
COMPARE_NODE_FIELD(rtable);
COMPARE_NODE_FIELD(jointree);
@@ -1082,6 +1083,14 @@ _equalSetOperationStmt(const SetOperationStmt *a, const SetOperationStmt *b)
return true;
}
+static bool
+_equalReturnStmt(const ReturnStmt *a, const ReturnStmt *b)
+{
+ COMPARE_NODE_FIELD(returnval);
+
+ return true;
+}
+
static bool
_equalAlterTableStmt(const AlterTableStmt *a, const AlterTableStmt *b)
{
@@ -1394,6 +1403,7 @@ _equalCreateFunctionStmt(const CreateFunctionStmt *a, const CreateFunctionStmt *
COMPARE_NODE_FIELD(parameters);
COMPARE_NODE_FIELD(returnType);
COMPARE_NODE_FIELD(options);
+ COMPARE_NODE_FIELD(sql_body);
return true;
}
@@ -3277,6 +3287,9 @@ equal(const void *a, const void *b)
case T_SetOperationStmt:
retval = _equalSetOperationStmt(a, b);
break;
+ case T_ReturnStmt:
+ retval = _equalReturnStmt(a, b);
+ break;
case T_AlterTableStmt:
retval = _equalAlterTableStmt(a, b);
break;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 08a049232e..a03b97e7d2 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2756,6 +2756,14 @@ _outSelectStmt(StringInfo str, const SelectStmt *node)
WRITE_NODE_FIELD(rarg);
}
+static void
+_outReturnStmt(StringInfo str, const ReturnStmt *node)
+{
+ WRITE_NODE_TYPE("RETURN");
+
+ WRITE_NODE_FIELD(returnval);
+}
+
static void
_outFuncCall(StringInfo str, const FuncCall *node)
{
@@ -2944,6 +2952,7 @@ _outQuery(StringInfo str, const Query *node)
WRITE_BOOL_FIELD(hasModifyingCTE);
WRITE_BOOL_FIELD(hasForUpdate);
WRITE_BOOL_FIELD(hasRowSecurity);
+ WRITE_BOOL_FIELD(isReturn);
WRITE_NODE_FIELD(cteList);
WRITE_NODE_FIELD(rtable);
WRITE_NODE_FIELD(jointree);
@@ -4194,6 +4203,9 @@ outNode(StringInfo str, const void *obj)
case T_SelectStmt:
_outSelectStmt(str, obj);
break;
+ case T_ReturnStmt:
+ _outReturnStmt(str, obj);
+ break;
case T_ColumnDef:
_outColumnDef(str, obj);
break;
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index ab7b535caa..d01746d0ee 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -263,6 +263,7 @@ _readQuery(void)
READ_BOOL_FIELD(hasModifyingCTE);
READ_BOOL_FIELD(hasForUpdate);
READ_BOOL_FIELD(hasRowSecurity);
+ READ_BOOL_FIELD(isReturn);
READ_NODE_FIELD(cteList);
READ_NODE_FIELD(rtable);
READ_NODE_FIELD(jointree);
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index e7d814651b..f9d27562c0 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4416,27 +4416,47 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
ALLOCSET_DEFAULT_SIZES);
oldcxt = MemoryContextSwitchTo(mycxt);
- /* Fetch the function body */
- tmp = SysCacheGetAttr(PROCOID,
- func_tuple,
- Anum_pg_proc_prosrc,
- &isNull);
- if (isNull)
- elog(ERROR, "null prosrc for function %u", funcid);
- src = TextDatumGetCString(tmp);
-
/*
* Setup error traceback support for ereport(). This is so that we can
* finger the function that bad information came from.
*/
callback_arg.proname = NameStr(funcform->proname);
- callback_arg.prosrc = src;
+ callback_arg.prosrc = NULL;
sqlerrcontext.callback = sql_inline_error_callback;
sqlerrcontext.arg = (void *) &callback_arg;
sqlerrcontext.previous = error_context_stack;
error_context_stack = &sqlerrcontext;
+ /* Fetch the function body */
+ tmp = SysCacheGetAttr(PROCOID,
+ func_tuple,
+ Anum_pg_proc_prosrc,
+ &isNull);
+ if (isNull)
+ {
+ Node *n;
+ List *querytree_list;
+
+ tmp = SysCacheGetAttr(PROCOID, func_tuple, Anum_pg_proc_prosqlbody, &isNull);
+ if (isNull)
+ elog(ERROR, "null prosrc and prosqlbody for function %u", funcid);
+
+ n = stringToNode(TextDatumGetCString(tmp));
+ if (IsA(n, List))
+ querytree_list = linitial_node(List, castNode(List, n));
+ else
+ querytree_list = list_make1(n);
+ if (list_length(querytree_list) != 1)
+ goto fail;
+ querytree = linitial(querytree_list);
+ }
+ else
+ {
+ src = TextDatumGetCString(tmp);
+
+ callback_arg.prosrc = src;
+
/*
* Set up to handle parameters while parsing the function body. We need a
* dummy FuncExpr node containing the already-simplified arguments to pass
@@ -4480,6 +4500,7 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
querytree = transformTopLevelStmt(pstate, linitial(raw_parsetree_list));
free_parsestate(pstate);
+ }
/*
* The single command must be a simple "SELECT expression".
@@ -4736,12 +4757,15 @@ sql_inline_error_callback(void *arg)
int syntaxerrposition;
/* If it's a syntax error, convert to internal syntax error report */
- syntaxerrposition = geterrposition();
- if (syntaxerrposition > 0)
+ if (callback_arg->prosrc)
{
- errposition(0);
- internalerrposition(syntaxerrposition);
- internalerrquery(callback_arg->prosrc);
+ syntaxerrposition = geterrposition();
+ if (syntaxerrposition > 0)
+ {
+ errposition(0);
+ internalerrposition(syntaxerrposition);
+ internalerrquery(callback_arg->prosrc);
+ }
}
errcontext("SQL function \"%s\" during inlining", callback_arg->proname);
@@ -4853,7 +4877,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
Oid func_oid;
HeapTuple func_tuple;
Form_pg_proc funcform;
- char *src;
Datum tmp;
bool isNull;
MemoryContext oldcxt;
@@ -4962,27 +4985,53 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
ALLOCSET_DEFAULT_SIZES);
oldcxt = MemoryContextSwitchTo(mycxt);
- /* Fetch the function body */
- tmp = SysCacheGetAttr(PROCOID,
- func_tuple,
- Anum_pg_proc_prosrc,
- &isNull);
- if (isNull)
- elog(ERROR, "null prosrc for function %u", func_oid);
- src = TextDatumGetCString(tmp);
-
/*
* Setup error traceback support for ereport(). This is so that we can
* finger the function that bad information came from.
*/
callback_arg.proname = NameStr(funcform->proname);
- callback_arg.prosrc = src;
+ callback_arg.prosrc = NULL;
sqlerrcontext.callback = sql_inline_error_callback;
sqlerrcontext.arg = (void *) &callback_arg;
sqlerrcontext.previous = error_context_stack;
error_context_stack = &sqlerrcontext;
+ /* Fetch the function body */
+ tmp = SysCacheGetAttr(PROCOID,
+ func_tuple,
+ Anum_pg_proc_prosrc,
+ &isNull);
+ if (isNull)
+ {
+ Node *n;
+
+ tmp = SysCacheGetAttr(PROCOID, func_tuple, Anum_pg_proc_prosqlbody, &isNull);
+ if (isNull)
+ elog(ERROR, "null prosrc and prosqlbody for function %u", func_oid);
+
+ n = stringToNode(TextDatumGetCString(tmp));
+ if (IsA(n, List))
+ querytree_list = linitial_node(List, castNode(List, n));
+ else
+ querytree_list = list_make1(n);
+ if (list_length(querytree_list) != 1)
+ goto fail;
+ querytree = linitial(querytree_list);
+
+ querytree_list = pg_rewrite_query(querytree);
+ if (list_length(querytree_list) != 1)
+ goto fail;
+ querytree = linitial(querytree_list);
+ }
+ else
+ {
+ char *src;
+
+ src = TextDatumGetCString(tmp);
+
+ callback_arg.prosrc = src;
+
/*
* Set up to handle parameters while parsing the function body. We can
* use the FuncExpr just created as the input for
@@ -4992,18 +5041,6 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
(Node *) fexpr,
fexpr->inputcollid);
- /*
- * Also resolve the actual function result tupdesc, if composite. If the
- * function is just declared to return RECORD, dig the info out of the AS
- * clause.
- */
- functypclass = get_expr_result_type((Node *) fexpr, NULL, &rettupdesc);
- if (functypclass == TYPEFUNC_RECORD)
- rettupdesc = BuildDescFromLists(rtfunc->funccolnames,
- rtfunc->funccoltypes,
- rtfunc->funccoltypmods,
- rtfunc->funccolcollations);
-
/*
* Parse, analyze, and rewrite (unlike inline_function(), we can't skip
* rewriting here). We can fail as soon as we find more than one query,
@@ -5020,6 +5057,19 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
if (list_length(querytree_list) != 1)
goto fail;
querytree = linitial(querytree_list);
+ }
+
+ /*
+ * Also resolve the actual function result tupdesc, if composite. If the
+ * function is just declared to return RECORD, dig the info out of the AS
+ * clause.
+ */
+ functypclass = get_expr_result_type((Node *) fexpr, NULL, &rettupdesc);
+ if (functypclass == TYPEFUNC_RECORD)
+ rettupdesc = BuildDescFromLists(rtfunc->funccolnames,
+ rtfunc->funccoltypes,
+ rtfunc->funccoltypmods,
+ rtfunc->funccolcollations);
/*
* The single command must be a plain SELECT.
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index c159fb2957..ed0622c5d7 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -66,6 +66,7 @@ static Node *transformSetOperationTree(ParseState *pstate, SelectStmt *stmt,
bool isTopLevel, List **targetlist);
static void determineRecursiveColTypes(ParseState *pstate,
Node *larg, List *nrtargetlist);
+static Query *transformReturnStmt(ParseState *pstate, ReturnStmt *stmt);
static Query *transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt);
static List *transformReturningList(ParseState *pstate, List *returningList);
static List *transformUpdateTargetList(ParseState *pstate,
@@ -304,6 +305,10 @@ transformStmt(ParseState *pstate, Node *parseTree)
}
break;
+ case T_ReturnStmt:
+ result = transformReturnStmt(pstate, (ReturnStmt *) parseTree);
+ break;
+
/*
* Special cases
*/
@@ -2221,6 +2226,36 @@ determineRecursiveColTypes(ParseState *pstate, Node *larg, List *nrtargetlist)
}
+/*
+ * transformReturnStmt -
+ * transforms a return statement
+ */
+static Query *
+transformReturnStmt(ParseState *pstate, ReturnStmt *stmt)
+{
+ Query *qry = makeNode(Query);
+
+ qry->commandType = CMD_SELECT;
+ qry->isReturn = true;
+
+ qry->targetList = list_make1(makeTargetEntry((Expr *) transformExpr(pstate, stmt->returnval, EXPR_KIND_SELECT_TARGET),
+ 1, NULL, false));
+
+ if (pstate->p_resolve_unknowns)
+ resolveTargetListUnknowns(pstate, qry->targetList);
+ qry->rtable = pstate->p_rtable;
+ qry->jointree = makeFromExpr(pstate->p_joinlist, NULL);
+ qry->hasSubLinks = pstate->p_hasSubLinks;
+ qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
+ qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
+ qry->hasAggs = pstate->p_hasAggs;
+
+ assign_query_collations(pstate, qry);
+
+ return qry;
+}
+
+
/*
* transformUpdateStmt -
* transforms an update statement
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 480d168346..e0f49a552f 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -253,7 +253,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
struct SelectLimit *selectlimit;
}
-%type <node> stmt schema_stmt
+%type <node> stmt toplevel_stmt schema_stmt routine_body_stmt
AlterEventTrigStmt AlterCollationStmt
AlterDatabaseStmt AlterDatabaseSetStmt AlterDomainStmt AlterEnumStmt
AlterFdwStmt AlterForeignServerStmt AlterGroupStmt
@@ -280,9 +280,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
GrantStmt GrantRoleStmt ImportForeignSchemaStmt IndexStmt InsertStmt
ListenStmt LoadStmt LockStmt NotifyStmt ExplainableStmt PreparableStmt
CreateFunctionStmt AlterFunctionStmt ReindexStmt RemoveAggrStmt
- RemoveFuncStmt RemoveOperStmt RenameStmt RevokeStmt RevokeRoleStmt
+ RemoveFuncStmt RemoveOperStmt RenameStmt ReturnStmt RevokeStmt RevokeRoleStmt
RuleActionStmt RuleActionStmtOrEmpty RuleStmt
- SecLabelStmt SelectStmt TransactionStmt TruncateStmt
+ SecLabelStmt SelectStmt TransactionStmt TransactionStmtLegacy TruncateStmt
UnlistenStmt UpdateStmt VacuumStmt
VariableResetStmt VariableSetStmt VariableShowStmt
ViewStmt CheckPointStmt CreateConversionStmt
@@ -385,14 +385,14 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <node> vacuum_relation
%type <selectlimit> opt_select_limit select_limit limit_clause
-%type <list> stmtblock stmtmulti
+%type <list> stmtblock stmtmulti routine_body_stmt_list
OptTableElementList TableElementList OptInherit definition
OptTypedTableElementList TypedTableElementList
reloptions opt_reloptions
OptWith distinct_clause opt_all_clause opt_definition func_args func_args_list
func_args_with_defaults func_args_with_defaults_list
aggr_args aggr_args_list
- func_as createfunc_opt_list alterfunc_opt_list
+ func_as createfunc_opt_list opt_createfunc_opt_list alterfunc_opt_list
old_aggr_definition old_aggr_list
oper_argtypes RuleActionList RuleActionMulti
opt_column_list columnList opt_name_list
@@ -418,6 +418,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
vacuum_relation_list opt_vacuum_relation_list
drop_option_list
+%type <node> opt_routine_body
%type <list> group_by_list
%type <node> group_by_item empty_grouping_set rollup_clause cube_clause
%type <node> grouping_sets_clause
@@ -628,7 +629,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
/* ordinary key words in alphabetical order */
%token <keyword> ABORT_P ABSOLUTE_P ACCESS ACTION ADD_P ADMIN AFTER
AGGREGATE ALL ALSO ALTER ALWAYS ANALYSE ANALYZE AND ANY ARRAY AS ASC
- ASSERTION ASSIGNMENT ASYMMETRIC AT ATTACH ATTRIBUTE AUTHORIZATION
+ ASSERTION ASSIGNMENT ASYMMETRIC AT ATOMIC ATTACH ATTRIBUTE AUTHORIZATION
BACKWARD BEFORE BEGIN_P BETWEEN BIGINT BINARY BIT
BOOLEAN_P BOTH BY
@@ -690,7 +691,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
RANGE READ REAL REASSIGN RECHECK RECURSIVE REF REFERENCES REFERENCING
REFRESH REINDEX RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
- RESET RESTART RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROLLUP
+ RESET RESTART RESTRICT RETURN RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROLLUP
ROUTINE ROUTINES ROW ROWS RULE
SAVEPOINT SCHEMA SCHEMAS SCROLL SEARCH SECOND_P SECURITY SELECT SEQUENCE SEQUENCES
@@ -812,7 +813,7 @@ stmtblock: stmtmulti
* we'd get -1 for the location in such cases.
* We also take care to discard empty statements entirely.
*/
-stmtmulti: stmtmulti ';' stmt
+stmtmulti: stmtmulti ';' toplevel_stmt
{
if ($1 != NIL)
{
@@ -824,7 +825,7 @@ stmtmulti: stmtmulti ';' stmt
else
$$ = $1;
}
- | stmt
+ | toplevel_stmt
{
if ($1 != NULL)
$$ = list_make1(makeRawStmt($1, 0));
@@ -833,7 +834,16 @@ stmtmulti: stmtmulti ';' stmt
}
;
-stmt :
+/*
+ * toplevel_stmt includes BEGIN and END. stmt does not include them, because
+ * those words have different meanings in function bodys.
+ */
+toplevel_stmt:
+ stmt
+ | TransactionStmtLegacy
+ ;
+
+stmt:
AlterEventTrigStmt
| AlterCollationStmt
| AlterDatabaseStmt
@@ -7339,7 +7349,7 @@ opt_nulls_order: NULLS_LA FIRST_P { $$ = SORTBY_NULLS_FIRST; }
CreateFunctionStmt:
CREATE opt_or_replace FUNCTION func_name func_args_with_defaults
- RETURNS func_return createfunc_opt_list
+ RETURNS func_return opt_createfunc_opt_list opt_routine_body
{
CreateFunctionStmt *n = makeNode(CreateFunctionStmt);
n->is_procedure = false;
@@ -7348,10 +7358,11 @@ CreateFunctionStmt:
n->parameters = $5;
n->returnType = $7;
n->options = $8;
+ n->sql_body = $9;
$$ = (Node *)n;
}
| CREATE opt_or_replace FUNCTION func_name func_args_with_defaults
- RETURNS TABLE '(' table_func_column_list ')' createfunc_opt_list
+ RETURNS TABLE '(' table_func_column_list ')' opt_createfunc_opt_list opt_routine_body
{
CreateFunctionStmt *n = makeNode(CreateFunctionStmt);
n->is_procedure = false;
@@ -7361,10 +7372,11 @@ CreateFunctionStmt:
n->returnType = TableFuncTypeName($9);
n->returnType->location = @7;
n->options = $11;
+ n->sql_body = $12;
$$ = (Node *)n;
}
| CREATE opt_or_replace FUNCTION func_name func_args_with_defaults
- createfunc_opt_list
+ opt_createfunc_opt_list opt_routine_body
{
CreateFunctionStmt *n = makeNode(CreateFunctionStmt);
n->is_procedure = false;
@@ -7373,10 +7385,11 @@ CreateFunctionStmt:
n->parameters = $5;
n->returnType = NULL;
n->options = $6;
+ n->sql_body = $7;
$$ = (Node *)n;
}
| CREATE opt_or_replace PROCEDURE func_name func_args_with_defaults
- createfunc_opt_list
+ opt_createfunc_opt_list opt_routine_body
{
CreateFunctionStmt *n = makeNode(CreateFunctionStmt);
n->is_procedure = true;
@@ -7385,6 +7398,7 @@ CreateFunctionStmt:
n->parameters = $5;
n->returnType = NULL;
n->options = $6;
+ n->sql_body = $7;
$$ = (Node *)n;
}
;
@@ -7695,6 +7709,11 @@ aggregate_with_argtypes_list:
{ $$ = lappend($1, $3); }
;
+opt_createfunc_opt_list:
+ createfunc_opt_list
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
createfunc_opt_list:
/* Must be at least one to prevent conflict */
createfunc_opt_item { $$ = list_make1($1); }
@@ -7806,6 +7825,51 @@ func_as: Sconst { $$ = list_make1(makeString($1)); }
}
;
+ReturnStmt: RETURN a_expr
+ {
+ ReturnStmt *r = makeNode(ReturnStmt);
+ r->returnval = (Node *) $2;
+ $$ = (Node *) r;
+ }
+ ;
+
+opt_routine_body:
+ ReturnStmt
+ {
+ $$ = $1;
+ }
+ | BEGIN_P ATOMIC routine_body_stmt_list END_P
+ {
+ /*
+ * A compound statement is stored as a single-item list
+ * containing the list of statements as its member. That
+ * way, the parse analysis code can tell apart an empty
+ * body from no body at all.
+ */
+ $$ = (Node *) list_make1($3);
+ }
+ | /*EMPTY*/
+ {
+ $$ = NULL;
+ }
+ ;
+
+routine_body_stmt_list:
+ routine_body_stmt_list routine_body_stmt ';'
+ {
+ $$ = lappend($1, $2);
+ }
+ | /*EMPTY*/
+ {
+ $$ = NIL;
+ }
+ ;
+
+routine_body_stmt:
+ stmt
+ | ReturnStmt
+ ;
+
transform_type_list:
FOR TYPE_P Typename { $$ = list_make1($3); }
| transform_type_list ',' FOR TYPE_P Typename { $$ = lappend($1, $5); }
@@ -9756,13 +9820,6 @@ TransactionStmt:
n->chain = $3;
$$ = (Node *)n;
}
- | BEGIN_P opt_transaction transaction_mode_list_or_empty
- {
- TransactionStmt *n = makeNode(TransactionStmt);
- n->kind = TRANS_STMT_BEGIN;
- n->options = $3;
- $$ = (Node *)n;
- }
| START TRANSACTION transaction_mode_list_or_empty
{
TransactionStmt *n = makeNode(TransactionStmt);
@@ -9778,14 +9835,6 @@ TransactionStmt:
n->chain = $3;
$$ = (Node *)n;
}
- | END_P opt_transaction opt_transaction_chain
- {
- TransactionStmt *n = makeNode(TransactionStmt);
- n->kind = TRANS_STMT_COMMIT;
- n->options = NIL;
- n->chain = $3;
- $$ = (Node *)n;
- }
| ROLLBACK opt_transaction opt_transaction_chain
{
TransactionStmt *n = makeNode(TransactionStmt);
@@ -9852,6 +9901,24 @@ TransactionStmt:
}
;
+TransactionStmtLegacy:
+ BEGIN_P opt_transaction transaction_mode_list_or_empty
+ {
+ TransactionStmt *n = makeNode(TransactionStmt);
+ n->kind = TRANS_STMT_BEGIN;
+ n->options = $3;
+ $$ = (Node *)n;
+ }
+ | END_P opt_transaction opt_transaction_chain
+ {
+ TransactionStmt *n = makeNode(TransactionStmt);
+ n->kind = TRANS_STMT_COMMIT;
+ n->options = NIL;
+ n->chain = $3;
+ $$ = (Node *)n;
+ }
+ ;
+
opt_transaction: WORK {}
| TRANSACTION {}
| /*EMPTY*/ {}
@@ -15074,6 +15141,7 @@ unreserved_keyword:
| ASSERTION
| ASSIGNMENT
| AT
+ | ATOMIC
| ATTACH
| ATTRIBUTE
| BACKWARD
@@ -15272,6 +15340,7 @@ unreserved_keyword:
| RESET
| RESTART
| RESTRICT
+ | RETURN
| RETURNS
| REVOKE
| ROLE
@@ -15578,6 +15647,7 @@ bare_label_keyword:
| ASSIGNMENT
| ASYMMETRIC
| AT
+ | ATOMIC
| ATTACH
| ATTRIBUTE
| AUTHORIZATION
@@ -15848,6 +15918,7 @@ bare_label_keyword:
| RESET
| RESTART
| RESTRICT
+ | RETURN
| RETURNS
| REVOKE
| RIGHT
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 411cfadbff..e145f552c0 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -176,7 +176,6 @@ static int interactive_getc(void);
static int SocketBackend(StringInfo inBuf);
static int ReadCommand(StringInfo inBuf);
static void forbidden_in_wal_sender(char firstchar);
-static List *pg_rewrite_query(Query *query);
static bool check_log_statement(List *stmt_list);
static int errdetail_execute(List *raw_parsetree_list);
static int errdetail_params(ParamListInfo params);
@@ -761,7 +760,7 @@ pg_analyze_and_rewrite_params(RawStmt *parsetree,
* Note: query must just have come from the parser, because we do not do
* AcquireRewriteLocks() on it.
*/
-static List *
+List *
pg_rewrite_query(Query *query)
{
List *querytree_list;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 6c656586e8..6a52686cca 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -174,6 +174,9 @@ typedef struct
List *outer_tlist; /* referent for OUTER_VAR Vars */
List *inner_tlist; /* referent for INNER_VAR Vars */
List *index_tlist; /* referent for INDEX_VAR Vars */
+ /* Special namespace representing a function signature: */
+ int numargs;
+ char **argnames;
} deparse_namespace;
/*
@@ -349,6 +352,7 @@ static int print_function_arguments(StringInfo buf, HeapTuple proctup,
bool print_table_args, bool print_defaults);
static void print_function_rettype(StringInfo buf, HeapTuple proctup);
static void print_function_trftypes(StringInfo buf, HeapTuple proctup);
+static void print_function_sqlbody(StringInfo buf, HeapTuple proctup);
static void set_rtable_names(deparse_namespace *dpns, List *parent_namespaces,
Bitmapset *rels_used);
static void set_deparse_for_query(deparse_namespace *dpns, Query *query,
@@ -2800,6 +2804,13 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
}
/* And finally the function definition ... */
+ tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
+ if (proc->prolang == SQLlanguageId && !isnull)
+ {
+ print_function_sqlbody(&buf, proctup);
+ }
+ else
+ {
appendStringInfoString(&buf, "AS ");
tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_probin, &isnull);
@@ -2831,6 +2842,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
appendBinaryStringInfo(&buf, dq.data, dq.len);
appendStringInfoString(&buf, prosrc);
appendBinaryStringInfo(&buf, dq.data, dq.len);
+ }
appendStringInfoChar(&buf, '\n');
@@ -3213,6 +3225,75 @@ pg_get_function_arg_default(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(string_to_text(str));
}
+static void
+print_function_sqlbody(StringInfo buf, HeapTuple proctup)
+{
+ int numargs;
+ Oid *argtypes;
+ char **argnames;
+ char *argmodes;
+ deparse_namespace dpns = {0};
+ Datum tmp;
+ bool isnull;
+ Node *n;
+
+ numargs = get_func_arg_info(proctup,
+ &argtypes, &argnames, &argmodes);
+ dpns.numargs = numargs;
+ dpns.argnames = argnames;
+
+ tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosqlbody, &isnull);
+ Assert(!isnull);
+ n = stringToNode(TextDatumGetCString(tmp));
+
+ if (IsA(n, List))
+ {
+ List *stmts;
+ ListCell *lc;
+
+ stmts = linitial(castNode(List, n));
+
+ appendStringInfoString(buf, "BEGIN ATOMIC\n");
+
+ foreach(lc, stmts)
+ {
+ Query *query = lfirst_node(Query, lc);
+
+ get_query_def(query, buf, list_make1(&dpns), NULL, PRETTYFLAG_INDENT, WRAP_COLUMN_DEFAULT, 1);
+ appendStringInfoChar(buf, ';');
+ appendStringInfoChar(buf, '\n');
+ }
+
+ appendStringInfoString(buf, "END");
+ }
+ else
+ {
+ get_query_def(castNode(Query, n), buf, list_make1(&dpns), NULL, PRETTYFLAG_INDENT, WRAP_COLUMN_DEFAULT, 1);
+ appendStringInfoChar(buf, '\n');
+ }
+}
+
+Datum
+pg_get_function_sqlbody(PG_FUNCTION_ARGS)
+{
+ Oid funcid = PG_GETARG_OID(0);
+ StringInfoData buf;
+ HeapTuple proctup;
+
+ initStringInfo(&buf);
+
+ /* Look up the function */
+ proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+ if (!HeapTupleIsValid(proctup))
+ PG_RETURN_NULL();
+
+ print_function_sqlbody(&buf, proctup);
+
+ ReleaseSysCache(proctup);
+
+ PG_RETURN_TEXT_P(cstring_to_text(buf.data));
+}
+
/*
* deparse_expression - General utility for deparsing expressions
@@ -5418,7 +5499,10 @@ get_basic_select_query(Query *query, deparse_context *context,
/*
* Build up the query string - first we say SELECT
*/
- appendStringInfoString(buf, "SELECT");
+ if (query->isReturn)
+ appendStringInfoString(buf, "RETURN");
+ else
+ appendStringInfoString(buf, "SELECT");
/* Add the DISTINCT clause if given */
if (query->distinctClause != NIL)
@@ -7550,6 +7634,26 @@ get_parameter(Param *param, deparse_context *context)
return;
}
+ /*
+ * If it's an external parameter, see if the outermost namespace provides
+ * function argument names.
+ */
+ if (param->paramkind == PARAM_EXTERN)
+ {
+ dpns = lfirst(list_tail(context->namespaces));
+ if (dpns->argnames)
+ {
+ char *argname = dpns->argnames[param->paramid - 1];
+
+ if (argname)
+ {
+ // TODO: qualify with function name if necessary
+ appendStringInfo(context->buf, "%s", quote_identifier(argname));
+ return;
+ }
+ }
+ }
+
/*
* Not PARAM_EXEC, or couldn't find referent: just print $N.
*/
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ff45e3fb8c..8acbab80c8 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -11803,6 +11803,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
char *proretset;
char *prosrc;
char *probin;
+ char *prosqlbody;
char *funcargs;
char *funciargs;
char *funcresult;
@@ -11849,7 +11850,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
"provolatile,\n"
"proisstrict,\n"
"prosecdef,\n"
- "(SELECT lanname FROM pg_catalog.pg_language WHERE oid = prolang) AS lanname,\n");
+ "lanname,\n");
if (fout->remoteVersion >= 80300)
appendPQExpBufferStr(query,
@@ -11869,9 +11870,9 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
* pg_get_function_result instead of examining proallargtypes etc.
*/
appendPQExpBufferStr(query,
- "pg_catalog.pg_get_function_arguments(oid) AS funcargs,\n"
- "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs,\n"
- "pg_catalog.pg_get_function_result(oid) AS funcresult,\n");
+ "pg_catalog.pg_get_function_arguments(p.oid) AS funcargs,\n"
+ "pg_catalog.pg_get_function_identity_arguments(p.oid) AS funciargs,\n"
+ "pg_catalog.pg_get_function_result(p.oid) AS funcresult,\n");
}
else if (fout->remoteVersion >= 80100)
appendPQExpBufferStr(query,
@@ -11914,21 +11915,39 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
if (fout->remoteVersion >= 120000)
appendPQExpBufferStr(query,
- "prosupport\n");
+ "prosupport,\n");
else
appendPQExpBufferStr(query,
- "'-' AS prosupport\n");
+ "'-' AS prosupport,\n");
+
+ if (fout->remoteVersion >= 140000)
+ appendPQExpBufferStr(query,
+ "CASE WHEN prosrc IS NULL AND lanname = 'sql' THEN pg_get_function_sqlbody(p.oid) END AS prosqlbody\n");
+ else
+ appendPQExpBufferStr(query,
+ "NULL AS prosqlbody\n");
appendPQExpBuffer(query,
- "FROM pg_catalog.pg_proc "
- "WHERE oid = '%u'::pg_catalog.oid",
+ "FROM pg_catalog.pg_proc p, pg_catalog.pg_language l\n"
+ "WHERE p.oid = '%u'::pg_catalog.oid "
+ "AND l.oid = p.prolang",
finfo->dobj.catId.oid);
res = ExecuteSqlQueryForSingleRow(fout, query->data);
proretset = PQgetvalue(res, 0, PQfnumber(res, "proretset"));
- prosrc = PQgetvalue(res, 0, PQfnumber(res, "prosrc"));
- probin = PQgetvalue(res, 0, PQfnumber(res, "probin"));
+ if (PQgetisnull(res, 0, PQfnumber(res, "prosqlbody")))
+ {
+ prosrc = PQgetvalue(res, 0, PQfnumber(res, "prosrc"));
+ probin = PQgetvalue(res, 0, PQfnumber(res, "probin"));
+ prosqlbody = NULL;
+ }
+ else
+ {
+ prosrc = NULL;
+ probin = NULL;
+ prosqlbody = PQgetvalue(res, 0, PQfnumber(res, "prosqlbody"));
+ }
if (fout->remoteVersion >= 80400)
{
funcargs = PQgetvalue(res, 0, PQfnumber(res, "funcargs"));
@@ -11965,7 +11984,11 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
* versions would set it to "-". There are no known cases in which prosrc
* is unused, so the tests below for "-" are probably useless.
*/
- if (probin[0] != '\0' && strcmp(probin, "-") != 0)
+ if (prosqlbody)
+ {
+ appendPQExpBufferStr(asPart, prosqlbody);
+ }
+ else if (probin[0] != '\0' && strcmp(probin, "-") != 0)
{
appendPQExpBufferStr(asPart, "AS ");
appendStringLiteralAH(asPart, probin, fout);
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 07d640021c..319c34ff2b 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -505,11 +505,18 @@ describeFunctions(const char *functypes, const char *pattern, bool verbose, bool
appendPQExpBufferStr(&buf, ",\n ");
printACLColumn(&buf, "p.proacl");
appendPQExpBuffer(&buf,
- ",\n l.lanname as \"%s\""
- ",\n p.prosrc as \"%s\""
+ ",\n l.lanname as \"%s\"",
+ gettext_noop("Language"));
+ if (pset.sversion >= 140000)
+ appendPQExpBuffer(&buf,
+ ",\n COALESCE(p.prosrc, pg_catalog.pg_get_function_sqlbody(p.oid)) as \"%s\"",
+ gettext_noop("Source code"));
+ else
+ appendPQExpBuffer(&buf,
+ ",\n p.prosrc as \"%s\"",
+ gettext_noop("Source code"));
+ appendPQExpBuffer(&buf,
",\n pg_catalog.obj_description(p.oid, 'pg_proc') as \"%s\"",
- gettext_noop("Language"),
- gettext_noop("Source code"),
gettext_noop("Description"));
}
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index 08dffde1ba..ee34463e67 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -645,10 +645,11 @@ other .
";" {
ECHO;
- if (cur_state->paren_depth == 0)
+ if (cur_state->paren_depth == 0 && cur_state->begin_depth == 0)
{
/* Terminate lexing temporarily */
cur_state->start_state = YY_START;
+ cur_state->identifier_count = 0;
return LEXRES_SEMI;
}
}
@@ -661,6 +662,8 @@ other .
"\\"[;:] {
/* Force a semi-colon or colon into the query buffer */
psqlscan_emit(cur_state, yytext + 1, 1);
+ if (yytext[1] == ';')
+ cur_state->identifier_count = 0;
}
"\\" {
@@ -867,6 +870,17 @@ other .
{identifier} {
+ cur_state->identifier_count++;
+ if (pg_strcasecmp(yytext, "begin") == 0)
+ {
+ if (cur_state->identifier_count > 1)
+ cur_state->begin_depth++;
+ }
+ else if (pg_strcasecmp(yytext, "end") == 0)
+ {
+ if (cur_state->begin_depth > 0)
+ cur_state->begin_depth--;
+ }
ECHO;
}
@@ -1054,6 +1068,11 @@ psql_scan(PsqlScanState state,
result = PSCAN_INCOMPLETE;
*prompt = PROMPT_PAREN;
}
+ if (state->begin_depth > 0)
+ {
+ result = PSCAN_INCOMPLETE;
+ *prompt = PROMPT_CONTINUE;
+ }
else if (query_buf->len > 0)
{
result = PSCAN_EOL;
@@ -1170,6 +1189,8 @@ psql_scan_reset(PsqlScanState state)
if (state->dolqstart)
free(state->dolqstart);
state->dolqstart = NULL;
+ state->identifier_count = 0;
+ state->begin_depth = 0;
}
/*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index bbcac69d48..71bf2d8cc6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3681,6 +3681,10 @@
proname => 'pg_get_function_arg_default', provolatile => 's',
prorettype => 'text', proargtypes => 'oid int4',
prosrc => 'pg_get_function_arg_default' },
+{ oid => '9704', descr => 'function SQL body',
+ proname => 'pg_get_function_sqlbody', provolatile => 's',
+ prorettype => 'text', proargtypes => 'oid',
+ prosrc => 'pg_get_function_sqlbody' },
{ oid => '1686', descr => 'list of SQL keywords',
proname => 'pg_get_keywords', procost => '10', prorows => '500',
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 268c810896..720d6c77ad 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -112,11 +112,14 @@ CATALOG(pg_proc,1255,ProcedureRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(81,Proce
Oid protrftypes[1] BKI_DEFAULT(_null_);
/* procedure source text */
- text prosrc BKI_FORCE_NOT_NULL;
+ text prosrc;
/* secondary procedure info (can be NULL) */
text probin BKI_DEFAULT(_null_);
+ /* pre-parsed SQL function body */
+ pg_node_tree prosqlbody BKI_DEFAULT(_null_);
+
/* procedure-local GUC settings */
text proconfig[1] BKI_DEFAULT(_null_);
@@ -187,6 +190,7 @@ extern ObjectAddress ProcedureCreate(const char *procedureName,
Oid languageValidator,
const char *prosrc,
const char *probin,
+ Node *prosqlbody,
char prokind,
bool security_definer,
bool isLeakProof,
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 7a079ef07f..a32cfba421 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -68,9 +68,11 @@ extern void interpret_function_parameter_list(ParseState *pstate,
Oid languageOid,
ObjectType objtype,
oidvector **parameterTypes,
+ List **parameterTypes_list,
ArrayType **allParameterTypes,
ArrayType **parameterModes,
ArrayType **parameterNames,
+ List **inParameterNames_list,
List **parameterDefaults,
Oid *variadicArgType,
Oid *requiredResultType);
diff --git a/src/include/executor/functions.h b/src/include/executor/functions.h
index a0db24bde6..3111ff824a 100644
--- a/src/include/executor/functions.h
+++ b/src/include/executor/functions.h
@@ -20,6 +20,21 @@
/* This struct is known only within executor/functions.c */
typedef struct SQLFunctionParseInfo *SQLFunctionParseInfoPtr;
+/*
+ * Data structure needed by the parser callback hooks to resolve parameter
+ * references during parsing of a SQL function's body. This is separate from
+ * SQLFunctionCache since we sometimes do parsing separately from execution.
+ */
+typedef struct SQLFunctionParseInfo
+{
+ char *fname; /* function's name */
+ int nargs; /* number of input arguments */
+ Oid *argtypes; /* resolved types of input arguments */
+ char **argnames; /* names of input arguments; NULL if none */
+ /* Note that argnames[i] can be NULL, if some args are unnamed */
+ Oid collation; /* function's input collation, if known */
+} SQLFunctionParseInfo;
+
extern Datum fmgr_sql(PG_FUNCTION_ARGS);
extern SQLFunctionParseInfoPtr prepare_sql_fn_parse_info(HeapTuple procedureTuple,
diff --git a/src/include/fe_utils/psqlscan_int.h b/src/include/fe_utils/psqlscan_int.h
index 311f80394a..fb8f58aa29 100644
--- a/src/include/fe_utils/psqlscan_int.h
+++ b/src/include/fe_utils/psqlscan_int.h
@@ -114,6 +114,8 @@ typedef struct PsqlScanStateData
int paren_depth; /* depth of nesting in parentheses */
int xcdepth; /* depth of nesting in slash-star comments */
char *dolqstart; /* current $foo$ quote start string */
+ int identifier_count; /* identifiers since start of statement */
+ int begin_depth; /* depth of begin/end routine body blocks */
/*
* Callback functions provided by the program making use of the lexer,
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 7ddd8c011b..f5716a3eb0 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -315,6 +315,7 @@ typedef enum NodeTag
T_DeleteStmt,
T_UpdateStmt,
T_SelectStmt,
+ T_ReturnStmt,
T_AlterTableStmt,
T_AlterTableCmd,
T_AlterDomainStmt,
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 60c2f45466..f0c0c307f9 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -132,6 +132,8 @@ typedef struct Query
bool hasForUpdate; /* FOR [KEY] UPDATE/SHARE was specified */
bool hasRowSecurity; /* rewriter has applied some RLS policy */
+ bool isReturn; /* is a RETURN statement */
+
List *cteList; /* WITH list (of CommonTableExpr's) */
List *rtable; /* list of range table entries */
@@ -1671,6 +1673,16 @@ typedef struct SetOperationStmt
} SetOperationStmt;
+/*
+ * RETURN statement (inside SQL function body)
+ */
+typedef struct ReturnStmt
+{
+ NodeTag type;
+ Node *returnval;
+} ReturnStmt;
+
+
/*****************************************************************************
* Other Statements (no optimizations required)
*
@@ -2842,6 +2854,7 @@ typedef struct CreateFunctionStmt
List *parameters; /* a list of FunctionParameter */
TypeName *returnType; /* the return type */
List *options; /* a list of DefElem */
+ Node *sql_body;
} CreateFunctionStmt;
typedef enum FunctionParameterMode
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 71dcdf2889..36317a14db 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -48,6 +48,7 @@ PG_KEYWORD("assertion", ASSERTION, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("assignment", ASSIGNMENT, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("asymmetric", ASYMMETRIC, RESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("at", AT, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("atomic", ATOMIC, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("attach", ATTACH, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("attribute", ATTRIBUTE, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("authorization", AUTHORIZATION, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL)
@@ -344,6 +345,7 @@ PG_KEYWORD("replica", REPLICA, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("reset", RESET, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("restart", RESTART, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("restrict", RESTRICT, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("return", RETURN, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("returning", RETURNING, RESERVED_KEYWORD, AS_LABEL)
PG_KEYWORD("returns", RETURNS, UNRESERVED_KEYWORD, BARE_LABEL)
PG_KEYWORD("revoke", REVOKE, UNRESERVED_KEYWORD, BARE_LABEL)
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index bd30607b07..e626c8eafd 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -43,6 +43,7 @@ typedef enum
extern PGDLLIMPORT int log_statement;
extern List *pg_parse_query(const char *query_string);
+extern List *pg_rewrite_query(Query *query);
extern List *pg_analyze_and_rewrite(RawStmt *parsetree,
const char *query_string,
Oid *paramTypes, int numParams,
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 300381eaad..0441561c52 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -87,6 +87,12 @@ ECPG: stmtTransactionStmt block
whenever_action(2);
free($1);
}
+ECPG: toplevel_stmtTransactionStmtLegacy block
+ {
+ fprintf(base_yyout, "{ ECPGtrans(__LINE__, %s, \"%s\");", connection ? connection : "NULL", $1);
+ whenever_action(2);
+ free($1);
+ }
ECPG: stmtViewStmt rule
| ECPGAllocateDescr
{
diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer b/src/interfaces/ecpg/preproc/ecpg.trailer
index 6ccc8ab916..7f4be655f9 100644
--- a/src/interfaces/ecpg/preproc/ecpg.trailer
+++ b/src/interfaces/ecpg/preproc/ecpg.trailer
@@ -4,8 +4,8 @@ statements: /*EMPTY*/
| statements statement
;
-statement: ecpgstart at stmt ';' { connection = NULL; }
- | ecpgstart stmt ';'
+statement: ecpgstart at toplevel_stmt ';' { connection = NULL; }
+ | ecpgstart toplevel_stmt ';'
| ecpgstart ECPGVarDeclaration
{
fprintf(base_yyout, "%s", $2);
diff --git a/src/test/regress/expected/create_function_3.out b/src/test/regress/expected/create_function_3.out
index ce508ae1dc..dbd6358ffd 100644
--- a/src/test/regress/expected/create_function_3.out
+++ b/src/test/regress/expected/create_function_3.out
@@ -255,6 +255,151 @@ SELECT pg_get_functiondef('functest_F_2'::regproc);
(1 row)
+--
+-- SQL-standard body
+--
+CREATE FUNCTION functest_S_1(a text, b date) RETURNS boolean
+ LANGUAGE SQL
+ RETURN a = 'abcd' AND b > '2001-01-01';
+CREATE FUNCTION functest_S_2(a text[]) RETURNS int
+ RETURN a[1]::int;
+CREATE FUNCTION functest_S_3() RETURNS boolean
+ RETURN false;
+CREATE FUNCTION functest_S_3a() RETURNS boolean
+ BEGIN ATOMIC
+ RETURN false;
+ END;
+CREATE FUNCTION functest_S_10(a text, b date) RETURNS boolean
+ LANGUAGE SQL
+ BEGIN ATOMIC
+ SELECT a = 'abcd' AND b > '2001-01-01';
+ END;
+CREATE FUNCTION functest_S_13() RETURNS boolean
+ BEGIN ATOMIC
+ SELECT 1;
+ SELECT false;
+ END;
+-- polymorphic arguments not allowed in this form
+CREATE FUNCTION functest_S_xx(x anyarray) RETURNS anyelement
+ LANGUAGE SQL
+ RETURN x[1];
+ERROR: SQL function with unquoted function body cannot have polymorphic arguments
+SELECT functest_S_1('abcd', '2020-01-01');
+ functest_s_1
+--------------
+ t
+(1 row)
+
+SELECT functest_S_2(ARRAY['1', '2', '3']);
+ functest_s_2
+--------------
+ 1
+(1 row)
+
+SELECT functest_S_3();
+ functest_s_3
+--------------
+ f
+(1 row)
+
+SELECT functest_S_10('abcd', '2020-01-01');
+ functest_s_10
+---------------
+ t
+(1 row)
+
+SELECT functest_S_13();
+ functest_s_13
+---------------
+ f
+(1 row)
+
+SELECT pg_get_functiondef('functest_S_1'::regproc);
+ pg_get_functiondef
+------------------------------------------------------------------------
+ CREATE OR REPLACE FUNCTION temp_func_test.functest_s_1(a text, b date)+
+ RETURNS boolean +
+ LANGUAGE sql +
+ RETURN ((a = 'abcd'::text) AND (b > '01-01-2001'::date)) +
+ +
+
+(1 row)
+
+SELECT pg_get_functiondef('functest_S_2'::regproc);
+ pg_get_functiondef
+------------------------------------------------------------------
+ CREATE OR REPLACE FUNCTION temp_func_test.functest_s_2(a text[])+
+ RETURNS integer +
+ LANGUAGE sql +
+ RETURN ((a)[1])::integer +
+ +
+
+(1 row)
+
+SELECT pg_get_functiondef('functest_S_3'::regproc);
+ pg_get_functiondef
+----------------------------------------------------------
+ CREATE OR REPLACE FUNCTION temp_func_test.functest_s_3()+
+ RETURNS boolean +
+ LANGUAGE sql +
+ RETURN false +
+ +
+
+(1 row)
+
+SELECT pg_get_functiondef('functest_S_3a'::regproc);
+ pg_get_functiondef
+-----------------------------------------------------------
+ CREATE OR REPLACE FUNCTION temp_func_test.functest_s_3a()+
+ RETURNS boolean +
+ LANGUAGE sql +
+ BEGIN ATOMIC +
+ RETURN false; +
+ END +
+
+(1 row)
+
+SELECT pg_get_functiondef('functest_S_10'::regproc);
+ pg_get_functiondef
+-------------------------------------------------------------------------
+ CREATE OR REPLACE FUNCTION temp_func_test.functest_s_10(a text, b date)+
+ RETURNS boolean +
+ LANGUAGE sql +
+ BEGIN ATOMIC +
+ SELECT ((a = 'abcd'::text) AND (b > '01-01-2001'::date)); +
+ END +
+
+(1 row)
+
+SELECT pg_get_functiondef('functest_S_13'::regproc);
+ pg_get_functiondef
+-----------------------------------------------------------
+ CREATE OR REPLACE FUNCTION temp_func_test.functest_s_13()+
+ RETURNS boolean +
+ LANGUAGE sql +
+ BEGIN ATOMIC +
+ SELECT 1; +
+ SELECT false AS bool; +
+ END +
+
+(1 row)
+
+-- test with views
+CREATE TABLE functest3 (a int);
+INSERT INTO functest3 VALUES (1), (2);
+CREATE VIEW functestv3 AS SELECT * FROM functest3;
+CREATE FUNCTION functest_S_14() RETURNS bigint
+ RETURN (SELECT count(*) FROM functestv3);
+SELECT functest_S_14();
+ functest_s_14
+---------------
+ 2
+(1 row)
+
+DROP TABLE functest3 CASCADE;
+NOTICE: drop cascades to 2 other objects
+DETAIL: drop cascades to view functestv3
+drop cascades to function functest_s_14()
-- information_schema tests
CREATE FUNCTION functest_IS_1(a int, b int default 1, c text default 'foo')
RETURNS int
@@ -284,6 +429,20 @@ SELECT routine_name, ordinal_position, parameter_name, parameter_default
(7 rows)
DROP FUNCTION functest_IS_1(int, int, text), functest_IS_2(int), functest_IS_3(int);
+CREATE TABLE functest1 (a int, b int);
+CREATE SEQUENCE functest2;
+CREATE FUNCTION functest_IS_4()
+ RETURNS int
+ LANGUAGE SQL
+ RETURN (SELECT count(a) FROM functest1);
+CREATE FUNCTION functest_IS_5()
+ RETURNS int
+ LANGUAGE SQL
+ RETURN nextval('functest2');
+DROP TABLE functest1 CASCADE;
+NOTICE: drop cascades to function functest_is_4()
+DROP SEQUENCE functest2 CASCADE;
+NOTICE: drop cascades to function functest_is_5()
-- overload
CREATE FUNCTION functest_B_2(bigint) RETURNS bool LANGUAGE 'sql'
IMMUTABLE AS 'SELECT $1 > 0';
@@ -302,6 +461,49 @@ CREATE OR REPLACE PROCEDURE functest1(a int) LANGUAGE SQL AS 'SELECT $1';
ERROR: cannot change routine kind
DETAIL: "functest1" is a function.
DROP FUNCTION functest1(a int);
+-- inlining of set-returning functions
+CREATE FUNCTION functest_sri1() RETURNS SETOF int
+LANGUAGE SQL
+STABLE
+AS '
+ VALUES (1), (2), (3);
+';
+SELECT * FROM functest_sri1();
+ functest_sri1
+---------------
+ 1
+ 2
+ 3
+(3 rows)
+
+EXPLAIN (verbose, costs off) SELECT * FROM functest_sri1();
+ QUERY PLAN
+------------------------------
+ Values Scan on "*VALUES*"
+ Output: "*VALUES*".column1
+(2 rows)
+
+CREATE FUNCTION functest_sri2() RETURNS SETOF int
+LANGUAGE SQL
+STABLE
+BEGIN ATOMIC
+ VALUES (1), (2), (3);
+END;
+SELECT * FROM functest_sri2();
+ functest_sri2
+---------------
+ 1
+ 2
+ 3
+(3 rows)
+
+EXPLAIN (verbose, costs off) SELECT * FROM functest_sri2();
+ QUERY PLAN
+------------------------------
+ Values Scan on "*VALUES*"
+ Output: "*VALUES*".column1
+(2 rows)
+
-- Check behavior of VOID-returning SQL functions
CREATE FUNCTION voidtest1(a int) RETURNS VOID LANGUAGE SQL AS
$$ SELECT a + 1 $$;
@@ -360,7 +562,7 @@ SELECT * FROM voidtest5(3);
-- Cleanup
DROP SCHEMA temp_func_test CASCADE;
-NOTICE: drop cascades to 21 other objects
+NOTICE: drop cascades to 29 other objects
DETAIL: drop cascades to function functest_a_1(text,date)
drop cascades to function functest_a_2(text[])
drop cascades to function functest_a_3()
@@ -376,7 +578,15 @@ drop cascades to function functest_f_1(integer)
drop cascades to function functest_f_2(integer)
drop cascades to function functest_f_3(integer)
drop cascades to function functest_f_4(integer)
+drop cascades to function functest_s_1(text,date)
+drop cascades to function functest_s_2(text[])
+drop cascades to function functest_s_3()
+drop cascades to function functest_s_3a()
+drop cascades to function functest_s_10(text,date)
+drop cascades to function functest_s_13()
drop cascades to function functest_b_2(bigint)
+drop cascades to function functest_sri1()
+drop cascades to function functest_sri2()
drop cascades to function voidtest1(integer)
drop cascades to function voidtest2(integer,integer)
drop cascades to function voidtest3(integer)
diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out
index 3838fa2324..5d634570c4 100644
--- a/src/test/regress/expected/create_procedure.out
+++ b/src/test/regress/expected/create_procedure.out
@@ -65,6 +65,41 @@ SELECT * FROM cp_test ORDER BY b COLLATE "C";
1 | xyzzy
(3 rows)
+-- SQL-standard body
+CREATE PROCEDURE ptest1s(x text)
+LANGUAGE SQL
+BEGIN ATOMIC
+ INSERT INTO cp_test VALUES (1, x);
+END;
+\df ptest1s
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type
+--------+---------+------------------+---------------------+------
+ public | ptest1s | | x text | proc
+(1 row)
+
+SELECT pg_get_functiondef('ptest1s'::regproc);
+ pg_get_functiondef
+----------------------------------------------------
+ CREATE OR REPLACE PROCEDURE public.ptest1s(x text)+
+ LANGUAGE sql +
+ BEGIN ATOMIC +
+ INSERT INTO cp_test (a, b) +
+ VALUES (1, x); +
+ END +
+
+(1 row)
+
+CALL ptest1s('b');
+SELECT * FROM cp_test ORDER BY b COLLATE "C";
+ a | b
+---+-------
+ 1 | 0
+ 1 | a
+ 1 | b
+ 1 | xyzzy
+(4 rows)
+
CREATE PROCEDURE ptest2()
LANGUAGE SQL
AS $$
@@ -146,6 +181,28 @@ AS $$
SELECT a = b;
$$;
CALL ptest7(least('a', 'b'), 'a');
+-- empty body
+CREATE PROCEDURE ptest8(x text)
+BEGIN ATOMIC
+END;
+\df ptest8
+ List of functions
+ Schema | Name | Result data type | Argument data types | Type
+--------+--------+------------------+---------------------+------
+ public | ptest8 | | x text | proc
+(1 row)
+
+SELECT pg_get_functiondef('ptest8'::regproc);
+ pg_get_functiondef
+---------------------------------------------------
+ CREATE OR REPLACE PROCEDURE public.ptest8(x text)+
+ LANGUAGE sql +
+ BEGIN ATOMIC +
+ END +
+
+(1 row)
+
+CALL ptest8('');
-- OUT parameters
CREATE PROCEDURE ptest9(OUT a int)
LANGUAGE SQL
@@ -214,6 +271,7 @@ ALTER ROUTINE ptest1a RENAME TO ptest1;
DROP ROUTINE cp_testfunc1(int);
-- cleanup
DROP PROCEDURE ptest1;
+DROP PROCEDURE ptest1s;
DROP PROCEDURE ptest2;
DROP TABLE cp_test;
DROP USER regress_cp_user1;
diff --git a/src/test/regress/sql/create_function_3.sql b/src/test/regress/sql/create_function_3.sql
index bd108a918f..092e15b591 100644
--- a/src/test/regress/sql/create_function_3.sql
+++ b/src/test/regress/sql/create_function_3.sql
@@ -153,6 +153,65 @@ CREATE FUNCTION functest_F_4(int) RETURNS bool LANGUAGE 'sql'
SELECT pg_get_functiondef('functest_F_2'::regproc);
+--
+-- SQL-standard body
+--
+CREATE FUNCTION functest_S_1(a text, b date) RETURNS boolean
+ LANGUAGE SQL
+ RETURN a = 'abcd' AND b > '2001-01-01';
+CREATE FUNCTION functest_S_2(a text[]) RETURNS int
+ RETURN a[1]::int;
+CREATE FUNCTION functest_S_3() RETURNS boolean
+ RETURN false;
+CREATE FUNCTION functest_S_3a() RETURNS boolean
+ BEGIN ATOMIC
+ RETURN false;
+ END;
+
+CREATE FUNCTION functest_S_10(a text, b date) RETURNS boolean
+ LANGUAGE SQL
+ BEGIN ATOMIC
+ SELECT a = 'abcd' AND b > '2001-01-01';
+ END;
+
+CREATE FUNCTION functest_S_13() RETURNS boolean
+ BEGIN ATOMIC
+ SELECT 1;
+ SELECT false;
+ END;
+
+-- polymorphic arguments not allowed in this form
+CREATE FUNCTION functest_S_xx(x anyarray) RETURNS anyelement
+ LANGUAGE SQL
+ RETURN x[1];
+
+SELECT functest_S_1('abcd', '2020-01-01');
+SELECT functest_S_2(ARRAY['1', '2', '3']);
+SELECT functest_S_3();
+
+SELECT functest_S_10('abcd', '2020-01-01');
+SELECT functest_S_13();
+
+SELECT pg_get_functiondef('functest_S_1'::regproc);
+SELECT pg_get_functiondef('functest_S_2'::regproc);
+SELECT pg_get_functiondef('functest_S_3'::regproc);
+SELECT pg_get_functiondef('functest_S_3a'::regproc);
+SELECT pg_get_functiondef('functest_S_10'::regproc);
+SELECT pg_get_functiondef('functest_S_13'::regproc);
+
+-- test with views
+CREATE TABLE functest3 (a int);
+INSERT INTO functest3 VALUES (1), (2);
+CREATE VIEW functestv3 AS SELECT * FROM functest3;
+
+CREATE FUNCTION functest_S_14() RETURNS bigint
+ RETURN (SELECT count(*) FROM functestv3);
+
+SELECT functest_S_14();
+
+DROP TABLE functest3 CASCADE;
+
+
-- information_schema tests
CREATE FUNCTION functest_IS_1(a int, b int default 1, c text default 'foo')
@@ -177,6 +236,23 @@ CREATE FUNCTION functest_IS_3(a int default 1, out b int)
DROP FUNCTION functest_IS_1(int, int, text), functest_IS_2(int), functest_IS_3(int);
+CREATE TABLE functest1 (a int, b int);
+CREATE SEQUENCE functest2;
+
+CREATE FUNCTION functest_IS_4()
+ RETURNS int
+ LANGUAGE SQL
+ RETURN (SELECT count(a) FROM functest1);
+
+CREATE FUNCTION functest_IS_5()
+ RETURNS int
+ LANGUAGE SQL
+ RETURN nextval('functest2');
+
+DROP TABLE functest1 CASCADE;
+DROP SEQUENCE functest2 CASCADE;
+
+
-- overload
CREATE FUNCTION functest_B_2(bigint) RETURNS bool LANGUAGE 'sql'
IMMUTABLE AS 'SELECT $1 > 0';
@@ -194,6 +270,29 @@ CREATE OR REPLACE PROCEDURE functest1(a int) LANGUAGE SQL AS 'SELECT $1';
DROP FUNCTION functest1(a int);
+-- inlining of set-returning functions
+
+CREATE FUNCTION functest_sri1() RETURNS SETOF int
+LANGUAGE SQL
+STABLE
+AS '
+ VALUES (1), (2), (3);
+';
+
+SELECT * FROM functest_sri1();
+EXPLAIN (verbose, costs off) SELECT * FROM functest_sri1();
+
+CREATE FUNCTION functest_sri2() RETURNS SETOF int
+LANGUAGE SQL
+STABLE
+BEGIN ATOMIC
+ VALUES (1), (2), (3);
+END;
+
+SELECT * FROM functest_sri2();
+EXPLAIN (verbose, costs off) SELECT * FROM functest_sri2();
+
+
-- Check behavior of VOID-returning SQL functions
CREATE FUNCTION voidtest1(a int) RETURNS VOID LANGUAGE SQL AS
diff --git a/src/test/regress/sql/create_procedure.sql b/src/test/regress/sql/create_procedure.sql
index 2ef1c82cea..8c0d70cb16 100644
--- a/src/test/regress/sql/create_procedure.sql
+++ b/src/test/regress/sql/create_procedure.sql
@@ -28,6 +28,21 @@ CREATE PROCEDURE ptest1(x text)
SELECT * FROM cp_test ORDER BY b COLLATE "C";
+-- SQL-standard body
+CREATE PROCEDURE ptest1s(x text)
+LANGUAGE SQL
+BEGIN ATOMIC
+ INSERT INTO cp_test VALUES (1, x);
+END;
+
+\df ptest1s
+SELECT pg_get_functiondef('ptest1s'::regproc);
+
+CALL ptest1s('b');
+
+SELECT * FROM cp_test ORDER BY b COLLATE "C";
+
+
CREATE PROCEDURE ptest2()
LANGUAGE SQL
AS $$
@@ -112,6 +127,16 @@ CREATE PROCEDURE ptest7(a text, b text)
CALL ptest7(least('a', 'b'), 'a');
+-- empty body
+CREATE PROCEDURE ptest8(x text)
+BEGIN ATOMIC
+END;
+
+\df ptest8
+SELECT pg_get_functiondef('ptest8'::regproc);
+CALL ptest8('');
+
+
-- OUT parameters
CREATE PROCEDURE ptest9(OUT a int)
@@ -170,6 +195,7 @@ CREATE USER regress_cp_user1;
-- cleanup
DROP PROCEDURE ptest1;
+DROP PROCEDURE ptest1s;
DROP PROCEDURE ptest2;
DROP TABLE cp_test;
base-commit: 2f0760c9ff827bb3d23ee327e6b46038000c7ef9
--
2.28.0
--------------AC3B021D8F5CFCF564639AE4--
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
@ 2022-04-12 16:27 Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Nathan Bossart @ 2022-04-12 16:27 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; pgsql-hackers
On Tue, Apr 12, 2022 at 03:46:31PM +0900, Kyotaro Horiguchi wrote:
> At Mon, 11 Apr 2022 09:52:57 -0700, Nathan Bossart <[email protected]> wrote in
>> I traced this back a while ago. I believe the link() was first added in
>> November 2000 as part of f0e37a8. This even predates WAL recycling, which
>> was added in July 2001 as part of 7d4d5c0.
>
> f0e37a8 lacks discussion.. It introduced the CHECKPOINT command from
> somwhere out of the ML.. This patch changed XLogFileInit to
> supportusing existent files so that XLogWrite can use the new segment
> provided by checkpoint and still allow XLogWrite to create a new
> segment by itself.
Yeah, I've been unable to find any discussion besides a brief reference to
adding checkpointing [0].
[0] https://postgr.es/m/8F4C99C66D04D4118F580090272A7A23018D85%40sectorbase1.sectorbase.com
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-05-01 13:08 ` Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:36 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
0 siblings, 2 replies; 16+ messages in thread
From: Michael Paquier @ 2022-05-01 13:08 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Tue, Apr 12, 2022 at 09:27:42AM -0700, Nathan Bossart wrote:
> On Tue, Apr 12, 2022 at 03:46:31PM +0900, Kyotaro Horiguchi wrote:
>> At Mon, 11 Apr 2022 09:52:57 -0700, Nathan Bossart <[email protected]> wrote in
>>> I traced this back a while ago. I believe the link() was first added in
>>> November 2000 as part of f0e37a8. This even predates WAL recycling, which
>>> was added in July 2001 as part of 7d4d5c0.
>>
>> f0e37a8 lacks discussion.. It introduced the CHECKPOINT command from
>> somwhere out of the ML.. This patch changed XLogFileInit to
>> supportusing existent files so that XLogWrite can use the new segment
>> provided by checkpoint and still allow XLogWrite to create a new
>> segment by itself.
Yes, I think that you are right here. I also suspect that the
checkpoint command was facing a concurrency issue while working on
the feature and that Vadim saw that this part of the implementation
would be safer in the long run if we use link() followed by unlink().
> Yeah, I've been unable to find any discussion besides a brief reference to
> adding checkpointing [0].
>
> [0] https://postgr.es/m/8F4C99C66D04D4118F580090272A7A23018D85%40sectorbase1.sectorbase.com
While looking at the history of this area, I have also noticed this
argument, telling also that this is a safety measure if this code were
to run in parallel, but that's without counting on the control file
lock hold while doing this operation anyway:
https://www.postgresql.org/message-id/[email protected]
As mentioned already upthread, f0e37a8 is the origin of the
link()/unlink() business in the WAL segment initialization logic, and
also note 1f159e5 that has added a rename() as extra code path for
systems where link() was not working.
At the end, switching directly from durable_rename_excl() to
durable_rename() should be fine for the WAL segment initialization,
but we could do things a bit more carefully by adding a check on the
file existence before calling durable_rename() and issue a elog(LOG)
if a file is found, giving a mean for the WAL recycling to give up
peacefully as it does now. Per my analysis, the TLI history file
created at the end of recovery ought to issue an elog(ERROR).
Now, I am surprised by the third code path of durable_rename_excl(),
as of the WAL receiver doing writeTimeLineHistoryFile(), to not cause
any issues, as link() should exit with EEXIST when the startup process
grabs the same history file concurrently. It seems to me that in this
last case using durable_rename() could be an improvement and prevent
extra WAL receiver restarts as a TLI history fetched from the primary
via streaming or from some archives should be the same, but we could
be more careful, like the WAL init logic, by skipping the
durable_rename() and issuing an elog(LOG). That would not be perfect,
still a bit better than the current state of HEAD.
As we are getting closer to the beta release, it looks safer to let
this change aside a bit longer and wait for v16 to be opened for
business on HEAD.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-05-02 10:48 ` Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2022-05-02 10:48 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Sun, May 01, 2022 at 10:08:53PM +0900, Michael Paquier wrote:
> Now, I am surprised by the third code path of durable_rename_excl(),
> as of the WAL receiver doing writeTimeLineHistoryFile(), to not cause
> any issues, as link() should exit with EEXIST when the startup process
> grabs the same history file concurrently. It seems to me that in this
> last case using durable_rename() could be an improvement and prevent
> extra WAL receiver restarts as a TLI history fetched from the primary
> via streaming or from some archives should be the same, but we could
> be more careful, like the WAL init logic, by skipping the
> durable_rename() and issuing an elog(LOG). That would not be perfect,
> still a bit better than the current state of HEAD.
Skimming through at the buildfarm logs, it happens that the tests are
able to see this race from time to time. Here is one such example on
rorqual:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rorqual&dt=2022-04-20%2004%3A47%3A...
And here are the relevant logs:
2022-04-20 05:04:19.028 UTC [3109048][startup][:0] LOG: restored log
file "00000002.history" from archive
2022-04-20 05:04:19.029 UTC [3109111][walreceiver][:0] LOG: fetching
timeline history file for timeline 2 from primary server
2022-04-20 05:04:19.048 UTC [3109111][walreceiver][:0] FATAL: could
not link file "pg_wal/xlogtemp.3109111" to "pg_wal/00000002.history":
File exists
[...]
2022-04-20 05:04:19.234 UTC [3109250][walreceiver][:0] LOG: started
streaming WAL from primary at 0/3000000 on timeline 2
The WAL receiver upgrades the ERROR to a FATAL, and restarts
streaming shortly after. Using durable_rename() would not be an issue
here.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-05-02 17:39 ` Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Nathan Bossart @ 2022-05-02 17:39 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Mon, May 02, 2022 at 07:48:18PM +0900, Michael Paquier wrote:
> Skimming through at the buildfarm logs, it happens that the tests are
> able to see this race from time to time. Here is one such example on
> rorqual:
> https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rorqual&dt=2022-04-20%2004%3A47%3A...
>
> And here are the relevant logs:
> 2022-04-20 05:04:19.028 UTC [3109048][startup][:0] LOG: restored log
> file "00000002.history" from archive
> 2022-04-20 05:04:19.029 UTC [3109111][walreceiver][:0] LOG: fetching
> timeline history file for timeline 2 from primary server
> 2022-04-20 05:04:19.048 UTC [3109111][walreceiver][:0] FATAL: could
> not link file "pg_wal/xlogtemp.3109111" to "pg_wal/00000002.history":
> File exists
> [...]
> 2022-04-20 05:04:19.234 UTC [3109250][walreceiver][:0] LOG: started
> streaming WAL from primary at 0/3000000 on timeline 2
>
> The WAL receiver upgrades the ERROR to a FATAL, and restarts
> streaming shortly after. Using durable_rename() would not be an issue
> here.
Thanks for investigating this one. I think I agree that we should simply
switch to durable_rename() (without a file existence check beforehand).
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-05-02 23:06 ` Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Nathan Bossart @ 2022-05-02 23:06 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Mon, May 02, 2022 at 10:39:07AM -0700, Nathan Bossart wrote:
> On Mon, May 02, 2022 at 07:48:18PM +0900, Michael Paquier wrote:
>> The WAL receiver upgrades the ERROR to a FATAL, and restarts
>> streaming shortly after. Using durable_rename() would not be an issue
>> here.
>
> Thanks for investigating this one. I think I agree that we should simply
> switch to durable_rename() (without a file existence check beforehand).
Here is a new patch set. For now, I've only removed the file existence
check in writeTimeLineHistoryFile(). I don't know if I'm totally convinced
that there isn't a problem here (e.g., due to concurrent .ready file
creation), but since some platforms have been using rename() for some time,
I don't know how worried we should be. I thought about adding some kind of
locking between the WAL receiver and startup processes, but that seems
excessive. Alternatively, we could just fix xlog.c as proposed earlier
[0]. AFAICT that is the only caller that can experience problems due to
the multiple-hard-link issue. All other callers are simply renaming a
temporary file into place, and the temporary file can be discarded if left
behind after a crash.
[0] https://postgr.es/m/20220407182954.GA1231544%40nathanxps13
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-05-05 11:10 ` Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2022-05-05 11:10 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Mon, May 02, 2022 at 04:06:13PM -0700, Nathan Bossart wrote:
> Here is a new patch set. For now, I've only removed the file existence
> check in writeTimeLineHistoryFile(). I don't know if I'm totally convinced
> that there isn't a problem here (e.g., due to concurrent .ready file
> creation), but since some platforms have been using rename() for some time,
> I don't know how worried we should be.
That's only about Windows these days, meaning that there is much less
coverage in this code path.
> I thought about adding some kind of
> locking between the WAL receiver and startup processes, but that seems
> excessive.
Agreed.
> Alternatively, we could just fix xlog.c as proposed earlier
> [0]. AFAICT that is the only caller that can experience problems due to
> the multiple-hard-link issue. All other callers are simply renaming a
> temporary file into place, and the temporary file can be discarded if left
> behind after a crash.
I'd agree with removing all the callers at the end. pgrename() is
quite robust on Windows, but I'd keep the two checks in
writeTimeLineHistory(), as the logic around findNewestTimeLine() would
consider a past TLI history file as in-use even if we have a crash
just after the file got created in the same path by the same standby,
and the WAL segment init part. Your patch does that.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../YnOwioN%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-07-05 01:19 ` Michael Paquier <[email protected]>
2022-07-05 16:58 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2024-12-18 00:50 ` Back-patch of: avoid multiple hard links to same WAL file after a crash Robert Pang <[email protected]>
0 siblings, 2 replies; 16+ messages in thread
From: Michael Paquier @ 2022-07-05 01:19 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Thu, May 05, 2022 at 08:10:02PM +0900, Michael Paquier wrote:
> I'd agree with removing all the callers at the end. pgrename() is
> quite robust on Windows, but I'd keep the two checks in
> writeTimeLineHistory(), as the logic around findNewestTimeLine() would
> consider a past TLI history file as in-use even if we have a crash
> just after the file got created in the same path by the same standby,
> and the WAL segment init part. Your patch does that.
As v16 is now open for business, I have revisited this change and
applied 0001 to change all the callers (aka removal of the assertion
for the WAL receiver when it overwrites a TLI history file). The
commit log includes details about the reasoning of all the areas
changed, for clarity, as of the WAL recycling part, the TLI history
file part and basic_archive.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../YsORtfT4+fXj%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-07-05 16:58 ` Nathan Bossart <[email protected]>
2022-07-05 23:57 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: Nathan Bossart @ 2022-07-05 16:58 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Tue, Jul 05, 2022 at 10:19:49AM +0900, Michael Paquier wrote:
> On Thu, May 05, 2022 at 08:10:02PM +0900, Michael Paquier wrote:
>> I'd agree with removing all the callers at the end. pgrename() is
>> quite robust on Windows, but I'd keep the two checks in
>> writeTimeLineHistory(), as the logic around findNewestTimeLine() would
>> consider a past TLI history file as in-use even if we have a crash
>> just after the file got created in the same path by the same standby,
>> and the WAL segment init part. Your patch does that.
>
> As v16 is now open for business, I have revisited this change and
> applied 0001 to change all the callers (aka removal of the assertion
> for the WAL receiver when it overwrites a TLI history file). The
> commit log includes details about the reasoning of all the areas
> changed, for clarity, as of the WAL recycling part, the TLI history
> file part and basic_archive.
Thanks! I wonder if we should add a comment in writeTimeLineHistoryFile()
about possible concurrent use by a WAL receiver and the startup process and
why that is okay.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 16:58 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2022-07-05 23:57 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Michael Paquier @ 2022-07-05 23:57 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Tue, Jul 05, 2022 at 09:58:38AM -0700, Nathan Bossart wrote:
> Thanks! I wonder if we should add a comment in writeTimeLineHistoryFile()
> about possible concurrent use by a WAL receiver and the startup process and
> why that is okay.
Agreed. Adding an extra note at the top of the routine would help in
the future.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Back-patch of: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2024-12-18 00:50 ` Robert Pang <[email protected]>
2024-12-18 16:38 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: Robert Pang @ 2024-12-18 00:50 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; [email protected]; pgsql-hackers
Dear team,
We recently observed a few cases where Postgres running on Linux
encountered an issue with WAL segment files. Specifically, two WAL
segments were linked to the same physical file after Postgres ran out
of memory and the OOM killer terminated one of its processes. This
resulted in the WAL segments overwriting each other and Postgres
failing a later recovery.
We found this fix [1] that has been applied to Postgres 16, but the
cases we observed were running Postgres 15. Given that older major
versions will be supported for a good number of years, and the
potential for irrecoverability exists (even if rare), we would like to
discuss the possibility of back-patching this fix.
Are there any technical reasons not to back-patch this fix to older
major versions?
Thank you for your consideration.
Sincerely,
Robert Pang
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=dac1ff3
On Sat, May 7, 2022 at 1:19 AM Michael Paquier <[email protected]> wrote:
>
> On Thu, May 05, 2022 at 08:10:02PM +0900, Michael Paquier wrote:
> > I'd agree with removing all the callers at the end. pgrename() is
> > quite robust on Windows, but I'd keep the two checks in
> > writeTimeLineHistory(), as the logic around findNewestTimeLine() would
> > consider a past TLI history file as in-use even if we have a crash
> > just after the file got created in the same path by the same standby,
> > and the WAL segment init part. Your patch does that.
>
> As v16 is now open for business, I have revisited this change and
> applied 0001 to change all the callers (aka removal of the assertion
> for the WAL receiver when it overwrites a TLI history file). The
> commit log includes details about the reasoning of all the areas
> changed, for clarity, as of the WAL recycling part, the TLI history
> file part and basic_archive.
> --
> Michael
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2024-12-18 00:50 ` Back-patch of: avoid multiple hard links to same WAL file after a crash Robert Pang <[email protected]>
@ 2024-12-18 16:38 ` Nathan Bossart <[email protected]>
2024-12-19 01:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Andres Freund <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Nathan Bossart @ 2024-12-18 16:38 UTC (permalink / raw)
To: Robert Pang <[email protected]>; +Cc: Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; [email protected]; pgsql-hackers
On Tue, Dec 17, 2024 at 04:50:16PM -0800, Robert Pang wrote:
> We recently observed a few cases where Postgres running on Linux
> encountered an issue with WAL segment files. Specifically, two WAL
> segments were linked to the same physical file after Postgres ran out
> of memory and the OOM killer terminated one of its processes. This
> resulted in the WAL segments overwriting each other and Postgres
> failing a later recovery.
Yikes!
> We found this fix [1] that has been applied to Postgres 16, but the
> cases we observed were running Postgres 15. Given that older major
> versions will be supported for a good number of years, and the
> potential for irrecoverability exists (even if rare), we would like to
> discuss the possibility of back-patching this fix.
IMHO this is a good time to reevaluate. It looks like we originally didn't
back-patch out of an abundance of caution, but now that this one has had
time to bake, I think it's worth seriously considering, especially now that
we have a report from the field.
--
nathan
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2024-12-18 00:50 ` Back-patch of: avoid multiple hard links to same WAL file after a crash Robert Pang <[email protected]>
2024-12-18 16:38 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
@ 2024-12-19 01:51 ` Andres Freund <[email protected]>
2024-12-19 05:44 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Andres Freund @ 2024-12-19 01:51 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Robert Pang <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; [email protected]; pgsql-hackers
Hi,
On 2024-12-18 10:38:19 -0600, Nathan Bossart wrote:
> On Tue, Dec 17, 2024 at 04:50:16PM -0800, Robert Pang wrote:
> > We recently observed a few cases where Postgres running on Linux
> > encountered an issue with WAL segment files. Specifically, two WAL
> > segments were linked to the same physical file after Postgres ran out
> > of memory and the OOM killer terminated one of its processes. This
> > resulted in the WAL segments overwriting each other and Postgres
> > failing a later recovery.
>
> Yikes!
Indeed. As chance would have it, I was asked for input on a corrupted server
*today*. Eventually we found that recovery stopped early, after encountering a
segment with a *newer* pageaddr than we expected. Which made me think of this
issue, and indeed, the file recovery stopped at had two links. Before that
the server had been crashing on a regular basis for unrelated reasons, which
presumably increased the chances sufficiently to eventually hit this problem.
It's a normal thing to discover the end of the WAL by finding a segment that
has an older pageaddr than its name suggests. But in this case we saw a newer
page address. I wonder if we should treat that differently...
> > We found this fix [1] that has been applied to Postgres 16, but the
> > cases we observed were running Postgres 15. Given that older major
> > versions will be supported for a good number of years, and the
> > potential for irrecoverability exists (even if rare), we would like to
> > discuss the possibility of back-patching this fix.
>
> IMHO this is a good time to reevaluate. It looks like we originally didn't
> back-patch out of an abundance of caution, but now that this one has had
> time to bake, I think it's worth seriously considering, especially now that
> we have a report from the field.
Strongly agreed.
I don't think the issue is actually quite as unlikely to be hit as reasoned in
the commit message. The crash has indeed to happen between the link() and
unlink() - but at the end of a checkpoint we do that operations hundreds of
times in a row on a busy server. And that's just after potentially doing lots
of write IO during a checkpoint, filling up drive write caches / eating up
IOPS/bandwidth disk quots.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2024-12-18 00:50 ` Back-patch of: avoid multiple hard links to same WAL file after a crash Robert Pang <[email protected]>
2024-12-18 16:38 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2024-12-19 01:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Andres Freund <[email protected]>
@ 2024-12-19 05:44 ` Michael Paquier <[email protected]>
2024-12-19 15:31 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2024-12-19 05:44 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Robert Pang <[email protected]>; Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; [email protected]; pgsql-hackers
On Wed, Dec 18, 2024 at 08:51:20PM -0500, Andres Freund wrote:
> I don't think the issue is actually quite as unlikely to be hit as reasoned in
> the commit message. The crash has indeed to happen between the link() and
> unlink() - but at the end of a checkpoint we do that operations hundreds of
> times in a row on a busy server. And that's just after potentially doing lots
> of write IO during a checkpoint, filling up drive write caches / eating up
> IOPS/bandwidth disk quots.
Looks so, yep. Your timing and the report's timing are interesting.
I've been double-checking the code to refresh myself with the problem,
and I don't see a reason to not apply something like the attached set
down to v13 for all these remaining branches (minus an edit of the
commit message).
Thoughts?
--
Michael
Attachments:
[text/x-diff] 0001-Replace-durable_rename_excl-by-durable_rename-ta-v15.patch (6.7K, ../../[email protected]/2-0001-Replace-durable_rename_excl-by-durable_rename-ta-v15.patch)
download | inline diff:
From 168c7071541d231fc13aee424709524f2ed4976e Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 5 Jul 2022 10:16:12 +0900
Subject: [PATCH] Replace durable_rename_excl() by durable_rename(), take two
durable_rename_excl() attempts to avoid overwriting any existing files
by using link() and unlink(), and it falls back to rename() on some
platforms (aka WIN32), which offers no such overwrite protection. Most
callers use durable_rename_excl() just in case there is an existing
file, but in practice there shouldn't be one (see below for more
details).
Furthermore, failures during durable_rename_excl() can result in
multiple hard links to the same file. As per Nathan's tests, it is
possible to end up with two links to the same file in pg_wal after a
crash just before unlink() during WAL recycling. Specifically, the test
produced links to the same file for the current WAL file and the next
one because the half-recycled WAL file was re-recycled upon restarting,
leading to WAL corruption.
This change replaces all the calls of durable_rename_excl() to
durable_rename(). This removes the protection against accidentally
overwriting an existing file, but some platforms are already living
without it and ordinarily there shouldn't be one. The function itself
is left around in case any extensions are using it. It will be removed
on HEAD via a follow-up commit.
Here is a summary of the existing callers of durable_rename_excl() (see
second discussion link at the bottom), replaced by this commit. First,
basic_archive used it to avoid overwriting an archive concurrently
created by another server, but as mentioned above, it will still
overwrite files on some platforms. Second, xlog.c uses it to recycle
past WAL segments, where an overwrite should not happen (origin of the
change at f0e37a8) because there are protections about the WAL segment
to select when recycling an entry. The third and last area is related
to the write of timeline history files. writeTimeLineHistory() will
write a new timeline history file at the end of recovery on promotion,
so there should be no such files for the same timeline.
What remains is writeTimeLineHistoryFile(), that can be used in parallel
by a WAL receiver and the startup process, and some digging of the
buildfarm shows that EEXIST from a WAL receiver can happen with an error
of "could not link file \"pg_wal/xlogtemp.NN\" to \"pg_wal/MM.history\",
which would cause an automatic restart of the WAL receiver as it is
promoted to FATAL, hence this should improve the stability of the WAL
receiver as rename() would overwrite an existing TLI history file
already fetched by the startup process at recovery.
This is a bug fix, but knowing the unlikeliness of the problem involving
one or more crashes at an exceptionally bad moment, no backpatch is
done. Also, I want to be careful with such changes (aaa3aed did the
opposite of this change by removing HAVE_WORKING_LINK so as Windows
would do a link() rather than a rename() but this was not
concurrent-safe). A backpatch could be revisited in the future. This
is the second time this change is attempted, ccfbd92 being the first
one, but this time no assertions are added for the case of a TLI history
file written concurrently by the WAL receiver or the startup process
because we can expect one to exist (some of the TAP tests are able to
trigger with a proper timing).
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20220407182954.GA1231544@nathanxps13
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/transam/timeline.c | 18 +++++-------------
src/backend/access/transam/xlog.c | 9 +++------
contrib/basic_archive/basic_archive.c | 5 +++--
3 files changed, 11 insertions(+), 21 deletions(-)
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index be21968293..e0a2a8ea68 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -441,12 +441,8 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
* Now move the completed history file into place with its final name.
*/
TLHistoryFilePath(path, newTLI);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ durable_rename(tmppath, path, ERROR);
/* The history file can be archived immediately. */
if (XLogArchivingActive())
@@ -516,15 +512,11 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
errmsg("could not close file \"%s\": %m", tmppath)));
/*
- * Now move the completed history file into place with its final name.
+ * Now move the completed history file into place with its final name,
+ * replacing any existing file with the same name.
*/
TLHistoryFilePath(path, tli);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ durable_rename(tmppath, path, ERROR);
}
/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5d8322fbd0..818284759d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3326,14 +3326,11 @@ InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
}
}
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- if (durable_rename_excl(tmppath, path, LOG) != 0)
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ if (durable_rename(tmppath, path, LOG) != 0)
{
LWLockRelease(ControlFileLock);
- /* durable_rename_excl already emitted log message */
+ /* durable_rename already emitted log message */
return false;
}
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 87dd77cdd3..db5fd87429 100644
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -282,9 +282,10 @@ basic_archive_file_internal(const char *file, const char *path)
/*
* Sync the temporary file to disk and move it to its final destination.
- * This will fail if destination already exists.
+ * Note that this will overwrite any existing file, but this is only
+ * possible if someone else created the file since the stat() above.
*/
- (void) durable_rename_excl(temp, destination, ERROR);
+ (void) durable_rename(temp, destination, ERROR);
ereport(DEBUG1,
(errmsg("archived \"%s\" via basic_archive", file)));
--
2.45.2
[text/x-diff] 0001-Replace-durable_rename_excl-by-durable_rename-ta-v14.patch (5.9K, ../../[email protected]/3-0001-Replace-durable_rename_excl-by-durable_rename-ta-v14.patch)
download | inline diff:
From 51649664dd7549ea2f7cadc26d024a3ea56d684a Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 5 Jul 2022 10:16:12 +0900
Subject: [PATCH] Replace durable_rename_excl() by durable_rename(), take two
durable_rename_excl() attempts to avoid overwriting any existing files
by using link() and unlink(), and it falls back to rename() on some
platforms (aka WIN32), which offers no such overwrite protection. Most
callers use durable_rename_excl() just in case there is an existing
file, but in practice there shouldn't be one (see below for more
details).
Furthermore, failures during durable_rename_excl() can result in
multiple hard links to the same file. As per Nathan's tests, it is
possible to end up with two links to the same file in pg_wal after a
crash just before unlink() during WAL recycling. Specifically, the test
produced links to the same file for the current WAL file and the next
one because the half-recycled WAL file was re-recycled upon restarting,
leading to WAL corruption.
This change replaces all the calls of durable_rename_excl() to
durable_rename(). This removes the protection against accidentally
overwriting an existing file, but some platforms are already living
without it and ordinarily there shouldn't be one. The function itself
is left around in case any extensions are using it. It will be removed
on HEAD via a follow-up commit.
Here is a summary of the existing callers of durable_rename_excl() (see
second discussion link at the bottom), replaced by this commit. First,
basic_archive used it to avoid overwriting an archive concurrently
created by another server, but as mentioned above, it will still
overwrite files on some platforms. Second, xlog.c uses it to recycle
past WAL segments, where an overwrite should not happen (origin of the
change at f0e37a8) because there are protections about the WAL segment
to select when recycling an entry. The third and last area is related
to the write of timeline history files. writeTimeLineHistory() will
write a new timeline history file at the end of recovery on promotion,
so there should be no such files for the same timeline.
What remains is writeTimeLineHistoryFile(), that can be used in parallel
by a WAL receiver and the startup process, and some digging of the
buildfarm shows that EEXIST from a WAL receiver can happen with an error
of "could not link file \"pg_wal/xlogtemp.NN\" to \"pg_wal/MM.history\",
which would cause an automatic restart of the WAL receiver as it is
promoted to FATAL, hence this should improve the stability of the WAL
receiver as rename() would overwrite an existing TLI history file
already fetched by the startup process at recovery.
This is a bug fix, but knowing the unlikeliness of the problem involving
one or more crashes at an exceptionally bad moment, no backpatch is
done. Also, I want to be careful with such changes (aaa3aed did the
opposite of this change by removing HAVE_WORKING_LINK so as Windows
would do a link() rather than a rename() but this was not
concurrent-safe). A backpatch could be revisited in the future. This
is the second time this change is attempted, ccfbd92 being the first
one, but this time no assertions are added for the case of a TLI history
file written concurrently by the WAL receiver or the startup process
because we can expect one to exist (some of the TAP tests are able to
trigger with a proper timing).
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20220407182954.GA1231544@nathanxps13
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/transam/timeline.c | 18 +++++-------------
src/backend/access/transam/xlog.c | 9 +++------
2 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index 8d0903c175..eeb505fb6a 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -441,12 +441,8 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
* Now move the completed history file into place with its final name.
*/
TLHistoryFilePath(path, newTLI);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ durable_rename(tmppath, path, ERROR);
/* The history file can be archived immediately. */
if (XLogArchivingActive())
@@ -516,15 +512,11 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
errmsg("could not close file \"%s\": %m", tmppath)));
/*
- * Now move the completed history file into place with its final name.
+ * Now move the completed history file into place with its final name,
+ * replacing any existing file with the same name.
*/
TLHistoryFilePath(path, tli);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ durable_rename(tmppath, path, ERROR);
}
/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f1a795bba9..a6e2cb88f3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3693,15 +3693,12 @@ InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
}
}
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- if (durable_rename_excl(tmppath, path, LOG) != 0)
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ if (durable_rename(tmppath, path, LOG) != 0)
{
if (use_lock)
LWLockRelease(ControlFileLock);
- /* durable_rename_excl already emitted log message */
+ /* durable_rename already emitted log message */
return false;
}
--
2.45.2
[text/x-diff] 0001-Replace-durable_rename_excl-by-durable_rename-ta-v13.patch (5.9K, ../../[email protected]/4-0001-Replace-durable_rename_excl-by-durable_rename-ta-v13.patch)
download | inline diff:
From 93c566230e5686da1b1d40186f31b79339be2bcf Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 5 Jul 2022 10:16:12 +0900
Subject: [PATCH] Replace durable_rename_excl() by durable_rename(), take two
durable_rename_excl() attempts to avoid overwriting any existing files
by using link() and unlink(), and it falls back to rename() on some
platforms (aka WIN32), which offers no such overwrite protection. Most
callers use durable_rename_excl() just in case there is an existing
file, but in practice there shouldn't be one (see below for more
details).
Furthermore, failures during durable_rename_excl() can result in
multiple hard links to the same file. As per Nathan's tests, it is
possible to end up with two links to the same file in pg_wal after a
crash just before unlink() during WAL recycling. Specifically, the test
produced links to the same file for the current WAL file and the next
one because the half-recycled WAL file was re-recycled upon restarting,
leading to WAL corruption.
This change replaces all the calls of durable_rename_excl() to
durable_rename(). This removes the protection against accidentally
overwriting an existing file, but some platforms are already living
without it and ordinarily there shouldn't be one. The function itself
is left around in case any extensions are using it. It will be removed
on HEAD via a follow-up commit.
Here is a summary of the existing callers of durable_rename_excl() (see
second discussion link at the bottom), replaced by this commit. First,
basic_archive used it to avoid overwriting an archive concurrently
created by another server, but as mentioned above, it will still
overwrite files on some platforms. Second, xlog.c uses it to recycle
past WAL segments, where an overwrite should not happen (origin of the
change at f0e37a8) because there are protections about the WAL segment
to select when recycling an entry. The third and last area is related
to the write of timeline history files. writeTimeLineHistory() will
write a new timeline history file at the end of recovery on promotion,
so there should be no such files for the same timeline.
What remains is writeTimeLineHistoryFile(), that can be used in parallel
by a WAL receiver and the startup process, and some digging of the
buildfarm shows that EEXIST from a WAL receiver can happen with an error
of "could not link file \"pg_wal/xlogtemp.NN\" to \"pg_wal/MM.history\",
which would cause an automatic restart of the WAL receiver as it is
promoted to FATAL, hence this should improve the stability of the WAL
receiver as rename() would overwrite an existing TLI history file
already fetched by the startup process at recovery.
This is a bug fix, but knowing the unlikeliness of the problem involving
one or more crashes at an exceptionally bad moment, no backpatch is
done. Also, I want to be careful with such changes (aaa3aed did the
opposite of this change by removing HAVE_WORKING_LINK so as Windows
would do a link() rather than a rename() but this was not
concurrent-safe). A backpatch could be revisited in the future. This
is the second time this change is attempted, ccfbd92 being the first
one, but this time no assertions are added for the case of a TLI history
file written concurrently by the WAL receiver or the startup process
because we can expect one to exist (some of the TAP tests are able to
trigger with a proper timing).
Author: Nathan Bossart
Reviewed-by: Robert Haas, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20220407182954.GA1231544@nathanxps13
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/transam/timeline.c | 18 +++++-------------
src/backend/access/transam/xlog.c | 9 +++------
2 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index e6a29d9a9b..517ab023a3 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -441,12 +441,8 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
* Now move the completed history file into place with its final name.
*/
TLHistoryFilePath(path, newTLI);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ durable_rename(tmppath, path, ERROR);
/* The history file can be archived immediately. */
if (XLogArchivingActive())
@@ -516,15 +512,11 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
errmsg("could not close file \"%s\": %m", tmppath)));
/*
- * Now move the completed history file into place with its final name.
+ * Now move the completed history file into place with its final name,
+ * replacing any existing file with the same name.
*/
TLHistoryFilePath(path, tli);
-
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- durable_rename_excl(tmppath, path, ERROR);
+ durable_rename(tmppath, path, ERROR);
}
/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ab4a510ea7..1fa15a58be 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3648,15 +3648,12 @@ InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
}
}
- /*
- * Perform the rename using link if available, paranoidly trying to avoid
- * overwriting an existing file (there shouldn't be one).
- */
- if (durable_rename_excl(tmppath, path, LOG) != 0)
+ Assert(access(path, F_OK) != 0 && errno == ENOENT);
+ if (durable_rename(tmppath, path, LOG) != 0)
{
if (use_lock)
LWLockRelease(ControlFileLock);
- /* durable_rename_excl already emitted log message */
+ /* durable_rename already emitted log message */
return false;
}
--
2.45.2
[application/pgp-signature] signature.asc (833B, ../../[email protected]/5-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Back-patch of: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2024-12-18 00:50 ` Back-patch of: avoid multiple hard links to same WAL file after a crash Robert Pang <[email protected]>
2024-12-18 16:38 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2024-12-19 01:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Andres Freund <[email protected]>
2024-12-19 05:44 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2024-12-19 15:31 ` Nathan Bossart <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Nathan Bossart @ 2024-12-19 15:31 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Pang <[email protected]>; Kyotaro Horiguchi <[email protected]>; Tom Lane <[email protected]>; [email protected]; pgsql-hackers
On Thu, Dec 19, 2024 at 02:44:53PM +0900, Michael Paquier wrote:
> I've been double-checking the code to refresh myself with the problem,
> and I don't see a reason to not apply something like the attached set
> down to v13 for all these remaining branches (minus an edit of the
> commit message).
LGTM
--
nathan
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: avoid multiple hard links to same WAL file after a crash
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
@ 2022-05-02 17:36 ` Nathan Bossart <[email protected]>
1 sibling, 0 replies; 16+ messages in thread
From: Nathan Bossart @ 2022-05-02 17:36 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; pgsql-hackers
On Sun, May 01, 2022 at 10:08:53PM +0900, Michael Paquier wrote:
> At the end, switching directly from durable_rename_excl() to
> durable_rename() should be fine for the WAL segment initialization,
> but we could do things a bit more carefully by adding a check on the
> file existence before calling durable_rename() and issue a elog(LOG)
> if a file is found, giving a mean for the WAL recycling to give up
> peacefully as it does now. Per my analysis, the TLI history file
> created at the end of recovery ought to issue an elog(ERROR).
My only concern with this approach is that it inevitably introduces a race
condition. In most cases, the file existence check will prevent
overwrites, but it might not always. Furthermore, we believe that such
overwrites either 1) should not happen (e.g., WAL recycling) or 2) won't
cause problems if they happen (e.g., when the WAL receiver writes the TLI
history file). Also, these races will be difficult to test, so we won't
know what breaks when they occur.
My instinct is to just let the overwrites happen. That way, we are more
likely to catch breakage in tests, and we'll have one less race condition
to worry about. I don't mind asserting that the file doesn't exist when we
don't expect it to, as that might help catch potential problems in
development without affecting behavior in production. If we do want to
add file existence checks, I think we'd better add a comment about the
potential for race conditions.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2024-12-19 15:31 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-27 13:40 [PATCH v5] SQL-standard function body Peter Eisentraut <[email protected]>
2022-04-12 16:27 Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-01 13:08 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 10:48 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-05-02 17:39 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 23:06 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-05 11:10 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 01:19 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2022-07-05 16:58 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-07-05 23:57 ` Re: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2024-12-18 00:50 ` Back-patch of: avoid multiple hard links to same WAL file after a crash Robert Pang <[email protected]>
2024-12-18 16:38 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2024-12-19 01:51 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Andres Freund <[email protected]>
2024-12-19 05:44 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Michael Paquier <[email protected]>
2024-12-19 15:31 ` Re: Back-patch of: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
2022-05-02 17:36 ` Re: avoid multiple hard links to same WAL file after a crash Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox