Joshua Tolley <eggyk...@gmail.com> writes:
> I've just looked at pg_execute_from_file[1]. The idea here is to execute all
> the SQL commands in a given file. My comments:
Thanks for your review. Please find attached a revised patch where I've
changed the internals of the function so that it's split in two and that
the opr_sanity check passes, per comments from David Wheeler and Tom Lane.
> * I'd like to see the docs slightly expanded, specifically to describe
> parameter replacement. I wondered for a while if I needed to set of
> parameters in any specific way, before reading the code and realizing they
> can be whatever I want.
My guess is that you knew that in the CREATE EXTENSION context, it has
been proposed to use the notation @extschema@ as a placeholder, and
you've then been confused. I've refrained from imposing any style with
respect to what the placeholder would look like in the mecanism-patch.
Do we still want to detail in the docs that there's nothing expected
about the placeholder syntax of format?
> * Does anyone think it needs representation in the test suite?
Well the patch will get its tests with the arrival of the extension main
patch, where all contribs are installed using it.
> * Is it at all bad to include spi.h in genfile.c? IOW should this function
> live elsewhere? It seems reasonable to me to do it as written, but I thought
> I'd ask.
Well, using spi at this place has been asked by Álvaro and Tom, so my
guess is that's ok :)
> * In the snippet below, it seems best just to use palloc0():
> query_string = (char *)palloc((fsize+1)*sizeof(char));
> memset(query_string, 0, fsize+1);
Edited.
> * Shouldn't it include SPI_push() and SPI_pop()?
ENOCLUE
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 14461,14466 **** postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
--- 14461,14475 ----
<entry><type>record</type></entry>
<entry>Return information about a file</entry>
</row>
+ <row>
+ <entry>
+ <literal><function>pg_execute_from_file(<parameter>filename</> <type>text</>
+ [, <parameter>variable</parameter> <type>text</type>, <parameter>value</parameter> <type>text</type>
+ [, ...] ]) )</function></literal>
+ </entry>
+ <entry><type>void</type></entry>
+ <entry>Executes the <acronym>SQL</> commands contained in a file, replacing given placeholders.</entry>
+ </row>
</tbody>
</tgroup>
</table>
***************
*** 14499,14504 **** SELECT (pg_stat_file('filename')).modification;
--- 14508,14527 ----
</programlisting>
</para>
+ <indexterm>
+ <primary>pg_execute_from_file</primary>
+ </indexterm>
+ <para>
+ <function>pg_execute_from_file</> makes the server
+ execute <acronym>SQL</> commands to be found in a file. This function is
+ reserved to superusers.
+ </para>
+ <para>
+ The script might contain placeholders that will be replaced by the
+ values given in the <literal>VARIADIC</literal> arguments, which must be
+ a pair of variable names and values.
+ </para>
+
<para>
The functions shown in <xref linkend="functions-advisory-locks"> manage
advisory locks. For details about proper use of these functions, see
***************
*** 14521,14526 **** SELECT (pg_stat_file('filename')).modification;
--- 14544,14550 ----
<entry><type>void</type></entry>
<entry>Obtain exclusive advisory lock</entry>
</row>
+
<row>
<entry>
<literal><function>pg_advisory_lock(<parameter>key1</> <type>int</>, <parameter>key2</> <type>int</>)</function></literal>
*** a/src/backend/utils/adt/genfile.c
--- b/src/backend/utils/adt/genfile.c
***************
*** 7,12 ****
--- 7,13 ----
* Copyright (c) 2004-2010, PostgreSQL Global Development Group
*
* Author: Andreas Pflug <pgad...@pse-consulting.de>
+ * Dimitri Fontaine <dimi...@2ndquadrant.fr>
*
* IDENTIFICATION
* src/backend/utils/adt/genfile.c
***************
*** 21,31 ****
--- 22,34 ----
#include <dirent.h>
#include "catalog/pg_type.h"
+ #include "executor/spi.h"
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "postmaster/syslogger.h"
#include "storage/fd.h"
+ #include "utils/array.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
#include "utils/timestamp.h"
***************
*** 264,266 **** pg_ls_dir(PG_FUNCTION_ARGS)
--- 267,441 ----
SRF_RETURN_DONE(funcctx);
}
+
+ /*
+ * Support functions for pg_execute_from_file and its variant,
+ * pg_execute_from_file_with_placeholders.
+ */
+ static char *
+ read_query_string_from_file(const char *filename)
+ {
+ FILE *file;
+ int64 fsize = -1, nbytes;
+ struct stat fst;
+ char *query_string = NULL;
+
+ /*
+ * Only superuser can call pg_execute_from_file, and CREATE EXTENSION
+ * uses that too. Don't double check the PATH. Also note that
+ * extension's install files are not in $PGDATA but `pg_config
+ * --sharedir`.
+ */
+ if (stat(filename, &fst) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", filename)));
+
+ fsize = Int64GetDatum((int64) fst.st_size);
+
+ if ((file = AllocateFile(filename, PG_BINARY_R)) == NULL)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open file \"%s\" for reading: %m",
+ filename)));
+
+ if (ferror(file))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", filename)));
+
+ query_string = (char *)palloc0((fsize+1)*sizeof(char));
+ nbytes = fread(query_string, 1, (size_t) fsize, file);
+ pg_verifymbstr(query_string, nbytes, false);
+ FreeFile(file);
+
+ return query_string;
+ }
+
+ /*
+ * Given an array of repeated {variable, value}, replaces the placeholders
+ * by their values in the given query_string, by calling replace_text over
+ * each pair of arguments.
+ */
+ static char *
+ replace_placeholders_in_query_string(const char *query_string,
+ ArrayType *placeholders)
+ {
+ text *src = cstring_to_text(query_string);
+ Datum *replacements;
+ int nrep;
+ int i;
+ char *ret;
+
+ Assert(ARR_ELEMTYPE(placeholders) == TEXTOID);
+
+ deconstruct_array(placeholders, TEXTOID, -1, false, 'i',
+ &replacements, NULL, &nrep);
+
+ if (nrep % 2 != 0)
+ ereport(ERROR,
+ (errmsg("Expected pairs of variable names and values"),
+ errdetail("Please give an even number of replacement parameters")));
+
+ for (i = 0; i < nrep; i+=2)
+ {
+ Datum rep;
+
+ elog(DEBUG1,
+ "pg_execute_from_file replaces '%s' with '%s'",
+ text_to_cstring(DatumGetTextP(replacements[i])),
+ text_to_cstring(DatumGetTextP(replacements[i+1])));
+
+ rep = DirectFunctionCall3(replace_text,
+ PointerGetDatum(src),
+ replacements[i], replacements[i+1]);
+ src = DatumGetTextP(rep);
+ }
+ ret = text_to_cstring(src);
+ elog(DEBUG2, "pg_execute_from_file: %s", ret);
+
+ return ret;
+ }
+
+ /*
+ * The bulk of the execute from file functions, just call SPI to do the real
+ * work
+ */
+ static void
+ pg_execute_from_query_string(const char *filename, const char *query_string)
+ {
+ /*
+ * We abuse some internal knowledge from spi.h here. As we don't know
+ * which queries are going to get executed, we don't know what to expect
+ * as an OK return code from SPI_execute(). We assume that
+ * SPI_OK_CONNECT, SPI_OK_FINISH and SPI_OK_FETCH are quite improbable,
+ * though, and the errors are negatives. So a valid return code is
+ * considered to be SPI_OK_UTILITY or anything from there.
+ */
+ if (SPI_connect() != SPI_OK_CONNECT)
+ elog(ERROR, "SPI_connect failed");
+
+ if (SPI_execute(query_string, false, 0) < SPI_OK_UTILITY)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_EXCEPTION),
+ errmsg("File '%s' could not be executed", filename)));
+
+ if (SPI_finish() != SPI_OK_FINISH)
+ elog(ERROR, "SPI_finish failed");
+ }
+
+ /*
+ * Read a file then execute the SQL commands it contains.
+ */
+ Datum
+ pg_execute_from_file(PG_FUNCTION_ARGS)
+ {
+ text *filename_t = PG_GETARG_TEXT_P(0);
+ char *filename = text_to_cstring(filename_t);
+
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to execute file"))));
+
+ pg_execute_from_query_string(
+ filename,
+ read_query_string_from_file(filename));
+
+ PG_RETURN_VOID();
+ }
+
+ /*
+ * Variant accepting a VARIADIC text parameter containing placeholder
+ * variables and values, one after the other (so the variadic array length
+ * must be even).
+ *
+ * The main use case of the replacement facility is for setting the
+ * extension's schema, using @pg_extschema@ variable and a user given
+ * schema.
+ *
+ * This could be implemented in a single function together with the previous
+ * pg_execute_from_file, if only it was possible to fill in the
+ * proargdefaults pg_proc column from the backend code.
+ */
+ Datum
+ pg_execute_from_file_with_placeholders(PG_FUNCTION_ARGS)
+ {
+ text *filename_t = PG_GETARG_TEXT_P(0);
+ ArrayType *placeholders = PG_GETARG_ARRAYTYPE_P(1);
+ char *filename = text_to_cstring(filename_t);
+ char *query_string = NULL;
+
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to execute file"))));
+
+ pg_execute_from_query_string(
+ filename,
+ replace_placeholders_in_query_string(
+ read_query_string_from_file(filename),
+ placeholders));
+
+ PG_RETURN_VOID();
+ }
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***************
*** 3399,3412 **** DESCR("reload configuration files");
DATA(insert OID = 2622 ( pg_rotate_logfile PGNSP PGUID 12 1 0 0 f f f t f v 0 0 16 "" _null_ _null_ _null_ _null_ pg_rotate_logfile _null_ _null_ _null_ ));
DESCR("rotate log file");
! DATA(insert OID = 2623 ( pg_stat_file PGNSP PGUID 12 1 0 0 f f f t f v 1 0 2249 "25" "{25,20,1184,1184,1184,1184,16}" "{i,o,o,o,o,o,o}" "{filename,size,access,modification,change,creation,isdir}" _null_ pg_stat_file _null_ _null_ _null_ ));
DESCR("return file information");
! DATA(insert OID = 2624 ( pg_read_file PGNSP PGUID 12 1 0 0 f f f t f v 3 0 25 "25 20 20" _null_ _null_ _null_ _null_ pg_read_file _null_ _null_ _null_ ));
DESCR("read text from a file");
! DATA(insert OID = 2625 ( pg_ls_dir PGNSP PGUID 12 1 1000 0 f f f t t v 1 0 25 "25" _null_ _null_ _null_ _null_ pg_ls_dir _null_ _null_ _null_ ));
DESCR("list all files in a directory");
! DATA(insert OID = 2626 ( pg_sleep PGNSP PGUID 12 1 0 0 f f f t f v 1 0 2278 "701" _null_ _null_ _null_ _null_ pg_sleep _null_ _null_ _null_ ));
DESCR("sleep for the specified time in seconds");
DATA(insert OID = 2971 ( text PGNSP PGUID 12 1 0 0 f f f t f i 1 0 25 "16" _null_ _null_ _null_ _null_ booltext _null_ _null_ _null_ ));
DESCR("convert boolean to text");
--- 3399,3416 ----
DATA(insert OID = 2622 ( pg_rotate_logfile PGNSP PGUID 12 1 0 0 f f f t f v 0 0 16 "" _null_ _null_ _null_ _null_ pg_rotate_logfile _null_ _null_ _null_ ));
DESCR("rotate log file");
! DATA(insert OID = 2623 ( pg_stat_file PGNSP PGUID 12 1 0 0 f f f t f v 1 0 2249 "25" "{25,20,1184,1184,1184,1184,16}" "{i,o,o,o,o,o,o}" "{filename,size,access,modification,change,creation,isdir}" _null_ pg_stat_file _null_ _null_ _null_ ));
DESCR("return file information");
! DATA(insert OID = 2624 ( pg_read_file PGNSP PGUID 12 1 0 0 f f f t f v 3 0 25 "25 20 20" _null_ _null_ _null_ _null_ pg_read_file _null_ _null_ _null_ ));
DESCR("read text from a file");
! DATA(insert OID = 2625 ( pg_ls_dir PGNSP PGUID 12 1 1000 0 f f f t t v 1 0 25 "25" _null_ _null_ _null_ _null_ pg_ls_dir _null_ _null_ _null_ ));
DESCR("list all files in a directory");
! DATA(insert OID = 2626 ( pg_sleep PGNSP PGUID 12 1 0 0 f f f t f v 1 0 2278 "701" _null_ _null_ _null_ _null_ pg_sleep _null_ _null_ _null_ ));
DESCR("sleep for the specified time in seconds");
+ DATA(insert OID = 3627 ( pg_execute_from_file PGNSP PGUID 12 1 0 0 f f f t f v 1 0 2278 "25" _null_ _null_ _null_ _null_ pg_execute_from_file _null_ _null_ _null_ ));
+ DESCR("execute queries read from a file");
+ DATA(insert OID = 3928 ( pg_execute_from_file PGNSP PGUID 12 1 0 25 f f f t f v 2 0 2278 "25 25" "{25,25}" "{i,v}" _null_ _null_ pg_execute_from_file_with_placeholders _null_ _null_ _null_ ));
+ DESCR("execute queries read from a file");
DATA(insert OID = 2971 ( text PGNSP PGUID 12 1 0 0 f f f t f i 1 0 25 "16" _null_ _null_ _null_ _null_ booltext _null_ _null_ _null_ ));
DESCR("convert boolean to text");
*** a/src/include/utils/builtins.h
--- b/src/include/utils/builtins.h
***************
*** 443,448 **** extern Datum pg_relation_filepath(PG_FUNCTION_ARGS);
--- 443,450 ----
extern Datum pg_stat_file(PG_FUNCTION_ARGS);
extern Datum pg_read_file(PG_FUNCTION_ARGS);
extern Datum pg_ls_dir(PG_FUNCTION_ARGS);
+ extern Datum pg_execute_from_file(PG_FUNCTION_ARGS);
+ extern Datum pg_execute_from_file_with_placeholders (PG_FUNCTION_ARGS);
/* misc.c */
extern Datum current_database(PG_FUNCTION_ARGS);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
