Allow COPY to use parameters
Folks,
Per discussion on IRC and some test code, COPY can't take parameters
in a PREPARE, which feature would make it even more useful. To make
this work, we'd need to:
- do parse analysis immediately
- parameterize all the options
This doesn't seem like a gigantic lift. What say?
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter <david@fetter.org> writes:
Per discussion on IRC and some test code, COPY can't take parameters
in a PREPARE, which feature would make it even more useful.
Uh, what?
regression=# prepare foo as copy c from stdin;
ERROR: syntax error at or near "copy"
LINE 1: prepare foo as copy c from stdin;
^
Passing parameters into a utility statement of any stripe is a pretty
considerable project, IIRC; the infrastructure isn't there.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, May 24, 2016 at 02:16:40PM -0400, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
Per discussion on IRC and some test code, COPY can't take parameters
in a PREPARE, which feature would make it even more useful.Uh, what?
regression=# prepare foo as copy c from stdin;
ERROR: syntax error at or near "copy"
LINE 1: prepare foo as copy c from stdin;
^Passing parameters into a utility statement of any stripe is a
pretty considerable project, IIRC; the infrastructure isn't there.
Maybe it should be, at least for some of the utility statements.
Please find attached a patch which, according to Andrew Gierth, its
author, just barely qualifies as a PoC. Yes, it's had to break a
couple of messages in the regression tests.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
parameterize_copy_001.difftext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3201476..66ae54e 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -279,12 +279,12 @@ static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
/* non-export function prototypes */
-static CopyState BeginCopy(bool is_from, Relation rel, Node *raw_query,
+static CopyState BeginCopy(bool is_from, Relation rel, Node *raw_query, ParamListInfo params,
const char *queryString, const Oid queryRelId, List *attnamelist,
List *options);
static void EndCopy(CopyState cstate);
static void ClosePipeToProgram(CopyState cstate);
-static CopyState BeginCopyTo(Relation rel, Node *query, const char *queryString,
+static CopyState BeginCopyTo(Relation rel, Node *query, ParamListInfo params, const char *queryString,
const Oid queryRelId, const char *filename, bool is_program,
List *attnamelist, List *options);
static void EndCopyTo(CopyState cstate);
@@ -787,7 +787,7 @@ CopyLoadRawBuf(CopyState cstate)
* the table or the specifically requested columns.
*/
Oid
-DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
+DoCopy(const CopyStmt *stmt, const char *queryString, ParamListInfo params, uint64 *processed)
{
CopyState cstate;
bool is_from = stmt->is_from;
@@ -944,7 +944,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
}
else
{
- cstate = BeginCopyTo(rel, query, queryString, relid,
+ cstate = BeginCopyTo(rel, query, params, queryString, relid,
stmt->filename, stmt->is_program,
stmt->attlist, stmt->options);
*processed = DoCopyTo(cstate); /* copy from database to file */
@@ -1321,6 +1321,7 @@ static CopyState
BeginCopy(bool is_from,
Relation rel,
Node *raw_query,
+ ParamListInfo params,
const char *queryString,
const Oid queryRelId,
List *attnamelist,
@@ -1391,11 +1392,16 @@ BeginCopy(bool is_from,
* function and is executed repeatedly. (See also the same hack in
* DECLARE CURSOR and PREPARE.) XXX FIXME someday.
*/
- rewritten = pg_analyze_and_rewrite((Node *) copyObject(raw_query),
- queryString, NULL, 0);
+ if (!IsA(raw_query,List))
+ {
+ rewritten = pg_analyze_and_rewrite((Node *) copyObject(raw_query),
+ queryString, NULL, 0);
+ }
+ else
+ rewritten = (List *) raw_query;
/* check that we got back something we can work with */
- if (rewritten == NIL)
+ if (rewritten == NIL || linitial(rewritten) == NIL)
{
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1453,7 +1459,7 @@ BeginCopy(bool is_from,
}
/* plan the query */
- plan = pg_plan_query(query, 0, NULL);
+ plan = pg_plan_query(query, 0, params);
/*
* With row level security and a user using "COPY relation TO", we
@@ -1495,7 +1501,7 @@ BeginCopy(bool is_from,
cstate->queryDesc = CreateQueryDesc(plan, queryString,
GetActiveSnapshot(),
InvalidSnapshot,
- dest, NULL, 0);
+ dest, params, 0);
/*
* Call ExecutorStart to prepare the plan for execution.
@@ -1682,6 +1688,7 @@ EndCopy(CopyState cstate)
static CopyState
BeginCopyTo(Relation rel,
Node *query,
+ ParamListInfo params,
const char *queryString,
const Oid queryRelId,
const char *filename,
@@ -1725,7 +1732,7 @@ BeginCopyTo(Relation rel,
RelationGetRelationName(rel))));
}
- cstate = BeginCopy(false, rel, query, queryString, queryRelId, attnamelist,
+ cstate = BeginCopy(false, rel, query, params, queryString, queryRelId, attnamelist,
options);
oldcontext = MemoryContextSwitchTo(cstate->copycontext);
@@ -2670,7 +2677,7 @@ BeginCopyFrom(Relation rel,
MemoryContext oldcontext;
bool volatile_defexprs;
- cstate = BeginCopy(true, rel, NULL, NULL, InvalidOid, attnamelist, options);
+ cstate = BeginCopy(true, rel, NULL, NULL, NULL, InvalidOid, attnamelist, options);
oldcontext = MemoryContextSwitchTo(cstate->copycontext);
/* Initialize state variables */
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 29c8c4e..a6d9dd6 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -299,6 +299,13 @@ transformStmt(ParseState *pstate, Node *parseTree)
result = makeNode(Query);
result->commandType = CMD_UTILITY;
result->utilityStmt = (Node *) parseTree;
+
+ if (IsA(parseTree,CopyStmt)
+ && ((CopyStmt *)parseTree)->query != NIL)
+ {
+ CopyStmt *stmt = (CopyStmt *) parseTree;
+ stmt->query = transformStmt(pstate, stmt->query);
+ }
break;
}
@@ -343,6 +350,7 @@ analyze_requires_snapshot(Node *parseTree)
case T_ExplainStmt:
case T_CreateTableAsStmt:
+ case T_CopyStmt:
/* yes, because we must analyze the contained statement */
result = true;
break;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 68811f1..4a18fec 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -739,6 +739,16 @@ pg_rewrite_query(Query *query)
if (query->commandType == CMD_UTILITY)
{
+ if (query->utilityStmt
+ && IsA(query->utilityStmt,CopyStmt)
+ && ((CopyStmt *)(query->utilityStmt))->query)
+ {
+ CopyStmt *stmt = (CopyStmt *)(query->utilityStmt);
+ Assert(IsA(stmt->query,Query));
+ stmt->query = (Node *) QueryRewrite((Query *)(stmt->query));
+ if (stmt->query == (Node *) NIL)
+ stmt->query = (Node *) list_make1(NIL);
+ }
/* don't rewrite utilities, just dump 'em into result list */
querytree_list = list_make1(query);
}
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index ac50c2a..a7deead 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -540,7 +540,7 @@ standard_ProcessUtility(Node *parsetree,
{
uint64 processed;
- DoCopy((CopyStmt *) parsetree, queryString, &processed);
+ DoCopy((CopyStmt *) parsetree, queryString, params, &processed);
if (completionTag)
snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
"COPY " UINT64_FORMAT, processed);
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 314d1f7..44c2c66 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -21,7 +21,7 @@
/* CopyStateData is private in commands/copy.c */
typedef struct CopyStateData *CopyState;
-extern Oid DoCopy(const CopyStmt *stmt, const char *queryString,
+extern Oid DoCopy(const CopyStmt *stmt, const char *queryString, ParamListInfo params,
uint64 *processed);
extern void ProcessCopyOptions(CopyState cstate, bool is_from, List *options);
On Tue, May 24, 2016 at 3:42 PM, David Fetter <david@fetter.org> wrote:
On Tue, May 24, 2016 at 02:16:40PM -0400, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
Per discussion on IRC and some test code, COPY can't take parameters
in a PREPARE, which feature would make it even more useful.Uh, what?
regression=# prepare foo as copy c from stdin;
ERROR: syntax error at or near "copy"
LINE 1: prepare foo as copy c from stdin;
^Passing parameters into a utility statement of any stripe is a
pretty considerable project, IIRC; the infrastructure isn't there.Maybe it should be, at least for some of the utility statements.
Please find attached a patch which, according to Andrew Gierth, its
author, just barely qualifies as a PoC. Yes, it's had to break a
couple of messages in the regression tests.
Hm, what's the use case preparing COPY? Note, the biggest pain point
I have with COPY is not being able to parameterize the filename
argument. That's pretty easily worked around with a plpgsql wrapper
however.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Merlin Moncure <mmoncure@gmail.com> writes:
Hm, what's the use case preparing COPY? Note, the biggest pain point
I have with COPY is not being able to parameterize the filename
argument.
Yeah. But putting a parameter symbol there (or anywhere in a utility
statement that's not part of an analyzable subexpression) introduces a
bunch of new issues. We don't really have a notion of a specific data
type associated with most arguments of utility statements, which
complicates parse analysis (specifically, resolution of the data type to
be attributed to the parameter symbol). And we don't have provision for
applying expression evaluation to such arguments, which is what you'd
expect to need to do to obtain the value of a parameter symbol. In many
cases you absolutely don't want expression evaluation to happen in
utility statements, because it would complicate semantics significantly.
Bottom line is that there's a pretty large can of worms hiding under
this, and I am not eager to open it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"Merlin" == Merlin Moncure <mmoncure@gmail.com> writes:
Merlin> Hm, what's the use case preparing COPY?
Preparing it isn't necessarily the point (and SQL-level PREPARE is not
addressed in that patch). The point is to allow parameters, so that a
client can do
COPY (select blah from whatever where thing = $1) TO ...
without having to mess around with interpolations.
Merlin> Note, the biggest pain point I have with COPY is not being able
Merlin> to parameterize the filename argument.
Certainly that can be handled too.
--
Andrew (irc:RhodiumToad)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"Merlin" == Merlin Moncure <mmoncure@gmail.com> writes:
Merlin> Note, the biggest pain point I have with COPY is not being able
Merlin> to parameterize the filename argument.
Second proof of concept attached. This goes so far as to allow
statements like:
do $$
declare t text := 'bar'; f text := '/tmp/copytest.dat';
begin copy (select t, now()) to (f) csv header; end;
$$;
Also "copy foo to $1" or "copy (select * from foo where x=$1) to $2" and
so on should work from PQexecParams or in a plpgsql EXECUTE.
(I haven't tried to parameterize anything other than the filename and
query. Also, it does not accept arbitrary expressions - only $n, '...'
or a columnref. $n and '...' can have parens or not, but the columnref
must have them due to conflicts with unreserved keywords PROGRAM, STDIN,
STDOUT. This could be hacked around in other ways, I guess, if the
parens are too ugly.)
--
Andrew (irc:RhodiumToad)
Attachments:
copyparam2.patchtext/x-patchDownload
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3201476..97debb7 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -37,6 +37,8 @@
#include "optimizer/clauses.h"
#include "optimizer/planner.h"
#include "nodes/makefuncs.h"
+#include "nodes/nodeFuncs.h"
+#include "parser/analyze.h"
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
#include "tcop/tcopprot.h"
@@ -279,13 +281,13 @@ static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
/* non-export function prototypes */
-static CopyState BeginCopy(bool is_from, Relation rel, Node *raw_query,
+static CopyState BeginCopy(bool is_from, Relation rel, Node *raw_query, ParamListInfo params,
const char *queryString, const Oid queryRelId, List *attnamelist,
List *options);
static void EndCopy(CopyState cstate);
static void ClosePipeToProgram(CopyState cstate);
-static CopyState BeginCopyTo(Relation rel, Node *query, const char *queryString,
- const Oid queryRelId, const char *filename, bool is_program,
+static CopyState BeginCopyTo(Relation rel, Node *query, ParamListInfo params, const char *queryString,
+ const Oid queryRelId, Node *filename_expr, bool is_program,
List *attnamelist, List *options);
static void EndCopyTo(CopyState cstate);
static uint64 DoCopyTo(CopyState cstate);
@@ -767,6 +769,43 @@ CopyLoadRawBuf(CopyState cstate)
}
+static char *
+CopyEvalFilename(QueryDesc *qd, Node *expr, ParamListInfo params)
+{
+ char *filename = NULL;
+
+ if (expr)
+ {
+ EState *estate = qd ? qd->estate : CreateExecutorState();
+ MemoryContext oldcontext = MemoryContextSwitchTo(estate->es_query_cxt);
+
+ Assert(exprType(expr) == CSTRINGOID);
+
+ if (qd == NULL)
+ estate->es_param_list_info = params;
+
+ {
+ ExprContext *ecxt = CreateExprContext(estate);
+ ExprState *exprstate = ExecPrepareExpr(copyObject(expr), estate);
+ bool isnull;
+ Datum result = ExecEvalExprSwitchContext(exprstate, ecxt, &isnull, NULL);
+ if (!isnull)
+ filename = MemoryContextStrdup(oldcontext, DatumGetCString(result));
+ FreeExprContext(ecxt, true);
+ }
+
+ MemoryContextSwitchTo(oldcontext);
+
+ if (qd == NULL)
+ FreeExecutorState(estate);
+
+ if (!filename)
+ elog(ERROR, "COPY filename expression must not return NULL");
+ }
+
+ return filename;
+}
+
/*
* DoCopy executes the SQL COPY statement
*
@@ -787,7 +826,7 @@ CopyLoadRawBuf(CopyState cstate)
* the table or the specifically requested columns.
*/
Oid
-DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
+DoCopy(const CopyStmt *stmt, const char *queryString, ParamListInfo params, uint64 *processed)
{
CopyState cstate;
bool is_from = stmt->is_from;
@@ -906,7 +945,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
select->targetList = list_make1(target);
select->fromClause = list_make1(from);
- query = (Node *) select;
+ query = (Node *) parse_analyze((Node *) select, queryString, NULL, 0);
/*
* Close the relation for now, but keep the lock on it to prevent
@@ -929,6 +968,8 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
if (is_from)
{
+ char *filename;
+
Assert(rel);
/* check read-only transaction and parallel mode */
@@ -936,15 +977,20 @@ DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *processed)
PreventCommandIfReadOnly("COPY FROM");
PreventCommandIfParallelMode("COPY FROM");
- cstate = BeginCopyFrom(rel, stmt->filename, stmt->is_program,
+ filename = CopyEvalFilename(NULL, stmt->filename, params);
+
+ cstate = BeginCopyFrom(rel, filename, stmt->is_program,
stmt->attlist, stmt->options);
cstate->range_table = range_table;
*processed = CopyFrom(cstate); /* copy from file to database */
EndCopyFrom(cstate);
+
+ if (filename)
+ pfree(filename);
}
else
{
- cstate = BeginCopyTo(rel, query, queryString, relid,
+ cstate = BeginCopyTo(rel, query, params, queryString, relid,
stmt->filename, stmt->is_program,
stmt->attlist, stmt->options);
*processed = DoCopyTo(cstate); /* copy from database to file */
@@ -1321,6 +1367,7 @@ static CopyState
BeginCopy(bool is_from,
Relation rel,
Node *raw_query,
+ ParamListInfo params,
const char *queryString,
const Oid queryRelId,
List *attnamelist,
@@ -1391,8 +1438,7 @@ BeginCopy(bool is_from,
* function and is executed repeatedly. (See also the same hack in
* DECLARE CURSOR and PREPARE.) XXX FIXME someday.
*/
- rewritten = pg_analyze_and_rewrite((Node *) copyObject(raw_query),
- queryString, NULL, 0);
+ rewritten = QueryRewrite(copyObject(raw_query));
/* check that we got back something we can work with */
if (rewritten == NIL)
@@ -1453,7 +1499,7 @@ BeginCopy(bool is_from,
}
/* plan the query */
- plan = pg_plan_query(query, 0, NULL);
+ plan = pg_plan_query(query, 0, params);
/*
* With row level security and a user using "COPY relation TO", we
@@ -1495,7 +1541,7 @@ BeginCopy(bool is_from,
cstate->queryDesc = CreateQueryDesc(plan, queryString,
GetActiveSnapshot(),
InvalidSnapshot,
- dest, NULL, 0);
+ dest, params, 0);
/*
* Call ExecutorStart to prepare the plan for execution.
@@ -1682,15 +1728,16 @@ EndCopy(CopyState cstate)
static CopyState
BeginCopyTo(Relation rel,
Node *query,
+ ParamListInfo params,
const char *queryString,
const Oid queryRelId,
- const char *filename,
+ Node *filename_expr,
bool is_program,
List *attnamelist,
List *options)
{
CopyState cstate;
- bool pipe = (filename == NULL);
+ bool pipe = (filename_expr == NULL);
MemoryContext oldcontext;
if (rel != NULL && rel->rd_rel->relkind != RELKIND_RELATION)
@@ -1725,7 +1772,7 @@ BeginCopyTo(Relation rel,
RelationGetRelationName(rel))));
}
- cstate = BeginCopy(false, rel, query, queryString, queryRelId, attnamelist,
+ cstate = BeginCopy(false, rel, query, params, queryString, queryRelId, attnamelist,
options);
oldcontext = MemoryContextSwitchTo(cstate->copycontext);
@@ -1737,7 +1784,7 @@ BeginCopyTo(Relation rel,
}
else
{
- cstate->filename = pstrdup(filename);
+ cstate->filename = CopyEvalFilename(cstate->queryDesc, filename_expr, params);
cstate->is_program = is_program;
if (is_program)
@@ -1758,7 +1805,7 @@ BeginCopyTo(Relation rel,
* Prevent write to relative path ... too easy to shoot oneself in
* the foot by overwriting a database file ...
*/
- if (!is_absolute_path(filename))
+ if (!is_absolute_path(cstate->filename))
ereport(ERROR,
(errcode(ERRCODE_INVALID_NAME),
errmsg("relative path not allowed for COPY to file")));
@@ -2670,7 +2717,7 @@ BeginCopyFrom(Relation rel,
MemoryContext oldcontext;
bool volatile_defexprs;
- cstate = BeginCopy(true, rel, NULL, NULL, InvalidOid, attnamelist, options);
+ cstate = BeginCopy(true, rel, NULL, NULL, NULL, InvalidOid, attnamelist, options);
oldcontext = MemoryContextSwitchTo(cstate->copycontext);
/* Initialize state variables */
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 20e38f0..f78404e 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2983,7 +2983,7 @@ _copyCopyStmt(const CopyStmt *from)
COPY_NODE_FIELD(attlist);
COPY_SCALAR_FIELD(is_from);
COPY_SCALAR_FIELD(is_program);
- COPY_STRING_FIELD(filename);
+ COPY_NODE_FIELD(filename);
COPY_NODE_FIELD(options);
return newnode;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index c5ccc42..4af21cb 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1144,7 +1144,7 @@ _equalCopyStmt(const CopyStmt *a, const CopyStmt *b)
COMPARE_NODE_FIELD(attlist);
COMPARE_SCALAR_FIELD(is_from);
COMPARE_SCALAR_FIELD(is_program);
- COMPARE_STRING_FIELD(filename);
+ COMPARE_NODE_FIELD(filename);
COMPARE_NODE_FIELD(options);
return true;
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 29c8c4e..f0a3f60 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -36,12 +36,14 @@
#include "parser/parse_coerce.h"
#include "parser/parse_collate.h"
#include "parser/parse_cte.h"
+#include "parser/parse_expr.h"
#include "parser/parse_oper.h"
#include "parser/parse_param.h"
#include "parser/parse_relation.h"
#include "parser/parse_target.h"
#include "parser/parsetree.h"
#include "rewrite/rewriteManip.h"
+#include "utils/builtins.h"
#include "utils/rel.h"
@@ -74,6 +76,7 @@ static Query *transformCreateTableAsStmt(ParseState *pstate,
CreateTableAsStmt *stmt);
static void transformLockingClause(ParseState *pstate, Query *qry,
LockingClause *lc, bool pushedDown);
+static Query *transformCopyStmt(ParseState *pstate, CopyStmt *stmt);
#ifdef RAW_EXPRESSION_COVERAGE_TEST
static bool test_raw_expression_coverage(Node *node, void *context);
#endif
@@ -290,6 +293,11 @@ transformStmt(ParseState *pstate, Node *parseTree)
(CreateTableAsStmt *) parseTree);
break;
+ case T_CopyStmt:
+ result = transformCopyStmt(pstate,
+ (CopyStmt *) parseTree);
+ break;
+
default:
/*
@@ -347,6 +355,11 @@ analyze_requires_snapshot(Node *parseTree)
result = true;
break;
+ case T_CopyStmt:
+ /* maybe, because we might have a contained statement */
+ result = ((CopyStmt *)parseTree)->query != NULL;
+ break;
+
default:
/* other utility statements don't have any real parse analysis */
result = false;
@@ -356,6 +369,40 @@ analyze_requires_snapshot(Node *parseTree)
return result;
}
+static Query *
+transformCopyStmt(ParseState *pstate, CopyStmt *stmt)
+{
+ Query *result = makeNode(Query);
+
+ result->commandType = CMD_UTILITY;
+ result->utilityStmt = (Node *) stmt;
+
+ if (stmt->filename)
+ {
+ Node *expr1 = transformExpr(pstate, stmt->filename, EXPR_KIND_OTHER);
+ Node *expr2 = coerce_to_target_type(pstate, expr1, exprType(expr1),
+ CSTRINGOID, -1,
+ COERCION_EXPLICIT,
+ COERCE_IMPLICIT_CAST,
+ exprLocation(expr1));
+
+ if (!expr2)
+ ereport(ERROR,
+ (errcode(ERRCODE_CANNOT_COERCE),
+ errmsg("cannot cast type %s to %s",
+ format_type_be(exprType(expr1)),
+ format_type_be(CSTRINGOID)),
+ parser_errposition(pstate, exprLocation(expr1))));
+
+ stmt->filename = expr2;
+ }
+
+ if (stmt->query != NULL)
+ stmt->query = (Node *) transformStmt(pstate, stmt->query);
+
+ return result;
+}
+
/*
* transformDeleteStmt -
* transforms a Delete Statement
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 18ec5f0..6854255 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -313,8 +313,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <defelt> event_trigger_when_item
%type <chr> enable_trigger
-%type <str> copy_file_name
- database_name access_method_clause access_method attr_name
+%type <node> copy_file_name param_opt_indirection
+%type <str> database_name access_method_clause access_method attr_name
name cursor_name file_name
index_name opt_index_name cluster_index_specification
@@ -2651,7 +2651,11 @@ opt_program:
* stdout. We silently correct the "typo".) - AY 9/94
*/
copy_file_name:
- Sconst { $$ = $1; }
+ Sconst { $$ = makeStringConst($1,@1); }
+ | '(' Sconst ')' { $$ = makeStringConst($2,@2); }
+ | param_opt_indirection { $$ = $1; }
+ | '(' columnref ')' { $$ = $2; }
+ | '(' param_opt_indirection ')' { $$ = $2; }
| STDIN { $$ = NULL; }
| STDOUT { $$ = NULL; }
;
@@ -12049,21 +12053,7 @@ b_expr: c_expr
*/
c_expr: columnref { $$ = $1; }
| AexprConst { $$ = $1; }
- | PARAM opt_indirection
- {
- ParamRef *p = makeNode(ParamRef);
- p->number = $1;
- p->location = @1;
- if ($2)
- {
- A_Indirection *n = makeNode(A_Indirection);
- n->arg = (Node *) p;
- n->indirection = check_indirection($2, yyscanner);
- $$ = (Node *) n;
- }
- else
- $$ = (Node *) p;
- }
+ | param_opt_indirection { $$ = $1; }
| '(' a_expr ')' opt_indirection
{
if ($4)
@@ -12192,6 +12182,23 @@ c_expr: columnref { $$ = $1; }
}
;
+param_opt_indirection: PARAM opt_indirection
+ {
+ ParamRef *p = makeNode(ParamRef);
+ p->number = $1;
+ p->location = @1;
+ if ($2)
+ {
+ A_Indirection *n = makeNode(A_Indirection);
+ n->arg = (Node *) p;
+ n->indirection = check_indirection($2, yyscanner);
+ $$ = (Node *) n;
+ }
+ else
+ $$ = (Node *) p;
+ }
+ ;
+
func_application: func_name '(' ')'
{
$$ = (Node *) makeFuncCall($1, NIL, @1);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index ac50c2a..dba2f5e 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -540,7 +540,7 @@ standard_ProcessUtility(Node *parsetree,
{
uint64 processed;
- DoCopy((CopyStmt *) parsetree, queryString, &processed);
+ DoCopy((CopyStmt *) parsetree, queryString, params, &processed);
if (completionTag)
snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
"COPY " UINT64_FORMAT, processed);
@@ -1769,6 +1769,16 @@ UtilityContainsQuery(Node *parsetree)
return UtilityContainsQuery(qry->utilityStmt);
return qry;
+ case T_CopyStmt:
+ qry = (Query *) ((CopyStmt *) parsetree)->query;
+ if (qry)
+ {
+ Assert(IsA(qry, Query));
+ if (qry->commandType == CMD_UTILITY)
+ return UtilityContainsQuery(qry->utilityStmt);
+ }
+ return qry;
+
case T_CreateTableAsStmt:
qry = (Query *) ((CreateTableAsStmt *) parsetree)->query;
Assert(IsA(qry, Query));
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 314d1f7..44c2c66 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -21,7 +21,7 @@
/* CopyStateData is private in commands/copy.c */
typedef struct CopyStateData *CopyState;
-extern Oid DoCopy(const CopyStmt *stmt, const char *queryString,
+extern Oid DoCopy(const CopyStmt *stmt, const char *queryString, ParamListInfo params,
uint64 *processed);
extern void ProcessCopyOptions(CopyState cstate, bool is_from, List *options);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 714cf15..049ac4a 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1692,7 +1692,7 @@ typedef struct CopyStmt
* for all columns */
bool is_from; /* TO or FROM */
bool is_program; /* is 'filename' a program to popen? */
- char *filename; /* filename, or NULL for STDIN/STDOUT */
+ Node *filename; /* filename, or NULL for STDIN/STDOUT */
List *options; /* List of DefElem nodes */
} CopyStmt;
diff --git a/src/test/regress/expected/copyselect.out b/src/test/regress/expected/copyselect.out
index 72865fe..a02a199 100644
--- a/src/test/regress/expected/copyselect.out
+++ b/src/test/regress/expected/copyselect.out
@@ -44,7 +44,9 @@ c
-- This should fail
--
copy (select t into temp test3 from test1 where id=3) to stdout;
-ERROR: COPY (SELECT INTO) is not supported
+ERROR: SELECT ... INTO is not allowed here
+LINE 1: copy (select t into temp test3 from test1 where id=3) to std...
+ ^
--
-- This should fail
--
diff --git a/src/test/regress/expected/select_into.out b/src/test/regress/expected/select_into.out
index b577d1b..6b6482e 100644
--- a/src/test/regress/expected/select_into.out
+++ b/src/test/regress/expected/select_into.out
@@ -90,7 +90,9 @@ ERROR: SELECT ... INTO is not allowed here
LINE 1: DECLARE foo CURSOR FOR SELECT 1 INTO b;
^
COPY (SELECT 1 INTO frak UNION SELECT 2) TO 'blob';
-ERROR: COPY (SELECT INTO) is not supported
+ERROR: SELECT ... INTO is not allowed here
+LINE 1: COPY (SELECT 1 INTO frak UNION SELECT 2) TO 'blob';
+ ^
SELECT * FROM (SELECT 1 INTO f) bar;
ERROR: SELECT ... INTO is not allowed here
LINE 1: SELECT * FROM (SELECT 1 INTO f) bar;
On 27 May 2016 at 15:17, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote:
"Merlin" == Merlin Moncure <mmoncure@gmail.com> writes:
Merlin> Note, the biggest pain point I have with COPY is not being able
Merlin> to parameterize the filename argument.Second proof of concept attached. This goes so far as to allow
statements like:do $$
declare t text := 'bar'; f text := '/tmp/copytest.dat';
begin copy (select t, now()) to (f) csv header; end;
$$;Also "copy foo to $1" or "copy (select * from foo where x=$1) to $2" and
so on should work from PQexecParams or in a plpgsql EXECUTE.(I haven't tried to parameterize anything other than the filename and
query. Also, it does not accept arbitrary expressions - only $n, '...'
or a columnref. $n and '...' can have parens or not, but the columnref
must have them due to conflicts with unreserved keywords PROGRAM, STDIN,
STDOUT. This could be hacked around in other ways, I guess, if the
parens are too ugly.)
In addition to it being generally nice to be able to send parameters to
COPY (SELECT ...), I'd personally like very basic and limited parameter
support for all utility statements so clients can use the v3 protocol and
parameter binding without having to figure out the type of statement.
Currently users have to know "Oh, this is a utility statement and can't be
parameterised, so instead of using my client driver's parameter binding I
have to do string interpolation".
SQL injection detection static analysis and trace tools might complain, and
it means we have to tell users to do exactly what they should otherwise
never do, but there's not really a way around it right now.
To make things more complicated, some client drivers use the simple-query
protocol and do client-side in-driver parameter interpolation. Like
psycopg2. Such drivers cannot easily enable use of server-side binding and
extended query protocol because right now they have to look at the SQL and
figure out which statements can be bound server-side and which require
client-side interpolation.
For drivers like PgJDBC that do support parameter binding the application
programmer has to know they can't use it for some kinds of statement, and
have to do string interpolation on the SQL string. Carefully, if they're
doing anything with client supplied data.
IMO this is a bit of a wart in Pg, and it'd be nice to get rid of it... but
I'm aware it might not be worth the server-side complexity of handling
parameter binding in utility statements.
But.
People will want to be able to parameterise identifiers too. There's just
no way to do that. For plannable or utility statements. You can't write
SELECT ... FROM $1;
or
COPY FROM $1 TO 'myfilename'
... and users have to know that and deal with it. By string interpolation.
So even if parameter binding for literals in utility statements becomes
possible it doesn't mean users never have to interpolate stuff.
I don't think it's much worse to say "you can't use parameter binding in
this statement type" than it is to say "you can't use paremeter binding for
identifiers". So I'm not really that excited to solve this unless there's a
way to solve it _just_ for SQL expressions within utility statements like
COPY (SELECT ...).
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Fri, May 27, 2016 at 6:02 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
COPY FROM $1 TO 'myfilename'
Random thought - how about at least making the following work:
For the following pretend that "STRING" has the same behavior as the
"format(...)" function.
EXECUTE STRING('COPY %I TO %L', 'testtable', 'testfile.txt');
<(conceptually similar to: EXECUTE format('COPY %I TO %L', 'testtable',
'testfile.txt')>
This doesn't solve the knowledge problem but at least provides an
idiomatic way to execute dynamic SQL without pl/pgsql and without forcing
the client library to take responsibility for proper data massaging in
order to eliminate sql injection.
As an extension making:
PREPARE name STRING('COPY %I TO %L', ?, ?);
EXECUTE name STRING USING ('testtable', 'testfile.txt');
David J.
For the following pretend that "STRING" has the same behavior as the
"format(...)" function.EXECUTE STRING('COPY %I TO %L', 'testtable', 'testfile.txt');
+1
We should make string sanitization easy so that people use it by default.
In the mean time, if you're just using psql, the new \gexec command will
cover that
select format('COPY %I TO %L', 'testtable', 'testfile.txt')
\gexec
but it won't help with any \-commands. And it won't work for
schema-qualified table names, and if you're using COPY tab FROM PROGRAM,
you're going to have cases where %L finds an escape-y character in the
command string (like using head -n 1 and sed to unpivot a header row) which
results in an E'...' string that COPY can't handle.
For \copy, I end up doing something like
select format('\\copy %I from program %L',:'table_name','pigz -cd ' ||
:'file_name') as copy_command
\gset
:copy_command
Which won't win any beauty contests, and suffers from all the limitations I
listed earlier, but works for me.
I'm indifferent to whether these commands need to be PREPARE-able so long
as sanitization becomes a solved problem.
On Fri, May 27, 2016 at 11:36 AM, Corey Huinker <corey.huinker@gmail.com> wrote:
For the following pretend that "STRING" has the same behavior as the
"format(...)" function.EXECUTE STRING('COPY %I TO %L', 'testtable', 'testfile.txt');
+1
-1
Why use syntax to do this? If we need a function with special
behaviors, let's make a function with special behaviors, hopefully not
called 'string()'...for example, format_safe(). Furthermore, we
already have functions to deal with safe string injection,
quote_ident/literal, and I'm not sure it's a good divert users away
from using those functions.
Either way, I don't think we should mix improvements to EXECUTE with
this patch, which I think addresses the problems I have with COPY
nicely (at least as it pertains to my pain points)...so +1 to Andrew's
proposal. I would rarely have to use dynamic SQL for COPY with that
in place. Other utility commands (off the top of my head) tend not to
be a problem for me (NOTIFY would be, but pg_notify handles that
case).
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers