use pg_get_functiondef() in pg_dump
Here is a patch to have pg_dump use pg_get_functiondef() instead of
assembling the CREATE FUNCTION/PROCEDURE commands itself. This should
save on maintenance effort in the future. It's also a prerequisite for
being able to dump functions with SQL-standard function body discussed
in [0]/messages/by-id/1c11f1eb-f00c-43b7-799d-2d44132c02d7@2ndquadrant.com.
pg_get_functiondef() was meant for psql's \ef command, so its defaults
are slightly different from what pg_dump would like, so this adds a few
optional parameters for tweaking the behavior. The naming of the
parameters is up for discussion.
[0]: /messages/by-id/1c11f1eb-f00c-43b7-799d-2d44132c02d7@2ndquadrant.com
/messages/by-id/1c11f1eb-f00c-43b7-799d-2d44132c02d7@2ndquadrant.com
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v1-0001-Drop-final-newline-from-pg_get_functiondef-result.patchtext/plain; charset=UTF-8; name=v1-0001-Drop-final-newline-from-pg_get_functiondef-result.patch; x-mac-creator=0; x-mac-type=0Download
From 6d87daace8d5e9cec80d1b3b5d1a03ff9c2206ee Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 12 Aug 2020 09:19:58 +0200
Subject: [PATCH v1 1/2] Drop final newline from pg_get_functiondef result
This makes it more consistent with other pg_get_*def() functions.
psql (for \ef) already ensures that a trailing newline is added as
necessary, so this change doesn't affect psql's functionality.
---
src/backend/utils/adt/ruleutils.c | 2 --
src/test/regress/expected/create_function_3.out | 12 ++++--------
src/test/regress/expected/create_procedure.out | 3 +--
src/test/regress/expected/rules.out | 3 +--
4 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 60dd80c23c..877f99cba3 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2832,8 +2832,6 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
appendStringInfoString(&buf, prosrc);
appendBinaryStringInfo(&buf, dq.data, dq.len);
- appendStringInfoChar(&buf, '\n');
-
ReleaseSysCache(proctup);
PG_RETURN_TEXT_P(string_to_text(buf.data));
diff --git a/src/test/regress/expected/create_function_3.out b/src/test/regress/expected/create_function_3.out
index ba260df996..5d0be17282 100644
--- a/src/test/regress/expected/create_function_3.out
+++ b/src/test/regress/expected/create_function_3.out
@@ -200,8 +200,7 @@ SELECT pg_get_functiondef('functest_A_1'::regproc);
CREATE OR REPLACE FUNCTION temp_func_test.functest_a_1(text, date)+
RETURNS boolean +
LANGUAGE sql +
- AS $function$SELECT $1 = 'abcd' AND $2 > '2001-01-01'$function$ +
-
+ AS $function$SELECT $1 = 'abcd' AND $2 > '2001-01-01'$function$
(1 row)
SELECT pg_get_functiondef('functest_B_3'::regproc);
@@ -211,8 +210,7 @@ SELECT pg_get_functiondef('functest_B_3'::regproc);
RETURNS boolean +
LANGUAGE sql +
STABLE +
- AS $function$SELECT $1 = 0$function$ +
-
+ AS $function$SELECT $1 = 0$function$
(1 row)
SELECT pg_get_functiondef('functest_C_3'::regproc);
@@ -222,8 +220,7 @@ SELECT pg_get_functiondef('functest_C_3'::regproc);
RETURNS boolean +
LANGUAGE sql +
SECURITY DEFINER +
- AS $function$SELECT $1 < 0$function$ +
-
+ AS $function$SELECT $1 < 0$function$
(1 row)
SELECT pg_get_functiondef('functest_F_2'::regproc);
@@ -233,8 +230,7 @@ SELECT pg_get_functiondef('functest_F_2'::regproc);
RETURNS boolean +
LANGUAGE sql +
STRICT +
- AS $function$SELECT $1 = 50$function$ +
-
+ AS $function$SELECT $1 = 50$function$
(1 row)
-- information_schema tests
diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out
index 211a42cefa..2c94b9a4b6 100644
--- a/src/test/regress/expected/create_procedure.out
+++ b/src/test/regress/expected/create_procedure.out
@@ -29,8 +29,7 @@ SELECT pg_get_functiondef('ptest1'::regproc);
LANGUAGE sql +
AS $procedure$ +
INSERT INTO cp_test VALUES (1, x); +
- $procedure$ +
-
+ $procedure$
(1 row)
-- show only normal functions
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 601734a6f1..be434c97ac 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3402,8 +3402,7 @@ SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
SET work_mem TO '4MB' +
SET "DateStyle" TO 'iso, mdy' +
SET local_preload_libraries TO 'Mixed/Case', 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'+
- AS $function$select 1;$function$ +
-
+ AS $function$select 1;$function$
(1 row)
-- tests for pg_get_*def with invalid objects
--
2.28.0
v1-0002-pg_dump-Use-pg_get_functiondef.patchtext/plain; charset=UTF-8; name=v1-0002-pg_dump-Use-pg_get_functiondef.patch; x-mac-creator=0; x-mac-type=0Download
From 23c52476b4cfd007543943d6d1c67baeb87f75af Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 12 Aug 2020 09:51:40 +0200
Subject: [PATCH v1 2/2] pg_dump: Use pg_get_functiondef()
Instead of having pg_dump assemble CREATE FUNCTION commands from the
pieces, use the existing pg_get_functiondef(). This should save
pg_dump maintenance effort in the future.
pg_get_functiondef() was meant for psql's \ef command, so its defaults
are slightly different from what pg_dump would like, so add a few
optional parameters for tweaking the behavior.
---
doc/src/sgml/func.sgml | 17 ++++--
src/backend/catalog/system_views.sql | 4 ++
src/backend/utils/adt/ruleutils.c | 63 ++++++++++++++++-----
src/bin/pg_dump/pg_dump.c | 41 +++++++++++++-
src/bin/pg_dump/t/002_pg_dump.pl | 42 ++++++++------
src/include/catalog/pg_proc.dat | 2 +-
src/test/modules/test_pg_dump/t/001_base.pl | 3 +-
7 files changed, 132 insertions(+), 40 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f766c1bc67..4f0f060d51 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -22076,16 +22076,25 @@ <title>System Catalog Information Functions</title>
<indexterm>
<primary>pg_get_functiondef</primary>
</indexterm>
- <function>pg_get_functiondef</function> ( <parameter>func</parameter> <type>oid</type> )
+ <function>pg_get_functiondef</function> ( <parameter>func</parameter> <type>oid</type> <optional>, <parameter>or_replace</parameter> <type>boolean</type>, <parameter>lavish_quoting</parameter> <type>boolean</type> </optional> )
<returnvalue>text</returnvalue>
</para>
<para>
Reconstructs the creating command for a function or procedure.
(This is a decompiled reconstruction, not the original text
of the command.)
- The result is a complete <command>CREATE OR REPLACE FUNCTION</command>
- or <command>CREATE OR REPLACE PROCEDURE</command> statement.
- </para></entry>
+ The result is a complete <command>CREATE FUNCTION</command>
+ or <command>CREATE PROCEDURE</command> statement.
+ </para>
+ <para>
+ If <parameter>or_replace</parameter> is true (the default), then
+ <literal>CREATE OR REPLACE</literal> commands are generated. If
+ <parameter>lavish_quoting</parameter> is true (the default), then the
+ created command text will use dollar-quoting in a way that is unlikely
+ to clash with any function body. This is useful when the fetched
+ command is meant to be edited by the user. Otherwise, it will try to
+ generate compact quoting adequate for the current function body.
+ </para></entry>
</row>
<row>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 8625cbeab6..823ca7b38a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1203,6 +1203,10 @@ CREATE FUNCTION ts_debug(IN document text,
-- to get filled in.)
--
+CREATE OR REPLACE FUNCTION
+ pg_get_functiondef(func oid, or_replace boolean DEFAULT true, lavish_quoting boolean DEFAULT true)
+ RETURNS text STRICT STABLE LANGUAGE internal AS 'pg_get_functiondef';
+
CREATE OR REPLACE FUNCTION
pg_start_backup(label text, fast boolean DEFAULT false, exclusive boolean DEFAULT true)
RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup'
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 877f99cba3..b1554e351d 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2590,6 +2590,38 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
}
+static void
+appendStringLiteralDQ(StringInfo buf, const char *str, const char *dqprefix)
+{
+ static const char suffixes[] = "_XXXXXXX";
+ int nextchar = 0;
+ StringInfo delimBuf = makeStringInfo();
+
+ /* start with $ + dqprefix if not NULL */
+ appendStringInfoChar(delimBuf, '$');
+ if (dqprefix)
+ appendStringInfoString(delimBuf, dqprefix);
+
+ /*
+ * Make sure we choose a delimiter which (without the trailing $) is not
+ * present in the string being quoted. We don't check with the trailing $
+ * because a string ending in $foo must not be quoted with $foo$.
+ */
+ while (strstr(str, delimBuf->data) != NULL)
+ {
+ appendStringInfoChar(delimBuf, suffixes[nextchar++]);
+ nextchar %= sizeof(suffixes) - 1;
+ }
+
+ /* add trailing $ */
+ appendStringInfoChar(delimBuf, '$');
+
+ /* quote it and we are all done */
+ appendStringInfoString(buf, delimBuf->data);
+ appendStringInfoString(buf, str);
+ appendStringInfoString(buf, delimBuf->data);
+}
+
/*
* pg_get_functiondef
* Returns the complete "CREATE OR REPLACE FUNCTION ..." statement for
@@ -2604,13 +2636,14 @@ Datum
pg_get_functiondef(PG_FUNCTION_ARGS)
{
Oid funcid = PG_GETARG_OID(0);
+ bool or_replace = PG_GETARG_BOOL(1);
+ bool lavish_quoting = PG_GETARG_BOOL(2);
StringInfoData buf;
- StringInfoData dq;
HeapTuple proctup;
Form_pg_proc proc;
bool isfunction;
Datum tmp;
- bool isnull;
+ bool isnull, isnull2;
const char *prosrc;
const char *name;
const char *nsp;
@@ -2639,7 +2672,8 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
* replaced.
*/
nsp = get_namespace_name(proc->pronamespace);
- appendStringInfo(&buf, "CREATE OR REPLACE %s %s(",
+ appendStringInfo(&buf, "CREATE %s%s %s(",
+ or_replace ? "OR REPLACE " : "",
isfunction ? "FUNCTION" : "PROCEDURE",
quote_qualified_identifier(nsp, name));
(void) print_function_arguments(&buf, proctup, false, true);
@@ -2809,8 +2843,8 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
appendStringInfoString(&buf, ", "); /* assume prosrc isn't null */
}
- tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosrc, &isnull);
- if (isnull)
+ tmp = SysCacheGetAttr(PROCOID, proctup, Anum_pg_proc_prosrc, &isnull2);
+ if (isnull2)
elog(ERROR, "null prosrc");
prosrc = TextDatumGetCString(tmp);
@@ -2821,16 +2855,15 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
* shouldn't use a short delimiter that he might easily create a conflict
* with. Hence prefer "$function$"/"$procedure$", but extend if needed.
*/
- initStringInfo(&dq);
- appendStringInfoChar(&dq, '$');
- appendStringInfoString(&dq, (isfunction ? "function" : "procedure"));
- while (strstr(prosrc, dq.data) != NULL)
- appendStringInfoChar(&dq, 'x');
- appendStringInfoChar(&dq, '$');
-
- appendBinaryStringInfo(&buf, dq.data, dq.len);
- appendStringInfoString(&buf, prosrc);
- appendBinaryStringInfo(&buf, dq.data, dq.len);
+ if (isnull)
+ appendStringLiteralDQ(&buf, prosrc, (lavish_quoting ? (isfunction ? "function" : "procedure") : NULL));
+ else
+ {
+ if (strchr(prosrc, '\'') == NULL && strchr(prosrc, '\\') == NULL)
+ simple_quote_literal(&buf, prosrc);
+ else
+ appendStringLiteralDQ(&buf, prosrc, NULL);
+ }
ReleaseSysCache(proctup);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 9c8436dde6..1f50563383 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -11764,6 +11764,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
PQExpBuffer q;
PQExpBuffer delqry;
PQExpBuffer asPart;
+ bool use_functiondef;
PGresult *res;
char *funcsig; /* identity signature */
char *funcfullsig = NULL; /* full signature */
@@ -11808,6 +11809,20 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
delqry = createPQExpBuffer();
asPart = createPQExpBuffer();
+ if (fout->remoteVersion >= 140000)
+ {
+ use_functiondef = true;
+
+ appendPQExpBuffer(query,
+ "SELECT\n"
+ "pg_get_functiondef(oid, false, false) AS functiondef,\n"
+ "prokind,\n"
+ "pg_catalog.pg_get_function_identity_arguments(oid) AS funciargs\n");
+ }
+ else
+ {
+ use_functiondef = false;
+
/* Fetch function-specific details */
appendPQExpBuffer(query,
"SELECT\n"
@@ -11886,6 +11901,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
else
appendPQExpBuffer(query,
"'-' AS prosupport\n");
+ }
appendPQExpBuffer(query,
"FROM pg_catalog.pg_proc "
@@ -11894,6 +11910,13 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
res = ExecuteSqlQueryForSingleRow(fout, query->data);
+ if (use_functiondef)
+ {
+ funciargs = PQgetvalue(res, 0, PQfnumber(res, "funciargs"));
+ prokind = PQgetvalue(res, 0, PQfnumber(res, "prokind"));
+ }
+ else
+ {
proretset = PQgetvalue(res, 0, PQfnumber(res, "proretset"));
prosrc = PQgetvalue(res, 0, PQfnumber(res, "prosrc"));
probin = PQgetvalue(res, 0, PQfnumber(res, "probin"));
@@ -12022,8 +12045,13 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
nconfigitems = 0;
}
}
+ }
- if (funcargs)
+ if (use_functiondef)
+ {
+ funcsig = format_function_arguments(finfo, funciargs, false);
+ }
+ else if (funcargs)
{
/* 8.4 or later; we rely on server-side code for most of the work */
funcfullsig = format_function_arguments(finfo, funcargs, false);
@@ -12047,6 +12075,12 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
fmtId(finfo->dobj.namespace->dobj.name),
funcsig);
+ if (use_functiondef)
+ {
+ appendPQExpBuffer(q, "%s", PQgetvalue(res, 0, PQfnumber(res, "functiondef")));
+ }
+ else
+ {
appendPQExpBuffer(q, "CREATE %s %s.%s",
keyword,
fmtId(finfo->dobj.namespace->dobj.name),
@@ -12198,7 +12232,10 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
appendStringLiteralAH(q, pos, fout);
}
- appendPQExpBuffer(q, "\n %s;\n", asPart->data);
+ appendPQExpBuffer(q, "\n %s", asPart->data);
+ }
+
+ appendPQExpBuffer(q, ";\n");
append_depends_on_extension(fout, q, &finfo->dobj,
"pg_catalog.pg_proc", keyword,
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index ec63662060..c99408dc0d 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -1495,10 +1495,10 @@
RETURNS LANGUAGE_HANDLER AS \'$libdir/plpgsql\',
\'plpgsql_call_handler\' LANGUAGE C;',
regexp => qr/^
- \QCREATE FUNCTION dump_test.pltestlang_call_handler() \E
- \QRETURNS language_handler\E
+ \QCREATE FUNCTION dump_test.pltestlang_call_handler()\E
+ \n\s+\QRETURNS language_handler\E
\n\s+\QLANGUAGE c\E
- \n\s+AS\ \'\$
+ \n\s*AS\ \'\$
\Qlibdir\/plpgsql', 'plpgsql_call_handler';\E
/xm,
like =>
@@ -1512,9 +1512,10 @@
RETURNS trigger LANGUAGE plpgsql
AS $$ BEGIN RETURN NULL; END;$$;',
regexp => qr/^
- \QCREATE FUNCTION dump_test.trigger_func() RETURNS trigger\E
+ \QCREATE FUNCTION dump_test.trigger_func()\E
+ \n\s+\QRETURNS trigger\E
\n\s+\QLANGUAGE plpgsql\E
- \n\s+AS\ \$\$
+ \n\s*AS\ \$\$
\Q BEGIN RETURN NULL; END;\E
\$\$;/xm,
like =>
@@ -1528,9 +1529,10 @@
RETURNS event_trigger LANGUAGE plpgsql
AS $$ BEGIN RETURN; END;$$;',
regexp => qr/^
- \QCREATE FUNCTION dump_test.event_trigger_func() RETURNS event_trigger\E
+ \QCREATE FUNCTION dump_test.event_trigger_func()\E
+ \n\s+\QRETURNS event_trigger\E
\n\s+\QLANGUAGE plpgsql\E
- \n\s+AS\ \$\$
+ \n\s*AS\ \$\$
\Q BEGIN RETURN; END;\E
\$\$;/xm,
like =>
@@ -1838,9 +1840,11 @@
RETURNS dump_test.int42 AS \'int4in\'
LANGUAGE internal STRICT IMMUTABLE;',
regexp => qr/^
- \QCREATE FUNCTION dump_test.int42_in(cstring) RETURNS dump_test.int42\E
- \n\s+\QLANGUAGE internal IMMUTABLE STRICT\E
- \n\s+AS\ \$\$int4in\$\$;
+ \QCREATE FUNCTION dump_test.int42_in(cstring)\E
+ \n\s+\QRETURNS dump_test.int42\E
+ \n\s+\QLANGUAGE internal\E
+ \n\s+\QIMMUTABLE STRICT\E
+ \n\s*AS\ \$\$int4in\$\$;
/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
@@ -1853,9 +1857,11 @@
RETURNS cstring AS \'int4out\'
LANGUAGE internal STRICT IMMUTABLE;',
regexp => qr/^
- \QCREATE FUNCTION dump_test.int42_out(dump_test.int42) RETURNS cstring\E
- \n\s+\QLANGUAGE internal IMMUTABLE STRICT\E
- \n\s+AS\ \$\$int4out\$\$;
+ \QCREATE FUNCTION dump_test.int42_out(dump_test.int42)\E
+ \n\s+\QRETURNS cstring\E
+ \n\s+\QLANGUAGE internal\E
+ \n\s+\QIMMUTABLE STRICT\E
+ \n\s*AS\ \$\$int4out\$\$;
/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
@@ -1867,9 +1873,11 @@
create_sql =>
'CREATE FUNCTION dump_test.func_with_support() RETURNS int LANGUAGE sql AS $$ SELECT 1 $$ SUPPORT varchar_support;',
regexp => qr/^
- \QCREATE FUNCTION dump_test.func_with_support() RETURNS integer\E
- \n\s+\QLANGUAGE sql SUPPORT varchar_support\E
- \n\s+AS\ \$\$\Q SELECT 1 \E\$\$;
+ \QCREATE FUNCTION dump_test.func_with_support()\E
+ \n\s+\QRETURNS integer\E
+ \n\s+\QLANGUAGE sql\E
+ \n\s+\QSUPPORT varchar_support\E
+ \n\s*AS\ \$\$\Q SELECT 1 \E\$\$;
/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
@@ -1883,7 +1891,7 @@
regexp => qr/^
\QCREATE PROCEDURE dump_test.ptest1(a integer)\E
\n\s+\QLANGUAGE sql\E
- \n\s+AS\ \$\$\Q INSERT INTO dump_test.test_table (col1) VALUES (a) \E\$\$;
+ \n\s*AS\ \$\$\Q INSERT INTO dump_test.test_table (col1) VALUES (a) \E\$\$;
/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 082a11f270..fe4953114e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3660,7 +3660,7 @@
proargtypes => 'text text', prosrc => 'pg_get_serial_sequence' },
{ oid => '2098', descr => 'definition of a function',
proname => 'pg_get_functiondef', provolatile => 's', prorettype => 'text',
- proargtypes => 'oid', prosrc => 'pg_get_functiondef' },
+ proargtypes => 'oid bool bool', prosrc => 'pg_get_functiondef' },
{ oid => '2162', descr => 'argument list of a function',
proname => 'pg_get_function_arguments', provolatile => 's',
prorettype => 'text', proargtypes => 'oid',
diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl
index ae120a5ee3..760b7d06d5 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -490,7 +490,8 @@
'CREATE FUNCTION regress_pg_dump_schema.test_func' => {
regexp => qr/^
- \QCREATE FUNCTION regress_pg_dump_schema.test_func() RETURNS integer\E
+ \QCREATE FUNCTION regress_pg_dump_schema.test_func()\E
+ \n\s+\QRETURNS integer\E
\n\s+\QLANGUAGE sql\E
\n/xm,
like => { binary_upgrade => 1, },
--
2.28.0
On Wed, Aug 12, 2020 at 4:25 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
Here is a patch to have pg_dump use pg_get_functiondef() instead of
assembling the CREATE FUNCTION/PROCEDURE commands itself. This should
save on maintenance effort in the future. It's also a prerequisite for
being able to dump functions with SQL-standard function body discussed
in [0].pg_get_functiondef() was meant for psql's \ef command, so its defaults
are slightly different from what pg_dump would like, so this adds a few
optional parameters for tweaking the behavior. The naming of the
parameters is up for discussion.
One problem with this, which I think Tom pointed out before, is that
it might make it to handle some forward-compatibility problems. In
other words, if something that the server is generating needs to be
modified for compatibility with a future release, it's not easy to do
that. Like if we needed to quote something we weren't previously
quoting, for example.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2020-08-12 21:54, Robert Haas wrote:
One problem with this, which I think Tom pointed out before, is that
it might make it to handle some forward-compatibility problems. In
other words, if something that the server is generating needs to be
modified for compatibility with a future release, it's not easy to do
that. Like if we needed to quote something we weren't previously
quoting, for example.
We already use a lot of other pg_get_*def functions in pg_dump. Does
this one introduce any fundamentally new problems?
A hypothetical change where syntax that we accept now would no longer be
accepted in a (near-)future version would create a lot of upsetness. I
don't think we'd do it.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 2020-08-12 21:54, Robert Haas wrote:
One problem with this, which I think Tom pointed out before, is that
it might make it to handle some forward-compatibility problems. In
other words, if something that the server is generating needs to be
modified for compatibility with a future release, it's not easy to do
that. Like if we needed to quote something we weren't previously
quoting, for example.
We already use a lot of other pg_get_*def functions in pg_dump. Does
this one introduce any fundamentally new problems?
I wouldn't say that it's *fundamentally* new, but nonethless it disturbs
me that this proposal has pg_dump assembling CREATE FUNCTION commands in
very different ways depending on the server version. I'd rather see us
continuing to build the bulk of the command the same as before, and
introduce new behavior only for deparsing the function body.
We've talked before about what a mess it is that some aspects of pg_dump's
output are built on the basis of what pg_dump sees in its stable snapshot
but others are built by ruleutils.c on the basis of up-to-the-minute
catalog contents. While I don't insist that this patch fix that, I'm
worried that it may be making things worse, or at least getting in the
way of ever fixing that.
Perhaps these concerns are unfounded, but I'd like to see some arguments
why before we go down this path.
regards, tom lane
I wrote:
I wouldn't say that it's *fundamentally* new, but nonethless it disturbs
me that this proposal has pg_dump assembling CREATE FUNCTION commands in
very different ways depending on the server version. I'd rather see us
continuing to build the bulk of the command the same as before, and
introduce new behavior only for deparsing the function body.
BTW, a concrete argument for doing it that way is that if you make a
backend function that does the whole CREATE-FUNCTION-building job in
exactly the way pg_dump wants it, that function is nigh useless for
any other client with slightly different requirements. A trivial
example here is that I don't think we want to become locked into
the proposition that psql's \ef and \sf must print functions exactly
the same way that pg_dump would.
regards, tom lane
Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
I wrote:
I wouldn't say that it's *fundamentally* new, but nonethless it disturbs
me that this proposal has pg_dump assembling CREATE FUNCTION commands in
very different ways depending on the server version. I'd rather see us
continuing to build the bulk of the command the same as before, and
introduce new behavior only for deparsing the function body.BTW, a concrete argument for doing it that way is that if you make a
backend function that does the whole CREATE-FUNCTION-building job in
exactly the way pg_dump wants it, that function is nigh useless for
any other client with slightly different requirements. A trivial
example here is that I don't think we want to become locked into
the proposition that psql's \ef and \sf must print functions exactly
the same way that pg_dump would.
The fact that the need that psql has and that which pg_dump has are at
least somewhat similar really argues that we should have put this code
into libpgcommon in the first place and not in the backend, and then had
both psql and pg_dump use that.
I'm sure there's a lot of folks who'd like to see more of the logic we
have in pg_dump for building objects from the catalog available to more
tools through libpgcommon- psql being one of the absolute first
use-cases for exactly that (there's certainly no shortage of people
who've asked how they can get a CREATE TABLE statement for a table by
using psql...).
Thanks,
Stephen
I'm sure there's a lot of folks who'd like to see more of the logic we
have in pg_dump for building objects from the catalog available to more
tools through libpgcommon- psql being one of the absolute first
use-cases for exactly that (there's certainly no shortage of people
who've asked how they can get a CREATE TABLE statement for a table by
using psql...).
I count myself among those folks (see
/messages/by-id/CADkLM=fxfsrHASKk_bY_A4uomJ1Te5MfGgD_rwwQfV8wP68ewg@mail.gmail.com
for
discussion of doing DESCRIBE and SHOW CREATE-ish functions either on server
side or client side).
I'm all for having this as "just" as set of pg_get_*def functions, because
they allow for the results to be used in queries. Granted, the shape of the
result set may not be stable, but that's the sort of thing we can warn for
the same way we have warnings for changes to pg_stat_activity. At that
point any DESCRIBE/SHOW CREATE server side functions essentially become
just shells around the pg_get_*def(), with no particular requirement to
make those new commands work inside a SELECT.
Would it be totally out of left field to have the functions have an
optional "version" parameter, defaulted to null, that would be used to give
backwards compatible results if and when we do make a breaking change?
Greetings,
* Corey Huinker (corey.huinker@gmail.com) wrote:
I'm sure there's a lot of folks who'd like to see more of the logic we
have in pg_dump for building objects from the catalog available to more
tools through libpgcommon- psql being one of the absolute first
use-cases for exactly that (there's certainly no shortage of people
who've asked how they can get a CREATE TABLE statement for a table by
using psql...).I count myself among those folks (see
/messages/by-id/CADkLM=fxfsrHASKk_bY_A4uomJ1Te5MfGgD_rwwQfV8wP68ewg@mail.gmail.com
for
discussion of doing DESCRIBE and SHOW CREATE-ish functions either on server
side or client side).I'm all for having this as "just" as set of pg_get_*def functions, because
they allow for the results to be used in queries. Granted, the shape of the
result set may not be stable, but that's the sort of thing we can warn for
the same way we have warnings for changes to pg_stat_activity. At that
point any DESCRIBE/SHOW CREATE server side functions essentially become
just shells around the pg_get_*def(), with no particular requirement to
make those new commands work inside a SELECT.
Another advantage of having this in libpgcommon is that the backend
*and* the frontend could then use it.
Would it be totally out of left field to have the functions have an
optional "version" parameter, defaulted to null, that would be used to give
backwards compatible results if and when we do make a breaking change?
So.. the code that's in pg_dump today works to go from "whatever the
connected server's version is" to "whatever the version is of the
pg_dump command itself". If we had the code in libpgcommon, and
functions in the backend to get at it along with psql having that code,
you could then, using the code we have today, go from a bunch of
'source' versions to 'target' version of either the version of the psql
command, or that of the server.
That is, consider a future where this is all done and all that crazy
version-specific code in pg_dump has been moved to libpgcommon in v14,
and then you have a v15 psql, so:
psql v15 connected to PG v14:
You do: \dct mytable -- psql internal command to get 'create table'
Result: a CREATE TABLE that works for v15
You do: DESCRIBE mytable; -- PG backend function to get 'create table'
Result: a CREATE TABLE that works for v14
Without having to add anything to what we're already doing (yes, yes,
beyond the complications of moving this stuff into libpgcommon, but at
least we're not having to create some kind of matrix of "source PG
version 10, target PG version 12" into PG14).
A bit crazy, sure, but would certainly be pretty useful.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
So.. the code that's in pg_dump today works to go from "whatever the
connected server's version is" to "whatever the version is of the
pg_dump command itself". If we had the code in libpgcommon, and
functions in the backend to get at it along with psql having that code,
you could then, using the code we have today, go from a bunch of
'source' versions to 'target' version of either the version of the psql
command, or that of the server.
At this point, I think I need a high-power telescope even to see the
goalposts :-(
If we actually want to do something like this, we need a plan not just
some handwaving. Let's start by enumerating the concerns that would
have to be solved. I can think of:
* Execution context. Stephen seems to be envisioning code that could be
compiled into the backend not just the frontend, but is that really worth
the trouble? Could we share such code across FE/BE at all (it'd certainly
be a far more ambitious exercise in common code than we've done to date)?
What's the backend version actually doing, issuing queries over SPI?
(I suppose if you were rigid about that, it could offer a guarantee
that the results match your snapshot, which is pretty attractive.)
* Global vs. per-object activity. pg_dump likes to query the entire state
of the database to start with, and then follow up by grabbing additional
details about objects it's going to dump. That's not an operating mode
that most other clients would want, but if for no other reason than
performance, I don't think we can walk away from it for pg_dump ---
indeed, I think pg_dump probably needs to be fixed to do less per-object
querying, not more. Meanwhile applications such as psql \d would only
want to investigate one object at a time. What design can we create that
will handle that? If there is persistent state involved, what in the
world does that mean for the case of a backend-side library?
* Context in which the output is valid. Target server version was already
mentioned, but a quick examination of pg_dump output scripts will remind
you that there's a bunch more assumptions:
SET statement_timeout = 0;
SET lock_timeout = 0;
SET idle_in_transaction_session_timeout = 0;
SET client_encoding = 'en_US.utf8';
SET standard_conforming_strings = on;
SELECT pg_catalog.set_config('search_path', '', false);
SET check_function_bodies = false;
SET xmloption = content;
SET client_min_messages = warning;
SET row_security = off;
not to mention special hackery for object ownership and tablespaces.
Some of these things probably don't matter for other use-cases, but
others definitely do. In particular, I really doubt that psql and
other clients would find it acceptable to force search_path to a
particular thing. Which brings us to
* Security. How robust do the output commands need to be, and
what will we have to do that pg_dump doesn't need to?
* No doubt there are some other topics I didn't think of.
This certainly would be attractive if we had it, but the task
seems dauntingly large. It's not going to happen without some
fairly serious investment of time.
regards, tom lane
On 2020-08-15 16:36, Tom Lane wrote:
I wrote:
I wouldn't say that it's *fundamentally* new, but nonethless it disturbs
me that this proposal has pg_dump assembling CREATE FUNCTION commands in
very different ways depending on the server version. I'd rather see us
continuing to build the bulk of the command the same as before, and
introduce new behavior only for deparsing the function body.BTW, a concrete argument for doing it that way is that if you make a
backend function that does the whole CREATE-FUNCTION-building job in
exactly the way pg_dump wants it, that function is nigh useless for
any other client with slightly different requirements. A trivial
example here is that I don't think we want to become locked into
the proposition that psql's \ef and \sf must print functions exactly
the same way that pg_dump would.
That's why the patch adds optional arguments to the function to choose
the behavior that is appropriate for the situation.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-08-15 16:23, Tom Lane wrote:
I wouldn't say that it's*fundamentally* new, but nonethless it disturbs
me that this proposal has pg_dump assembling CREATE FUNCTION commands in
very different ways depending on the server version. I'd rather see us
continuing to build the bulk of the command the same as before, and
introduce new behavior only for deparsing the function body.
OK, I'll work on something like that.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services