Re: [HACKERS] Allow COPY to use parameters

[Merlin Moncure]

On Fri, May 27, 2016 at 11:36 AM, Corey Huinker  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

Re: [HACKERS] Allow COPY to use parameters

[Corey Huinker]

>
>
> 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.

Re: [HACKERS] Allow COPY to use parameters

[David G. Johnston]

On Fri, May 27, 2016 at 6:02 AM, Craig Ringer  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.

Re: [HACKERS] Allow COPY to use parameters

[Craig Ringer]

On 27 May 2016 at 15:17, Andrew Gierth  wrote:

> > "Merlin" == Merlin Moncure  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

Re: [HACKERS] Allow COPY to use parameters

[Andrew Gierth]

> "Merlin" == Merlin Moncure  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)

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, , 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,
 			 

Re: [HACKERS] Allow COPY to use parameters

[Andrew Gierth]

> "Merlin" == Merlin Moncure  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

Re: [HACKERS] Allow COPY to use parameters

[Tom Lane]

Merlin Moncure  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

Re: [HACKERS] Allow COPY to use parameters

[Merlin Moncure]

On Tue, May 24, 2016 at 3:42 PM, David Fetter  wrote:
> On Tue, May 24, 2016 at 02:16:40PM -0400, Tom Lane wrote:
>> David Fetter  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

Re: [HACKERS] Allow COPY to use parameters

[David Fetter]

On Tue, May 24, 2016 at 02:16:40PM -0400, Tom Lane wrote:
> David Fetter  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  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
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;
boolis_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(),
   

Re: [HACKERS] Allow COPY to use parameters

[Tom Lane]

David Fetter  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