Thread: [PATCH 0/4] COPY to a UDF: "COPY ... TO FUNCTION ..."
I have extended COPY to support using a UDF as a target instead of the normal 'file' or STDOUT targets. This dovetails nicely with a couple of extensions I have also written for dblink for the purposes of enabling direct cross-node bulk loading and replication. Please peruse the patches (the non-test containing patches also possess robust human-readable summaries and explanations) that are In-Reply-To this email for more details. You can also get these patches from a git repo. These patches are applied against the history tracked by git.postgresql.org: git fetch http://fdr.lolrus.org/postgresql.git \ copy-to-function:copy-to-function git checkout copy-to-function While the functionality exposed in these patches has appeared robust and efficient to us at Truviso, the code had ease-of-upstream merging as a central design point, and as such I have shied away from adding more invasive functionality that would make the interface less byzantine/more usable. This was intended to be the most surgical cut before it seemed likely that this might be interesting to the PostgreSQL project. At least one additional datapoint of someone else wanting such a functionality is seen in this thread: http://archives.postgresql.org/pgsql-hackers/2009-08/msg00428.php Open Issues: * setup/copy/teardown and error handling: as-is it is unhealthily tempting to use a global variable (as seen in the dblinkpatches) to track state between setup/copy/cleanup steps. I'm not sure what the right aesthetic is to make thisa little more controlled than calling specific functions in exactly the right order. * 'transition state': similar to an aggregate, it may make sense for the target of TO FUNCTION to have a context in whichit can stash state, or at least have access to a few constant parameters as it accepts records. If such functionalityexisted one might be able to conveniently rewrite the current COPY ... TO (STDOUT|'file') behavior to be syntacticsugar for TO FUNCTION behavior, which is somewhat aesthetically pleasing to me. * It might be interesting to increase the symmetry of this operation allowing COPY to bulk load into UDFs. With that inmind, the design the interfaces may change...or not. This work is released under the BSD license as utilized by the PostgreSQL project. The copyright owner is Truviso, Inc in 2009. Daniel Farina (4): Add "COPY ... TO FUNCTION ..." support Add tests for "COPY ... TO FUNCTION ..." Add dblink functions foruse with COPY ... TO FUNCTION ... Add tests to dblink covering use of COPY TO FUNCTION contrib/dblink/dblink.c | 190 ++++++++++++++++++++++++contrib/dblink/dblink.h | 5 +contrib/dblink/dblink.sql.in | 20 +++contrib/dblink/expected/dblink.out | 272 +++++++++++++++++++++++++++++++++++contrib/dblink/sql/dblink.sql | 112 ++++++++++++++contrib/dblink/uninstall_dblink.sql| 8 +src/backend/catalog/namespace.c | 21 +++src/backend/commands/copy.c | 190 +++++++++++++++++++++----src/backend/executor/spi.c | 2 +-src/backend/nodes/copyfuncs.c | 2 +-src/backend/nodes/equalfuncs.c | 2 +-src/backend/parser/gram.y | 30 +++--src/include/catalog/namespace.h | 1 +src/include/nodes/parsenodes.h | 3 +-src/test/regress/input/copy.source | 38 +++++src/test/regress/output/copy.source | 69 +++++++++src/test/regress/regress.c | 56 +++++++17 files changed, 982 insertions(+), 39 deletions(-)
This patch enables dblink to be used for the purpose of efficient
bulk-loading via COPY and libpq in combination with the COPY TO
FUNCTION patch.
The following functions were added to accomplish this:
dblink_connection_reset: useful when handling errors and one just
wants to restore a connection to a known state, rolling back as many
transactions as necessary.
dblink_copy_end: completes the COPY
dblink_copy_open: puts a connection into the COPY state. Accepts
connection name, relation name, and binary mode flag.
dblink_copy_write: writes a row to the last connection put in the COPY
state by dblink_copy_open.
Generally speaking, code that uses this will look like the following
(presuming a named connection has already been made):
try: SELECT dblink_copy_open('myconn', 'relation_name', true); COPY bar TO FUNCTION dblink_copy_write;
-- since the dblink connection is still in the COPY state, one -- can even copy some more data in multiple
steps...COPYbar_2 TO FUNCTION dblink_copy_write;
SELECT dblink_copy_end(); finally: SELECT dblink_copy_reset('myconn');
Signed-off-by: Daniel Farina <dfarina@truviso.com>
---contrib/dblink/dblink.c | 190 +++++++++++++++++++++++++++++++++++contrib/dblink/dblink.h |
5 +contrib/dblink/dblink.sql.in | 20 ++++contrib/dblink/uninstall_dblink.sql | 8 ++4 files changed, 223
insertions(+),0 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 72fdf56..d32aeec 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -1722,6 +1722,196 @@ dblink_get_notify(PG_FUNCTION_ARGS) * internal functions */
+/*
+ * Attempts to take the connection into a known state by rolling back
+ * transactions. If unable to restore the connection to a known idle state,
+ * raises an error.
+ */
+PG_FUNCTION_INFO_V1(dblink_connection_reset);
+Datum
+dblink_connection_reset(PG_FUNCTION_ARGS)
+{
+ PGresult *res = NULL;
+ PGconn *conn = NULL;
+ char *conname = NULL;
+ remoteConn *rconn = NULL;
+
+ bool triedonce = false;
+
+ DBLINK_INIT;
+
+ /* must be text */
+ Assert(PG_NARGS() == 1);
+ DBLINK_GET_NAMED_CONN;
+
+ if (!conn)
+ DBLINK_CONN_NOT_AVAIL;
+
+ while (!triedonce)
+ {
+ switch (PQtransactionStatus(conn))
+ {
+ case PQTRANS_IDLE:
+ /* Everything is okay */
+ goto finish;
+ case PQTRANS_ACTIVE:
+ case PQTRANS_INTRANS:
+ case PQTRANS_INERROR:
+ res = PQexec(conn, "ROLLBACK;");
+
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("%s: could not issue ROLLBACK command",
+ PG_FUNCNAME_MACRO)));
+
+ PQclear(res);
+ triedonce = true;
+ break;
+ case PQTRANS_UNKNOWN:
+ elog(ERROR, "%s: connection in unknown transaction state",
+ PG_FUNCNAME_MACRO);
+ }
+ }
+
+finish:
+ PG_RETURN_VOID();
+}
+
+/*
+ * dblink COPY support, procedures and variables
+ */
+static PGconn *dblink_copy_current = NULL;
+
+/*
+ * dblink_copy_open
+ *
+ * Start a COPY into a given relation on the named remote connection.
+ */
+PG_FUNCTION_INFO_V1(dblink_copy_open);
+Datum
+dblink_copy_open(PG_FUNCTION_ARGS)
+{
+ PGresult *res = NULL;
+ PGconn *conn = NULL;
+ char *conname = NULL;
+ remoteConn *rconn = NULL;
+
+ const char *copy_stmt = "COPY %s FROM STDIN%s;";
+ const char *with_binary = " WITH BINARY";
+ const char *quoted_remoted_relname;
+ bool isbinary;
+ int snprintf_retcode;
+
+ /*
+ * Should be large enough to contain any formatted output. Formed by
+ * counting the characters in the static formatting sections plus the
+ * bounded length of identifiers. Some modest padding was added for
+ * paranoia's sake, although all uses of this buffer are checked for
+ * over-length formats anyway.
+ */
+ char buf[64 + NAMEDATALEN];
+
+ DBLINK_INIT;
+
+ /* must be text,text,bool */
+ Assert(PG_NARGS() == 3);
+ DBLINK_GET_NAMED_CONN;
+
+ if (!conn)
+ DBLINK_CONN_NOT_AVAIL;
+
+ /* Read the procedure arguments into primitive values */
+ quoted_remoted_relname = NameListToQuotedString(
+ textToQualifiedNameList(PG_GETARG_TEXT_P(1)));
+ isbinary = PG_GETARG_BOOL(2);
+
+ /*
+ * Query parameterization only handles value-parameters -- of which
+ * identifiers are not considered one of -- so format the string the old
+ * fashioned way. It is very important to quote identifiers for this
+ * reason, as performed previously.
+ */
+ snprintf_retcode = snprintf(buf, sizeof buf, copy_stmt,
+ quoted_remoted_relname,
+ isbinary ? with_binary : "");
+
+ if (snprintf_retcode < 0)
+ elog(ERROR, "could not format dblink COPY query");
+ else if (snprintf_retcode >= sizeof buf)
+ /*
+ * Should not be able to happen, see documentation of the "buf" value
+ * in this procedure.
+ */
+ elog(ERROR, "could not fit formatted dblink COPY query into buffer");
+
+ /*
+ * Run the created query, and check to ensure that PGRES_COPY_IN state has
+ * been achieved.
+ */
+ res = PQexec(conn, buf);
+ if (!res || PQresultStatus(res) != PGRES_COPY_IN)
+ ereport(ERROR,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not start COPY FROM on remote node")));
+ PQclear(res);
+
+ /*
+ * Everything went well; finally bind the global dblink_copy_current to the
+ * connection ready to accept copy data.
+ */
+ dblink_copy_current = conn;
+ PG_RETURN_TEXT_P(cstring_to_text("OK"));
+}
+
+/*
+ * dblink_copy_write
+ *
+ * Write the provided StringInfo to the currently open COPY.
+ */
+PG_FUNCTION_INFO_V1(dblink_copy_write);
+Datum
+dblink_copy_write(PG_FUNCTION_ARGS)
+{
+ StringInfo copybuf = (void *) PG_GETARG_POINTER(0);
+
+ if (PQputCopyData(dblink_copy_current, copybuf->data, copybuf->len) != 1)
+ ereport(ERROR,
+ (errcode(ERRCODE_CONNECTION_EXCEPTION),
+ errmsg("could not send row to remote node")));
+
+ PG_RETURN_VOID();
+}
+
+/*
+ * dblink_copy_end
+ *
+ * Finish the currently open COPY.
+ */
+PG_FUNCTION_INFO_V1(dblink_copy_end);
+Datum
+dblink_copy_end(PG_FUNCTION_ARGS)
+{
+ PGresult *res;
+
+ /* Check to ensure that termination data was sent successfully */
+ if (PQputCopyEnd(dblink_copy_current, NULL) != 1)
+ elog(ERROR, "COPY end failed");
+
+ do
+ {
+ res = PQgetResult(dblink_copy_current);
+ if (res == NULL)
+ break;
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ elog(ERROR, "COPY failed: %s",
+ PQerrorMessage(dblink_copy_current));
+ PQclear(res);
+ } while (true);
+
+ dblink_copy_current = NULL;
+ PG_RETURN_TEXT_P(cstring_to_text("OK"));
+}/* * get_pkey_attnames
diff --git a/contrib/dblink/dblink.h b/contrib/dblink/dblink.h
index 255f5d0..8a2faee 100644
--- a/contrib/dblink/dblink.h
+++ b/contrib/dblink/dblink.h
@@ -59,4 +59,9 @@ extern Datum dblink_build_sql_update(PG_FUNCTION_ARGS);extern Datum
dblink_current_query(PG_FUNCTION_ARGS);externDatum dblink_get_notify(PG_FUNCTION_ARGS);
+extern Datum dblink_connection_reset(PG_FUNCTION_ARGS);
+
+extern Datum dblink_copy_open(PG_FUNCTION_ARGS);
+extern Datum dblink_copy_write(PG_FUNCTION_ARGS);
+extern Datum dblink_copy_end(PG_FUNCTION_ARGS);#endif /* DBLINK_H */
diff --git a/contrib/dblink/dblink.sql.in b/contrib/dblink/dblink.sql.in
index da5dd65..aedca34 100644
--- a/contrib/dblink/dblink.sql.in
+++ b/contrib/dblink/dblink.sql.in
@@ -221,3 +221,23 @@ CREATE OR REPLACE FUNCTION dblink_get_notify(RETURNS setof recordAS 'MODULE_PATHNAME',
'dblink_get_notify'LANGUAGEC STRICT;
+
+CREATE OR REPLACE FUNCTION dblink_connection_reset (text)
+RETURNS void
+AS 'MODULE_PATHNAME','dblink_connection_reset'
+LANGUAGE C STRICT;
+
+CREATE OR REPLACE FUNCTION dblink_copy_open (text, text, boolean)
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_copy_open'
+LANGUAGE C STRICT;
+
+CREATE OR REPLACE FUNCTION dblink_copy_write (internal)
+RETURNS void
+AS 'MODULE_PATHNAME','dblink_copy_write'
+LANGUAGE C STRICT;
+
+CREATE OR REPLACE FUNCTION dblink_copy_end ()
+RETURNS text
+AS 'MODULE_PATHNAME','dblink_copy_end'
+LANGUAGE C STRICT;
diff --git a/contrib/dblink/uninstall_dblink.sql b/contrib/dblink/uninstall_dblink.sql
index 45cf13c..465beb7 100644
--- a/contrib/dblink/uninstall_dblink.sql
+++ b/contrib/dblink/uninstall_dblink.sql
@@ -11,6 +11,14 @@ DROP FUNCTION dblink_build_sql_delete (text, int2vector, int4, _text);DROP FUNCTION
dblink_build_sql_insert(text, int2vector, int4, _text, _text);
+DROP FUNCTION dblink_copy_end ();
+
+DROP FUNCTION dblink_copy_open (text, text, boolean);
+
+DROP FUNCTION dblink_copy_write (internal);
+
+DROP FUNCTION dblink_connection_reset (text);
+DROP FUNCTION dblink_get_pkey (text);DROP TYPE dblink_pkey_results;
--
1.6.5.3
Signed-off-by: Daniel Farina <dfarina@truviso.com>
---src/test/regress/input/copy.source | 38 +++++++++++++++++++src/test/regress/output/copy.source | 69
+++++++++++++++++++++++++++++++++++src/test/regress/regress.c | 56 ++++++++++++++++++++++++++++3 files
changed,163 insertions(+), 0 deletions(-)
diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source
index 376329d..e5dcd62 100644
--- a/src/test/regress/input/copy.source
+++ b/src/test/regress/input/copy.source
@@ -107,3 +107,41 @@ this is just a line full of junk that would error out if parsedcopy copytest3 to stdout csv
header;
+
+-- test copy to function
+
+CREATE FUNCTION copyto_setup_state ()
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+
+CREATE FUNCTION copyto_function (internal)
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+
+CREATE FUNCTION copyto_yield_len ()
+ RETURNS int4
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+
+CREATE FUNCTION copyto_yield_text ()
+ RETURNS text
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+
+CREATE FUNCTION copyto_free ()
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+
+select copyto_setup_state();
+copy copytest to function copyto_function;
+select copyto_yield_len();
+select copyto_yield_text();
+select copyto_free();
+
+select copyto_setup_state();
+copy binary copytest to function copyto_function;
+select copyto_yield_len();
+select copyto_free();
diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source
index 5a88d6e..74ea935 100644
--- a/src/test/regress/output/copy.source
+++ b/src/test/regress/output/copy.source
@@ -71,3 +71,72 @@ copy copytest3 to stdout csv header;c1,"col with , comma","col with "" quote"1,a,12,b,2
+-- test copy to function
+CREATE FUNCTION copyto_setup_state ()
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+CREATE FUNCTION copyto_function (internal)
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+CREATE FUNCTION copyto_yield_len ()
+ RETURNS int4
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+CREATE FUNCTION copyto_yield_text ()
+ RETURNS text
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+CREATE FUNCTION copyto_free ()
+ RETURNS void
+ AS '@libdir@/regress@DLSUFFIX@'
+ LANGUAGE C;
+select copyto_setup_state();
+ copyto_setup_state
+--------------------
+
+(1 row)
+
+copy copytest to function copyto_function;
+select copyto_yield_len();
+ copyto_yield_len
+------------------
+ 76
+(1 row)
+
+select copyto_yield_text();
+ copyto_yield_text
+-------------------------------------------
+ DOS abc\r\ndef 1 +
+ Unix abc\ndef 2 +
+ Mac abc\rdef 3 +
+ esc\\ape a\\r\\\r\\\n\\nb 4+
+
+(1 row)
+
+select copyto_free();
+ copyto_free
+-------------
+
+(1 row)
+
+select copyto_setup_state();
+ copyto_setup_state
+--------------------
+
+(1 row)
+
+copy binary copytest to function copyto_function;
+select copyto_yield_len();
+ copyto_yield_len
+------------------
+ 142
+(1 row)
+
+select copyto_free();
+ copyto_free
+-------------
+
+(1 row)
+
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 0e94e68..a96a085 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -16,6 +16,7 @@#include "executor/spi.h"#include "utils/builtins.h"#include "utils/geo_decls.h"
+#include "utils/memutils.h"#define P_MAXDIG 12
@@ -34,6 +35,11 @@ extern char *reverse_name(char *string);extern int oldstyle_length(int n, text *t);extern Datum
int44in(PG_FUNCTION_ARGS);externDatum int44out(PG_FUNCTION_ARGS);
+extern Datum copyto_free(PG_FUNCTION_ARGS);
+extern Datum copyto_function(PG_FUNCTION_ARGS);
+extern Datum copyto_setup_state(PG_FUNCTION_ARGS);
+extern Datum copyto_yield_len(PG_FUNCTION_ARGS);
+extern Datum copyto_yield_text(PG_FUNCTION_ARGS);#ifdef PG_MODULE_MAGICPG_MODULE_MAGIC;
@@ -737,3 +743,53 @@ int44out(PG_FUNCTION_ARGS) *--walk = '\0'; PG_RETURN_CSTRING(result);}
+
+/*
+ * copyto testing
+ */
+static StringInfo global_buf;
+
+PG_FUNCTION_INFO_V1(copyto_setup_state);
+
+Datum
+copyto_setup_state(PG_FUNCTION_ARGS)
+{
+ MemoryContext oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+ global_buf = makeStringInfo();
+ MemoryContextSwitchTo(oldcxt);
+ PG_RETURN_VOID();
+}
+
+PG_FUNCTION_INFO_V1(copyto_function);
+
+Datum
+copyto_function(PG_FUNCTION_ARGS)
+{
+ StringInfo copybuf = (void *) PG_GETARG_POINTER(0);
+ appendBinaryStringInfo(global_buf, copybuf->data, copybuf->len);
+ PG_RETURN_VOID();
+}
+
+PG_FUNCTION_INFO_V1(copyto_yield_len);
+
+Datum
+copyto_yield_len(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_INT32(global_buf->len);
+}
+
+PG_FUNCTION_INFO_V1(copyto_yield_text);
+
+Datum
+copyto_yield_text(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_DATUM(DirectFunctionCall1(textin,
+ CStringGetDatum(global_buf->data)));
+}
+
+Datum
+copyto_free(PG_FUNCTION_ARGS)
+{
+ pfree(global_buf);
+ PG_RETURN_VOID();
+}
--
1.6.5.3
This relatively small change enables all sort of new and shiny evil by
allowing specification of a function to COPY that accepts each
produced row's content in turn. The function must accept a single
INTERNAL-type argument, which is actually of the type StringInfo.
Patch highlights:
- Grammar production changes to enable "TO FUNCTION <qualified name>"
- A new enumeration value in CopyDest to indicate this new mode called COPY_FN.
- CopyStateData's "filename" field has been renamed "destination" and is now a Node type. Before it was either a
stringor NULL; now it may be a RangeVar, a string, or NULL. Some code now has to go through some minor strVal()
unboxingfor the regular TO '/file' behavior.
- Due to the relatively restricted way this function can be called it was possible to reduce per-row overhead by
computingthe FunctionCallInfo once and then reusing it, as opposed to simply using one of the convenience functions
inthe fmgr.
- Add and expose the makeNameListFromRangeVar procedure to src/catalog/namespace.c, the inverse of
makeRangeVarFromNameList.
Signed-off-by: Daniel Farina <dfarina@truviso.com>
---src/backend/catalog/namespace.c | 21 +++++src/backend/commands/copy.c | 190
+++++++++++++++++++++++++++++++++-----src/backend/executor/spi.c | 2 +-src/backend/nodes/copyfuncs.c | 2
+-src/backend/nodes/equalfuncs.c | 2 +-src/backend/parser/gram.y | 30 ++++--src/include/catalog/namespace.h
| 1 +src/include/nodes/parsenodes.h | 3 +-8 files changed, 212 insertions(+), 39 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 99c9140..8911e29 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -2467,6 +2467,27 @@ QualifiedNameGetCreationNamespace(List *names, char **objname_p)}/*
+ * makeNameListFromRangeVar
+ * Utility routine to convert a qualified-name list into RangeVar form.
+ */
+List *
+makeNameListFromRangeVar(RangeVar *rangevar)
+{
+ List *names = NIL;
+
+ Assert(rangevar->relname != NULL);
+ names = lcons(makeString(rangevar->relname), names);
+
+ if (rangevar->schemaname != NULL)
+ names = lcons(makeString(rangevar->schemaname), names);
+
+ if (rangevar->catalogname != NULL)
+ names = lcons(makeString(rangevar->catalogname), names);
+
+ return names;
+}
+
+/* * makeRangeVarFromNameList * Utility routine to convert a qualified-name list into RangeVar form. */
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 5e95fd7..985505a 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -33,6 +33,7 @@#include "mb/pg_wchar.h"#include "miscadmin.h"#include "optimizer/planner.h"
+#include "parser/parse_func.h"#include "parser/parse_relation.h"#include "rewrite/rewriteHandler.h"#include
"storage/fd.h"
@@ -55,7 +56,8 @@ typedef enum CopyDest{ COPY_FILE, /* to/from file */ COPY_OLD_FE,
/* to/from frontend (2.0 protocol) */
- COPY_NEW_FE /* to/from frontend (3.0 protocol) */
+ COPY_NEW_FE, /* to/from frontend (3.0 protocol) */
+ COPY_FN /* to function */} CopyDest;/*
@@ -104,7 +106,8 @@ typedef struct CopyStateData Relation rel; /* relation to copy to or from */
QueryDesc *queryDesc; /* executable query to copy from */ List *attnumlist; /* integer list of
attnumsto copy */
- char *filename; /* filename, or NULL for STDIN/STDOUT */
+ Node *destination; /* filename, or NULL for STDIN/STDOUT, or a
+ * function */ bool binary; /* binary format? */ bool
oids; /* include OIDs? */ bool csv_mode; /* Comma Separated Value format? */
@@ -131,6 +134,13 @@ typedef struct CopyStateData MemoryContext rowcontext; /* per-row evaluation context */
/*
+ * For writing rows out to a function. Used if copy_dest == COPY_FN
+ *
+ * Avoids repeated use of DirectFunctionCall for efficiency.
+ */
+ FunctionCallInfoData output_fcinfo;
+
+ /* * These variables are used to reduce overhead in textual COPY FROM. * * attribute_buf holds the
separated,de-escaped text for each field of
@@ -425,9 +435,11 @@ CopySendEndOfRow(CopyState cstate){ StringInfo fe_msgbuf = cstate->fe_msgbuf;
+ /* Take care adding row delimiters*/ switch (cstate->copy_dest) { case COPY_FILE:
+ case COPY_FN: if (!cstate->binary) { /* Default line termination depends
onplatform */
@@ -437,6 +449,18 @@ CopySendEndOfRow(CopyState cstate) CopySendString(cstate, "\r\n");#endif
}
+ break;
+ case COPY_NEW_FE:
+ case COPY_OLD_FE:
+ /* The FE/BE protocol uses \n as newline for all platforms */
+ if (!cstate->binary)
+ CopySendChar(cstate, '\n');
+ break;
+ }
+
+ switch (cstate->copy_dest)
+ {
+ case COPY_FILE: (void) fwrite(fe_msgbuf->data, fe_msgbuf->len, 1,
cstate->copy_file);
@@ -446,10 +470,6 @@ CopySendEndOfRow(CopyState cstate) errmsg("could not write to COPY file:
%m"))); break; case COPY_OLD_FE:
- /* The FE/BE protocol uses \n as newline for all platforms */
- if (!cstate->binary)
- CopySendChar(cstate, '\n');
- if (pq_putbytes(fe_msgbuf->data, fe_msgbuf->len)) { /* no hope of recovering
connectionsync, so FATAL */
@@ -459,13 +479,19 @@ CopySendEndOfRow(CopyState cstate) } break; case COPY_NEW_FE:
- /* The FE/BE protocol uses \n as newline for all platforms */
- if (!cstate->binary)
- CopySendChar(cstate, '\n');
- /* Dump the accumulated row as one CopyData message */ (void) pq_putmessage('d',
fe_msgbuf->data,fe_msgbuf->len); break;
+ case COPY_FN:
+ FunctionCallInvoke(&cstate->output_fcinfo);
+
+ /*
+ * These are set earlier and are not supposed to change row to row.
+ */
+ Assert(cstate->output_fcinfo.arg[0] ==
+ PointerGetDatum(cstate->fe_msgbuf));
+ Assert(!cstate->output_fcinfo.argnull[0]);
+ break; } resetStringInfo(fe_msgbuf);
@@ -577,6 +603,12 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread) bytesread +=
avail; } break;
+ case COPY_FN:
+ /*
+ * Should be disallowed by some prior step
+ */
+ Assert(false);
+ break; } return bytesread;
@@ -719,7 +751,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString){ CopyState cstate; bool
is_from= stmt->is_from;
- bool pipe = (stmt->filename == NULL);
+ bool pipe = (stmt->destination == NULL); List *attnamelist = stmt->attlist; List
*force_quote= NIL; List *force_notnull = NIL;
@@ -986,6 +1018,14 @@ DoCopy(const CopyStmt *stmt, const char *queryString) errhint("Anyone can COPY to
stdoutor from stdin. " "psql's \\copy command also works for anyone.")));
+ /* Disallow COPY ... FROM FUNCTION (only TO FUNCTION supported) */
+ if (is_from && cstate->destination != NULL &&
+ IsA(cstate->destination, RangeVar))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("COPY FROM does not support functions as sources")));
+
+ if (stmt->relation) { Assert(!stmt->query);
@@ -1183,7 +1223,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString) cstate->encoding_embeds_ascii =
PG_ENCODING_IS_CLIENT_ONLY(cstate->client_encoding); cstate->copy_dest = COPY_FILE; /* default */
- cstate->filename = stmt->filename;
+ cstate->destination = stmt->destination; if (is_from) CopyFrom(cstate); /* copy from file to
database*/
@@ -1225,7 +1265,7 @@ DoCopy(const CopyStmt *stmt, const char *queryString)static voidDoCopyTo(CopyState cstate){
- bool pipe = (cstate->filename == NULL);
+ bool pipe = (cstate->destination == NULL); if (cstate->rel) {
@@ -1257,37 +1297,128 @@ DoCopyTo(CopyState cstate) else cstate->copy_file = stdout; }
- else
+ else if (IsA(cstate->destination, String)) { mode_t oumask; /* Pre-existing umask value */
struct stat st;
+ char *dest_filename = strVal(cstate->destination); /* * Prevent write to relative path
...too easy to shoot oneself in the * foot by overwriting a database file ... */
- if (!is_absolute_path(cstate->filename))
+ if (!is_absolute_path(dest_filename)) ereport(ERROR,
(errcode(ERRCODE_INVALID_NAME), errmsg("relative path not allowed for COPY to file")));
oumask= umask((mode_t) 022);
- cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_W);
+ cstate->copy_file = AllocateFile(dest_filename, PG_BINARY_W); umask(oumask); if
(cstate->copy_file== NULL) ereport(ERROR, (errcode_for_file_access(),
errmsg("couldnot open file \"%s\" for writing: %m",
- cstate->filename)));
+ dest_filename))); fstat(fileno(cstate->copy_file), &st); if
(S_ISDIR(st.st_mode)) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a directory", cstate->filename)));
+ errmsg("\"%s\" is a directory", dest_filename))); }
+ /* Branch taken in the "COPY ... TO FUNCTION funcname" situation */
+ else if (IsA(cstate->destination, RangeVar))
+ {
+ List *names;
+ FmgrInfo *flinfo;
+ FuncDetailCode fdresult;
+ Oid funcid;
+ Oid rettype;
+ bool retset;
+ int nvargs;
+ Oid *true_typeids;
+ const int nargs = 1;
+ Oid argtypes[] = { INTERNALOID };
+
+ /* Flip copy-action dispatch flag */
+ cstate->copy_dest = COPY_FN;
+
+ /* Make an fcinfo that can be reused and is stored on the cstate. */
+ names = makeNameListFromRangeVar((RangeVar *) cstate->destination);
+ flinfo = palloc0(sizeof *flinfo);
+
+
+ fdresult = func_get_detail(names, NIL, NIL, nargs, argtypes, false,
+ false,
+
+ /* Begin out-arguments */
+ &funcid, &rettype, &retset, &nvargs,
+ &true_typeids, NULL);
+
+ /*
+ * Check to ensure that this is a "normal" function when looked up,
+ * otherwise error.
+ */
+ switch (fdresult)
+ {
+ /* Normal function found; do nothing */
+ case FUNCDETAIL_NORMAL:
+ break;
+
+ case FUNCDETAIL_NOTFOUND:
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_FUNCTION),
+ errmsg("function %s does not exist",
+ func_signature_string(names, nargs, NIL,
+ argtypes))));
+ break;
+
+ case FUNCDETAIL_AGGREGATE:
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("function %s must not be an aggregate",
+ func_signature_string(names, nargs, NIL,
+ argtypes))));
+ break;
+
+ case FUNCDETAIL_WINDOWFUNC:
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("function %s must not be a window function",
+ func_signature_string(names, nargs, NIL,
+ argtypes))));
+ break;
+
+ case FUNCDETAIL_COERCION:
+ /*
+ * Should never be yielded from func_get_detail if it is passed
+ * fargs == NIL, as it is previously.
+ */
+ Assert(false);
+ break;
+
+ case FUNCDETAIL_MULTIPLE:
+ /*
+ * Only support one signature, thus overloading of a name with
+ * different types should never occur.
+ */
+ Assert(false);
+ break;
+
+ }
+
+ fmgr_info(funcid, flinfo);
+ InitFunctionCallInfoData(cstate->output_fcinfo, flinfo,
+ 1, NULL, NULL);
+ }
+ else
+ /* Unexpected type was found for cstate->destination. */
+ Assert(false);
+
+ PG_TRY(); { if (cstate->fe_copy)
@@ -1310,13 +1441,13 @@ DoCopyTo(CopyState cstate) } PG_END_TRY();
- if (!pipe)
+ if (!pipe && cstate->copy_dest != COPY_FN) { if (FreeFile(cstate->copy_file)) ereport(ERROR,
(errcode_for_file_access(), errmsg("could not write to file \"%s\": %m",
- cstate->filename)));
+ strVal(cstate->destination)))); }}
@@ -1342,6 +1473,13 @@ CopyTo(CopyState cstate) /* We use fe_msgbuf as a per-row buffer regardless of copy_dest */
cstate->fe_msgbuf = makeStringInfo();
+ /*
+ * fe_msgbuf is never rebound, so there is only a need to set up the
+ * output_fcinfo once.
+ */
+ cstate->output_fcinfo.arg[0] = PointerGetDatum(cstate->fe_msgbuf);
+ cstate->output_fcinfo.argnull[0] = false;
+ /* Get info about the columns we need to process. */ cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs
*sizeof(FmgrInfo)); foreach(cur, cstate->attnumlist)
@@ -1668,7 +1806,7 @@ limit_printout_length(const char *str)static voidCopyFrom(CopyState cstate){
- bool pipe = (cstate->filename == NULL);
+ bool pipe = (cstate->destination == NULL); HeapTuple tuple; TupleDesc tupDesc;
Form_pg_attribute*attr;
@@ -1768,19 +1906,21 @@ CopyFrom(CopyState cstate) { struct stat st;
- cstate->copy_file = AllocateFile(cstate->filename, PG_BINARY_R);
+ cstate->copy_file = AllocateFile(strVal(cstate->destination),
+ PG_BINARY_R); if (cstate->copy_file == NULL) ereport(ERROR,
(errcode_for_file_access(), errmsg("could not open file \"%s\" for reading: %m",
- cstate->filename)));
+ strVal(cstate->destination)))); fstat(fileno(cstate->copy_file), &st); if
(S_ISDIR(st.st_mode)) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a directory", cstate->filename)));
+ errmsg("\"%s\" is a directory",
+ strVal(cstate->destination)))); } tupDesc = RelationGetDescr(cstate->rel);
@@ -2215,7 +2355,7 @@ CopyFrom(CopyState cstate) ereport(ERROR,
(errcode_for_file_access(), errmsg("could not read from file \"%s\": %m",
- cstate->filename)));
+ strVal(cstate->destination)))); } /*
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index f045f9c..0914dc9 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1829,7 +1829,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI, {
CopyStmt *cstmt = (CopyStmt *) stmt;
- if (cstmt->filename == NULL)
+ if (cstmt->destination == NULL) { my_res =
SPI_ERROR_COPY; goto fail;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 8bc72d1..9b39abe 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2485,7 +2485,7 @@ _copyCopyStmt(CopyStmt *from) COPY_NODE_FIELD(query); COPY_NODE_FIELD(attlist);
COPY_SCALAR_FIELD(is_from);
- COPY_STRING_FIELD(filename);
+ COPY_NODE_FIELD(destination); COPY_NODE_FIELD(options); return newnode;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 3d65d8b..6ddf226 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1085,7 +1085,7 @@ _equalCopyStmt(CopyStmt *a, CopyStmt *b) COMPARE_NODE_FIELD(query);
COMPARE_NODE_FIELD(attlist); COMPARE_SCALAR_FIELD(is_from);
- COMPARE_STRING_FIELD(filename);
+ COMPARE_NODE_FIELD(destination); COMPARE_NODE_FIELD(options); return true;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 130e6f4..23331ee 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -251,8 +251,7 @@ static TypeName *TableFuncTypeName(List *columns);%type <value> TriggerFuncArg%type <node>
TriggerWhen
-%type <str> copy_file_name
- database_name access_method_clause access_method attr_name
+%type <str> database_name access_method_clause access_method attr_name index_name name
cursor_namefile_name cluster_index_specification%type <list> func_name handler_name qual_Op qual_all_Op subquery_Op
@@ -433,6 +432,8 @@ static TypeName *TableFuncTypeName(List *columns);%type <ival> opt_frame_clause frame_extent
frame_bound
+%type <node> copy_file_or_function_name
+/* * Non-keyword token types. These are hard-wired into the "flex" lexer. * They must be listed first so that their
numericcodes do not depend on
@@ -1977,14 +1978,15 @@ ClosePortalStmt:
*****************************************************************************/CopyStmt: COPY opt_binary
qualified_nameopt_column_list opt_oids
- copy_from copy_file_name copy_delimiter opt_with copy_options
+ copy_from copy_file_or_function_name copy_delimiter opt_with
+ copy_options { CopyStmt *n = makeNode(CopyStmt);
n->relation= $3; n->query = NULL; n->attlist = $4; n->is_from
=$6;
- n->filename = $7;
+ n->destination = $7; n->options = NIL; /* Concatenate
user-suppliedflags */
@@ -1998,14 +2000,15 @@ CopyStmt: COPY opt_binary qualified_name opt_column_list opt_oids
n->options= list_concat(n->options, $10); $$ = (Node *)n; }
- | COPY select_with_parens TO copy_file_name opt_with copy_options
+ | COPY select_with_parens TO copy_file_or_function_name
+ opt_with copy_options { CopyStmt *n = makeNode(CopyStmt);
n->relation = NULL; n->query = $2; n->attlist = NIL;
n->is_from= false;
- n->filename = $4;
+ n->destination = $4; n->options = $6; $$ = (Node *)n;
}
@@ -2021,10 +2024,17 @@ copy_from: * used depends on the direction. (It really doesn't make sense to copy from *
stdout.We silently correct the "typo".) - AY 9/94 */
-copy_file_name:
- Sconst { $$ = $1; }
- | STDIN { $$ = NULL; }
- | STDOUT { $$ = NULL; }
+copy_file_or_function_name:
+ Sconst { $$ = (Node *) makeString($1); }
+
+ /*
+ * Note that func_name is not used here because there is no need to
+ * accept the "funcname(TYPES)" construction, as there is only one
+ * valid signature.
+ */
+ | FUNCTION qualified_name { $$ = (Node *) $2; }
+ | STDIN { $$ = NULL; }
+ | STDOUT { $$ = NULL; } ;copy_options: copy_opt_list
{ $$ = $1; }
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index d356635..1d801cd 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -94,6 +94,7 @@ extern Oid LookupExplicitNamespace(const char *nspname);extern Oid
LookupCreationNamespace(constchar *nspname);extern Oid QualifiedNameGetCreationNamespace(List *names, char
**objname_p);
+extern List *makeNameListFromRangeVar(RangeVar *rangevar);extern RangeVar *makeRangeVarFromNameList(List
*names);externchar *NameListToString(List *names);extern char *NameListToQuotedString(List *names);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index b34300f..203088c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1293,7 +1293,8 @@ typedef struct CopyStmt List *attlist; /* List of column names (as Strings), or
NIL * for all columns */ bool is_from; /* TO or FROM */
- char *filename; /* filename, or NULL for STDIN/STDOUT */
+ Node *destination; /* filename, or NULL for STDIN/STDOUT, or a
+ * function */ List *options; /* List of DefElem nodes */} CopyStmt;
--
1.6.5.3
Signed-off-by: Daniel Farina <dfarina@truviso.com>
---contrib/dblink/expected/dblink.out | 272 ++++++++++++++++++++++++++++++++++++contrib/dblink/sql/dblink.sql |
112+++++++++++++++2 files changed, 384 insertions(+), 0 deletions(-)
diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out
index d39aa45..788b2a3 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -872,6 +872,278 @@ SELECT * from dblink_get_notify();-------------+--------+-------(0 rows)
+-- test COPY ... TO FUNCTION support
+CREATE SCHEMA dblink_copy_to_function;
+SET search_path = dblink_copy_to_function, public;
+CREATE TABLE xyzzy(f1 int, f2 text, f3 text[], primary key (f1,f2));
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "xyzzy_pkey" for table "xyzzy"
+INSERT INTO xyzzy VALUES (0,'a','{"a0","b0","c0"}');
+INSERT INTO xyzzy VALUES (1,'b','{"a1","b1","c1"}');
+INSERT INTO xyzzy VALUES (2,'c','{"a2","b2","c2"}');
+INSERT INTO xyzzy VALUES (3,'d','{"a3","b3","c3"}');
+INSERT INTO xyzzy VALUES (4,'e','{"a4","b4","c4"}');
+INSERT INTO xyzzy VALUES (5,'f','{"a5","b5","c5"}');
+INSERT INTO xyzzy VALUES (6,'g','{"a6","b6","c6"}');
+INSERT INTO xyzzy VALUES (7,'h','{"a7","b7","c7"}');
+INSERT INTO xyzzy VALUES (8,'i','{"a8","b8","c8"}');
+INSERT INTO xyzzy VALUES (9,'j','{"a9","b9","c9"}');
+CREATE TABLE bar(f1 int, f2 text, f3 text[], primary key (f1,f2));
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "bar_pkey" for table "bar"
+INSERT INTO bar VALUES (100,'w','{"a100","b100","c100"}');
+INSERT INTO bar VALUES (101,'x','{"a101","b101","c101"}');
+CREATE TABLE baz(f1 int, f2 text, f3 text[], primary key (f1,f2));
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "baz_pkey" for table "baz"
+INSERT INTO baz VALUES (102,'y','{"a102","b102","c102"}');
+INSERT INTO baz VALUES (103,'z','{"a103","b103","c103"}');
+CREATE TABLE plugh(f1 int, f2 text, f3 text[], primary key (f1,f2));
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "plugh_pkey" for table "plugh"
+INSERT INTO plugh VALUES (104,'u','{"a102","b102","c102"}');
+INSERT INTO plugh VALUES (105,'v','{"a103","b103","c103"}');
+SELECT dblink_connect('copytofunction','dbname=contrib_regression');
+ dblink_connect
+----------------
+ OK
+(1 row)
+
+SELECT dblink_exec('copytofunction',
+ 'SET search_path = dblink_copy_to_function, public;');
+ dblink_exec
+-------------
+ SET
+(1 row)
+
+-- ensure that original base data is present
+SELECT *
+FROM dblink('copytofunction', 'SELECT * FROM xyzzy') AS t(a int, b text, c text[]);
+ a | b | c
+---+---+------------
+ 0 | a | {a0,b0,c0}
+ 1 | b | {a1,b1,c1}
+ 2 | c | {a2,b2,c2}
+ 3 | d | {a3,b3,c3}
+ 4 | e | {a4,b4,c4}
+ 5 | f | {a5,b5,c5}
+ 6 | g | {a6,b6,c6}
+ 7 | h | {a7,b7,c7}
+ 8 | i | {a8,b8,c8}
+ 9 | j | {a9,b9,c9}
+(10 rows)
+
+-- try doing a few consecutive copies with one open connection
+SELECT dblink_copy_open('copytofunction', 'xyzzy', false);
+ dblink_copy_open
+------------------
+ OK
+(1 row)
+
+COPY bar TO FUNCTION dblink_copy_write;
+COPY baz TO FUNCTION dblink_copy_write;
+SELECT dblink_copy_end();
+ dblink_copy_end
+-----------------
+ OK
+(1 row)
+
+-- confirm that data has arrived
+SELECT *
+FROM dblink('copytofunction', 'SELECT * FROM xyzzy') AS t(a int, b text, c text[]);
+ a | b | c
+-----+---+------------------
+ 0 | a | {a0,b0,c0}
+ 1 | b | {a1,b1,c1}
+ 2 | c | {a2,b2,c2}
+ 3 | d | {a3,b3,c3}
+ 4 | e | {a4,b4,c4}
+ 5 | f | {a5,b5,c5}
+ 6 | g | {a6,b6,c6}
+ 7 | h | {a7,b7,c7}
+ 8 | i | {a8,b8,c8}
+ 9 | j | {a9,b9,c9}
+ 100 | w | {a100,b100,c100}
+ 101 | x | {a101,b101,c101}
+ 102 | y | {a102,b102,c102}
+ 103 | z | {a103,b103,c103}
+(14 rows)
+
+-- try doing a binary COPY
+SELECT dblink_copy_open('copytofunction', 'xyzzy', true);
+ dblink_copy_open
+------------------
+ OK
+(1 row)
+
+COPY plugh TO FUNCTION dblink_copy_write BINARY;
+SELECT dblink_copy_end();
+ dblink_copy_end
+-----------------
+ OK
+(1 row)
+
+-- confirm that data has arrived
+SELECT *
+FROM dblink('copytofunction', 'SELECT * FROM xyzzy') AS t(a int, b text, c text[]);
+ a | b | c
+-----+---+------------------
+ 0 | a | {a0,b0,c0}
+ 1 | b | {a1,b1,c1}
+ 2 | c | {a2,b2,c2}
+ 3 | d | {a3,b3,c3}
+ 4 | e | {a4,b4,c4}
+ 5 | f | {a5,b5,c5}
+ 6 | g | {a6,b6,c6}
+ 7 | h | {a7,b7,c7}
+ 8 | i | {a8,b8,c8}
+ 9 | j | {a9,b9,c9}
+ 100 | w | {a100,b100,c100}
+ 101 | x | {a101,b101,c101}
+ 102 | y | {a102,b102,c102}
+ 103 | z | {a103,b103,c103}
+ 104 | u | {a102,b102,c102}
+ 105 | v | {a103,b103,c103}
+(16 rows)
+
+-- try using reset to abort out of a copy state
+SELECT dblink_copy_open('copytofunction', 'xyzzy', true);
+ dblink_copy_open
+------------------
+ OK
+(1 row)
+
+COPY plugh TO FUNCTION dblink_copy_write BINARY;
+SELECT dblink_connection_reset('copytofunction');
+ dblink_connection_reset
+-------------------------
+
+(1 row)
+
+-- should fail, as COPY should have been aborted
+SELECT dblink_copy_end();
+ERROR: COPY end failed
+-- no new data should have appeared
+SELECT *
+FROM dblink('copytofunction', 'SELECT * FROM xyzzy') AS t(a int, b text, c text[]);
+ a | b | c
+-----+---+------------------
+ 0 | a | {a0,b0,c0}
+ 1 | b | {a1,b1,c1}
+ 2 | c | {a2,b2,c2}
+ 3 | d | {a3,b3,c3}
+ 4 | e | {a4,b4,c4}
+ 5 | f | {a5,b5,c5}
+ 6 | g | {a6,b6,c6}
+ 7 | h | {a7,b7,c7}
+ 8 | i | {a8,b8,c8}
+ 9 | j | {a9,b9,c9}
+ 100 | w | {a100,b100,c100}
+ 101 | x | {a101,b101,c101}
+ 102 | y | {a102,b102,c102}
+ 103 | z | {a103,b103,c103}
+ 104 | u | {a102,b102,c102}
+ 105 | v | {a103,b103,c103}
+(16 rows)
+
+-- should be a no-op, since no transaction should be active at this
+-- point
+SELECT dblink_connection_reset('copytofunction');
+ dblink_connection_reset
+-------------------------
+
+(1 row)
+
+-- generate an error in the remote transaction
+SELECT dblink_exec('copytofunction','BEGIN');
+ dblink_exec
+-------------
+ BEGIN
+(1 row)
+
+SELECT * FROM dblink('copytofunction', 'SELECT 1 / 0') AS t (a int);
+ERROR: division by zero
+CONTEXT: Error occurred on dblink connection named "unnamed": could not execute query.
+-- rollback the errored transaction
+SELECT dblink_connection_reset('copytofunction');
+ dblink_connection_reset
+-------------------------
+
+(1 row)
+
+-- should just work, if reset didn't actually reset the transaction
+-- state an error would result.
+SELECT * FROM dblink('copytofunction', 'SELECT 1;') AS t (a int);
+ a
+---
+ 1
+(1 row)
+
+-- try a really long identifier to test string handlig in
+-- dblink_copy_open. This should neatly hit NAMEDATALEN on most
+-- systems, or 64 - 1
+create table
+"012345678901234567890123456789012345678901234567890123456789012" (a int);
+-- should put the connection into the COPY state without complaint...
+SELECT dblink_copy_open('copytofunction',
+ '012345678901234567890123456789012345678901234567890123456789012',
+ true);
+ dblink_copy_open
+------------------
+ OK
+(1 row)
+
+COPY (SELECT generate_series(1, 5)) TO FUNCTION dblink_copy_write BINARY;
+SELECT dblink_copy_end();
+ dblink_copy_end
+-----------------
+ OK
+(1 row)
+
+-- check to see if data made it
+SELECT * FROM
+ "012345678901234567890123456789012345678901234567890123456789012";
+ a
+---
+ 1
+ 2
+ 3
+ 4
+ 5
+(5 rows)
+
+-- postgres truncates long identifiers and advertises with a NOTICE,
+-- and as of right now dblink does no remote-machine NOTICE handling.
+-- The result is silent truncation to the remote machine's
+-- NAMEDATALEN.
+SELECT dblink_copy_open('copytofunction',
+ '012345678901234567890123456789012345678901234567890123456789012345678',
+ true);
+ dblink_copy_open
+------------------
+ OK
+(1 row)
+
+COPY (SELECT generate_series(6, 10)) TO FUNCTION dblink_copy_write BINARY;
+SELECT dblink_copy_end();
+ dblink_copy_end
+-----------------
+ OK
+(1 row)
+
+-- check to see if data made it
+SELECT * FROM
+ "012345678901234567890123456789012345678901234567890123456789012";
+ a
+----
+ 1
+ 2
+ 3
+ 4
+ 5
+ 6
+ 7
+ 8
+ 9
+ 10
+(10 rows)
+SELECT dblink_disconnect(); dblink_disconnect -------------------
diff --git a/contrib/dblink/sql/dblink.sql b/contrib/dblink/sql/dblink.sql
index d0ad876..919fd78 100644
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -405,4 +405,116 @@ SELECT notify_name, be_pid = (select t.be_pid from dblink('select pg_backend_pidSELECT * from
dblink_get_notify();
+-- test COPY ... TO FUNCTION support
+CREATE SCHEMA dblink_copy_to_function;
+SET search_path = dblink_copy_to_function, public;
+CREATE TABLE xyzzy(f1 int, f2 text, f3 text[], primary key (f1,f2));
+INSERT INTO xyzzy VALUES (0,'a','{"a0","b0","c0"}');
+INSERT INTO xyzzy VALUES (1,'b','{"a1","b1","c1"}');
+INSERT INTO xyzzy VALUES (2,'c','{"a2","b2","c2"}');
+INSERT INTO xyzzy VALUES (3,'d','{"a3","b3","c3"}');
+INSERT INTO xyzzy VALUES (4,'e','{"a4","b4","c4"}');
+INSERT INTO xyzzy VALUES (5,'f','{"a5","b5","c5"}');
+INSERT INTO xyzzy VALUES (6,'g','{"a6","b6","c6"}');
+INSERT INTO xyzzy VALUES (7,'h','{"a7","b7","c7"}');
+INSERT INTO xyzzy VALUES (8,'i','{"a8","b8","c8"}');
+INSERT INTO xyzzy VALUES (9,'j','{"a9","b9","c9"}');
+
+CREATE TABLE bar(f1 int, f2 text, f3 text[], primary key (f1,f2));
+INSERT INTO bar VALUES (100,'w','{"a100","b100","c100"}');
+INSERT INTO bar VALUES (101,'x','{"a101","b101","c101"}');
+
+CREATE TABLE baz(f1 int, f2 text, f3 text[], primary key (f1,f2));
+INSERT INTO baz VALUES (102,'y','{"a102","b102","c102"}');
+INSERT INTO baz VALUES (103,'z','{"a103","b103","c103"}');
+
+CREATE TABLE plugh(f1 int, f2 text, f3 text[], primary key (f1,f2));
+INSERT INTO plugh VALUES (104,'u','{"a102","b102","c102"}');
+INSERT INTO plugh VALUES (105,'v','{"a103","b103","c103"}');
+
+SELECT dblink_connect('copytofunction','dbname=contrib_regression');
+SELECT dblink_exec('copytofunction',
+ 'SET search_path = dblink_copy_to_function, public;');
+
+-- ensure that original base data is present
+SELECT *
+FROM dblink('copytofunction', 'SELECT * FROM xyzzy') AS t(a int, b text, c text[]);
+
+-- try doing a few consecutive copies with one open connection
+SELECT dblink_copy_open('copytofunction', 'xyzzy', false);
+COPY bar TO FUNCTION dblink_copy_write;
+COPY baz TO FUNCTION dblink_copy_write;
+SELECT dblink_copy_end();
+
+-- confirm that data has arrived
+SELECT *
+FROM dblink('copytofunction', 'SELECT * FROM xyzzy') AS t(a int, b text, c text[]);
+
+-- try doing a binary COPY
+SELECT dblink_copy_open('copytofunction', 'xyzzy', true);
+COPY plugh TO FUNCTION dblink_copy_write BINARY;
+SELECT dblink_copy_end();
+
+-- confirm that data has arrived
+SELECT *
+FROM dblink('copytofunction', 'SELECT * FROM xyzzy') AS t(a int, b text, c text[]);
+
+-- try using reset to abort out of a copy state
+SELECT dblink_copy_open('copytofunction', 'xyzzy', true);
+COPY plugh TO FUNCTION dblink_copy_write BINARY;
+SELECT dblink_connection_reset('copytofunction');
+
+-- should fail, as COPY should have been aborted
+SELECT dblink_copy_end();
+
+-- no new data should have appeared
+SELECT *
+FROM dblink('copytofunction', 'SELECT * FROM xyzzy') AS t(a int, b text, c text[]);
+
+-- should be a no-op, since no transaction should be active at this
+-- point
+SELECT dblink_connection_reset('copytofunction');
+
+-- generate an error in the remote transaction
+SELECT dblink_exec('copytofunction','BEGIN');
+SELECT * FROM dblink('copytofunction', 'SELECT 1 / 0') AS t (a int);
+
+-- rollback the errored transaction
+SELECT dblink_connection_reset('copytofunction');
+
+-- should just work, if reset didn't actually reset the transaction
+-- state an error would result.
+SELECT * FROM dblink('copytofunction', 'SELECT 1;') AS t (a int);
+
+-- try a really long identifier to test string handlig in
+-- dblink_copy_open. This should neatly hit NAMEDATALEN on most
+-- systems, or 64 - 1
+create table
+"012345678901234567890123456789012345678901234567890123456789012" (a int);
+
+-- should put the connection into the COPY state without complaint...
+SELECT dblink_copy_open('copytofunction',
+ '012345678901234567890123456789012345678901234567890123456789012',
+ true);
+COPY (SELECT generate_series(1, 5)) TO FUNCTION dblink_copy_write BINARY;
+SELECT dblink_copy_end();
+
+-- check to see if data made it
+SELECT * FROM
+ "012345678901234567890123456789012345678901234567890123456789012";
+
+-- postgres truncates long identifiers and advertises with a NOTICE,
+-- and as of right now dblink does no remote-machine NOTICE handling.
+-- The result is silent truncation to the remote machine's
+-- NAMEDATALEN.
+SELECT dblink_copy_open('copytofunction',
+ '012345678901234567890123456789012345678901234567890123456789012345678',
+ true);
+COPY (SELECT generate_series(6, 10)) TO FUNCTION dblink_copy_write BINARY;
+SELECT dblink_copy_end();
+
+-- check to see if data made it
+SELECT * FROM
+ "012345678901234567890123456789012345678901234567890123456789012";
+SELECT dblink_disconnect();
--
1.6.5.3
On Mon, Nov 23, 2009 at 4:34 PM, Daniel Farina <dfarina@truviso.com> wrote: > Signed-off-by: Daniel Farina <dfarina@truviso.com> Thanks for the patch. You may want to take a look at this: http://wiki.postgresql.org/wiki/Submitting_a_Patch I'm fuzzy on what problem this is attempting to solve... as mentioned in the above guidelines, it's usually good to start with some design discussions before writing/submitting code. Also, we prefer that patches be submitted as context diffs and that they not be split up over multiple emails. Thanks, ...Robert
Robert Haas wrote: > I'm fuzzy on what problem this is attempting to solve... as mentioned > in the above guidelines, it's usually good to start with some design > discussions before writing/submitting code. This has been through some heavy design discussions with a few PG hackers you know and some you don't, they just couldn't release the result until now. As for what it's good for, if you look at what you can do now with dblink, you can easily move rows between nodes using dblink_build_sql_insert. This is perfectly fine for small bits of work, but the performance isn't nearly good enough to do serious replication with it. The upper level patch here allows using COPY as the mechanism to move things between them, which is much faster for some use cases (which includes most of the really useful ones). It dramatically increases the scale of what you can move around using dblink as the replication transport. The lower level patch is needed to build that layer, which is an immediate proof of its utility. In addition, adding a user-defined function as a COPY target opens up all sorts of possibilities for things like efficient ETL implementation. And if this approach is accepted as a reasonable one, as Dan suggested a next step might even be to similarly allow passing COPY FROM through a UDF, which has the potential to provide a new efficient implementation path for some of the custom input filter requests that pop up here periodically. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On Mon, Nov 23, 2009 at 2:16 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Nov 23, 2009 at 4:34 PM, Daniel Farina <dfarina@truviso.com> wrote: >> Signed-off-by: Daniel Farina <dfarina@truviso.com> > > Thanks for the patch. You may want to take a look at this: > > http://wiki.postgresql.org/wiki/Submitting_a_Patch > > I'm fuzzy on what problem this is attempting to solve... It seems somewhat strange that the only things COPY can do with its output stream of bytes is exactly two modes that are baked into Postgres in the core. This allows carefully written UDFs to do whatever they will with the stream of bytes, such as sending into a waiting libpq connection. > as mentioned in the above guidelines, it's usually good to start with some design > discussions before writing/submitting code. The patch is derived from functionality in the Truviso postgres-derived database product which is non-optional. This is extruded from that. > Also, we prefer that patches be submitted as context diffs I actually remembered this right after I sent it...sorry about that. > And that they not be split up over multiple emails. With the possible exception of squashing together the test cases into their implementing patches, I would say this is at least two patches. One is to a contrib, the other to core PostgreSQL. It so happens the core addition makes the contrib changes much more obviously useful. fdr
Greg Smith wrote: > Robert Haas wrote: >> I'm fuzzy on what problem this is attempting to solve... as mentioned >> in the above guidelines, it's usually good to start with some design >> discussions before writing/submitting code. > This has been through some heavy design discussions with a few PG > hackers you know and some you don't, they just couldn't release the > result until now. As for what it's good for, if you look at what you > can do now with dblink, you can easily move rows between nodes using > dblink_build_sql_insert. This is perfectly fine for small bits of > work, but the performance isn't nearly good enough to do serious > replication with it. The upper level patch here allows using COPY as > the mechanism to move things between them, which is much faster for > some use cases (which includes most of the really useful ones). It > dramatically increases the scale of what you can move around using > dblink as the replication transport. I recently found myself trying to push data through dblink() and ended up writing code to make a call to the target to call a function which called back to the source to select the data and insert it. The speedup was massive, so I'll be interested to dig into the details here. > The lower level patch is needed to build that layer, which is an > immediate proof of its utility. In addition, adding a user-defined > function as a COPY target opens up all sorts of possibilities for > things like efficient ETL implementation. And if this approach is > accepted as a reasonable one, as Dan suggested a next step might even > be to similarly allow passing COPY FROM through a UDF, which has the > potential to provide a new efficient implementation path for some of > the custom input filter requests that pop up here periodically. I'm also interested in COPY returning rows as text[], which was discussed recently. Does this help move us towards that? cheers andrew
On Mon, Nov 23, 2009 at 3:46 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > I recently found myself trying to push data through dblink() and ended up > writing code to make a call to the target to call a function which called > back to the source to select the data and insert it. The speedup was > massive, so I'll be interested to dig into the details here. The way the indirection is accomplished here should be very cheap. Overhead should be comparable to the fwrite() call that is used for copying to a file...this is why I mentioned that it would be interesting to make this good enough to be the underlying mechanism of TO STDOUT/TO 'file' to reduce the overall number of mechanisms used to perform COPY TO. > I'm also interested in COPY returning rows as text[], which was discussed > recently. Does this help move us towards that? Yes. Take a look at the tests introduced to core PostgeSQL (see patch 2), where instead of returning a text[] I return just a single text of the verbatim output of the copy. You could imagine making that an SRF instead. It would have to understand COPY row delimiters in whatever mode you were operating in, though. fdr
On Mon, Nov 23, 2009 at 4:03 PM, Daniel Farina <dfarina@truviso.com> wrote: > Yes. Take a look at the tests introduced to core PostgeSQL (see patch > 2), where instead of returning a text[] I return just a single text of > the verbatim output of the copy. You could imagine making that an SRF > instead. It would have to understand COPY row delimiters in whatever > mode you were operating in, though. Actually, sorry, I lie, but not in a bad way.... Since COPY operates row at a time (rather than a stream of bytes with arbitrary boundaries) you could rely on being passed each record one-at-a-time. You don't have to understand the delimiter. So you could even make a bytea[][] that even contains the binary output, the first dimension being row number, the second being the bytes themselves. The header would pose an interesting problem, though. fdr
Greg Smith <greg@2ndquadrant.com> writes: > Robert Haas wrote: >> I'm fuzzy on what problem this is attempting to solve... as mentioned >> in the above guidelines, it's usually good to start with some design >> discussions before writing/submitting code. > This has been through some heavy design discussions with a few PG > hackers you know and some you don't, they just couldn't release the > result until now. Those discussions don't have a lot of credibility if they didn't take place on the public mailing lists. pgsql-hackers had some preliminary discussions a couple months back on refactoring COPY to allow things like this --- see the thread starting here: http://archives.postgresql.org/pgsql-hackers/2009-09/msg00486.php While I don't think we arrived at any final decisions, I would like to know how this design fits in with what was discussed then. regards, tom lane
On Mon, Nov 23, 2009 at 4:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > pgsql-hackers had some preliminary discussions a couple months back > on refactoring COPY to allow things like this --- see the thread > starting here: > http://archives.postgresql.org/pgsql-hackers/2009-09/msg00486.php > While I don't think we arrived at any final decisions, I would like > to know how this design fits in with what was discussed then. This seems to be about importing/ingress, whereas this patch is about exporting/egress...it is an interesting question on how much parsing to do before on the ingress side before handing a row to a function though, should we try to make these kinds of operations a bit more symmetrical. I did consider refactoring COPY, but since it's never clear when we start a feature whether it is going to manifest itself as a good upstream candidate we default to trying to make future merges with Postgres tractable I did not take on such a large and artistic task. fdr
Tom Lane wrote: > Those discussions don't have a lot of credibility if they didn't take > place on the public mailing lists. > You know how people complain about how new contributors are treated here? Throwing out comments like this, that come off as belittling to other people's work, doesn't help. All I was suggesting was that Dan wasn't developing this in complete isolation from the hackers community as Robert had feared, as will be obvious when we get to: > pgsql-hackers had some preliminary discussions a couple months back > on refactoring COPY to allow things like this --- see the thread > starting here: > http://archives.postgresql.org/pgsql-hackers/2009-09/msg00486.php > While I don't think we arrived at any final decisions, I would like > to know how this design fits in with what was discussed then. > The patch provided here is a first step toward what you suggested in the related "Copy enhancements thread" a few days later, at http://archives.postgresql.org/pgsql-hackers/2009-09/msg00616.php It's one way to implement a better decoupled "data transformation" layer on top of COPY. When Dan showed me an earlier implementation of the basic idea embodied in this patch (developed independently and earlier actually), I gave it a thumbs-up as seeming to match the general direction the community discussion suggested, and encouraged him to work on getting the code to where it could be released. He started with output rather than input, mainly because the dblink feature had a compelling use-case that justified spending time on development for the company. Rather than keep going into input transformation, this development checkpoint made a good place to pause and solicit public feedback, particularly since the input side has additional hurdles to clear before it can work. As far as other past discussion here that might be relevant, this patch includes a direct change to gram.y to support the new syntax. You've already suggested before that it might be time to update COPY the same way EXPLAIN and now VACUUM have been overhauled to provide a more flexible options interface: http://archives.postgresql.org/pgsql-hackers/2009-09/msg00616.php This patch might be more fuel for that idea. Emmanuel has given up the more error tolerant COPY patch that thread was associated with, and I haven't heard anything from Andrew about ragged CVS import either. I think that ultimately those features are useful, but just exceed what the existing code could be hacked to handle cleanly. If it's possible to lower the complexity bar to implementing them by abstracting the transformation into a set of functions, and have more flexible syntax for the built-in ones the database ships with, that may be useful groundwork for returning to implementing those features without bloating COPY's internals (and therefore performance) intolerably. Dan even suggested in his first note that it might be possible to write the current STDOUT|file behavior in terms of the new function interface, which has the potential to make the COPY implementation cleaner rather than more cluttered (as long as the performance doesn't suffer). -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
Greg Smith wrote:
> I haven't heard anything from Andrew about ragged CVS import either.
> I think that ultimately those features are useful, but just exceed
> what the existing code could be hacked to handle cleanly.
The patch is attached for your edification/amusement. I have backpatched
it to 8.4 for the client that needed it, and it's working just fine. I
didn't pursue it when it was clear that it was not going to be accepted.
COPY returning text[] would allow us to achieve the same thing, a bit
more verbosely, but it would be a lot more work to develop.
cheers
andrew
Index: src/backend/commands/copy.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/copy.c,v
retrieving revision 1.316
diff -c -r1.316 copy.c
*** src/backend/commands/copy.c 29 Jul 2009 20:56:18 -0000 1.316
--- src/backend/commands/copy.c 13 Sep 2009 02:57:16 -0000
***************
*** 116,121 ****
--- 116,122 ----
char *escape; /* CSV escape char (must be 1 byte) */
bool *force_quote_flags; /* per-column CSV FQ flags */
bool *force_notnull_flags; /* per-column CSV FNN flags */
+ bool ragged; /* allow ragged CSV input? */
/* these are just for error messages, see copy_in_error_callback */
const char *cur_relname; /* table name for error messages */
***************
*** 822,827 ****
--- 823,836 ----
errmsg("conflicting or redundant options")));
force_notnull = (List *) defel->arg;
}
+ else if (strcmp(defel->defname, "ragged") == 0)
+ {
+ if (cstate->ragged)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options")));
+ cstate->ragged = intVal(defel->arg);
+ }
else
elog(ERROR, "option \"%s\" not recognized",
defel->defname);
***************
*** 948,953 ****
--- 957,972 ----
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("COPY force not null only available using COPY FROM")));
+ /* Check ragged */
+ if (!cstate->csv_mode && cstate->ragged)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("COPY ragged available only in CSV mode")));
+ if (cstate->ragged && !is_from)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("COPY ragged only available using COPY FROM")));
+
/* Don't allow the delimiter to appear in the null string. */
if (strchr(cstate->null_print, cstate->delim[0]) != NULL)
ereport(ERROR,
***************
*** 2951,2964 ****
int input_len;
/* Make sure space remains in fieldvals[] */
! if (fieldno >= maxfields)
ereport(ERROR,
(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
errmsg("extra data after last expected column")));
/* Remember start of field on both input and output sides */
start_ptr = cur_ptr;
! fieldvals[fieldno] = output_ptr;
/*
* Scan data for field,
--- 2970,2984 ----
int input_len;
/* Make sure space remains in fieldvals[] */
! if (fieldno >= maxfields && ! cstate->ragged)
ereport(ERROR,
(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
errmsg("extra data after last expected column")));
/* Remember start of field on both input and output sides */
start_ptr = cur_ptr;
! if (fieldno < maxfields)
! fieldvals[fieldno] = output_ptr;
/*
* Scan data for field,
***************
*** 3045,3051 ****
/* Check whether raw input matched null marker */
input_len = end_ptr - start_ptr;
if (!saw_quote && input_len == cstate->null_print_len &&
! strncmp(start_ptr, cstate->null_print, input_len) == 0)
fieldvals[fieldno] = NULL;
fieldno++;
--- 3065,3072 ----
/* Check whether raw input matched null marker */
input_len = end_ptr - start_ptr;
if (!saw_quote && input_len == cstate->null_print_len &&
! strncmp(start_ptr, cstate->null_print, input_len) == 0 &&
! fieldno < maxfields)
fieldvals[fieldno] = NULL;
fieldno++;
***************
*** 3059,3065 ****
Assert(*output_ptr == '\0');
cstate->attribute_buf.len = (output_ptr - cstate->attribute_buf.data);
! return fieldno;
}
--- 3080,3092 ----
Assert(*output_ptr == '\0');
cstate->attribute_buf.len = (output_ptr - cstate->attribute_buf.data);
! /* for ragged input, set field null for underflowed fields */
! if (cstate->ragged)
! while (fieldno < maxfields)
! fieldvals[fieldno++] = NULL;
!
!
! return cstate->ragged ? maxfields : fieldno;
}
Index: src/backend/parser/gram.y
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.677
diff -c -r2.677 gram.y
*** src/backend/parser/gram.y 18 Aug 2009 23:40:20 -0000 2.677
--- src/backend/parser/gram.y 13 Sep 2009 02:57:17 -0000
***************
*** 504,510 ****
QUOTE
! RANGE READ REAL REASSIGN RECHECK RECURSIVE REFERENCES REINDEX
RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA RESET RESTART
RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROW ROWS RULE
--- 504,510 ----
QUOTE
! RAGGED RANGE READ REAL REASSIGN RECHECK RECURSIVE REFERENCES REINDEX
RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA RESET RESTART
RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROW ROWS RULE
***************
*** 2050,2055 ****
--- 2050,2059 ----
{
$$ = makeDefElem("force_notnull", (Node *)$4);
}
+ | RAGGED
+ {
+ $$ = makeDefElem("ragged",(Node *)makeInteger(TRUE));
+ }
;
/* The following exist for backward compatibility */
***************
*** 10373,10378 ****
--- 10377,10383 ----
| PROCEDURAL
| PROCEDURE
| QUOTE
+ | RAGGED
| RANGE
| READ
| REASSIGN
Index: src/bin/psql/copy.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/copy.c,v
retrieving revision 1.82
diff -c -r1.82 copy.c
*** src/bin/psql/copy.c 7 Aug 2009 20:16:11 -0000 1.82
--- src/bin/psql/copy.c 13 Sep 2009 02:57:17 -0000
***************
*** 34,40 ****
* The documented syntax is:
* \copy tablename [(columnlist)] from|to filename
* [ with ] [ binary ] [ oids ] [ delimiter [as] char ] [ null [as] string ]
! * [ csv [ header ] [ quote [ AS ] string ] escape [as] string
* [ force not null column [, ...] | force quote column [, ...] | * ] ]
*
* \copy ( select stmt ) to filename
--- 34,40 ----
* The documented syntax is:
* \copy tablename [(columnlist)] from|to filename
* [ with ] [ binary ] [ oids ] [ delimiter [as] char ] [ null [as] string ]
! * [ csv [ header ] [ quote [ AS ] string ] escape [as] string [ ragged ]
* [ force not null column [, ...] | force quote column [, ...] | * ] ]
*
* \copy ( select stmt ) to filename
***************
*** 69,74 ****
--- 69,75 ----
char *escape;
char *force_quote_list;
char *force_notnull_list;
+ bool ragged;
};
***************
*** 268,273 ****
--- 269,276 ----
result->csv_mode = true;
else if (pg_strcasecmp(token, "header") == 0)
result->header = true;
+ else if (pg_strcasecmp(token, "ragged") == 0)
+ result->ragged = true;
else if (pg_strcasecmp(token, "delimiter") == 0)
{
if (result->delim)
***************
*** 477,482 ****
--- 480,488 ----
if (options->header)
appendPQExpBuffer(&query, " HEADER");
+ if (options->ragged)
+ appendPQExpBuffer(&query, " RAGGED");
+
if (options->quote)
emit_copy_option(&query, " QUOTE AS ", options->quote);
Index: src/bin/psql/tab-complete.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/tab-complete.c,v
retrieving revision 1.185
diff -c -r1.185 tab-complete.c
*** src/bin/psql/tab-complete.c 2 Aug 2009 22:14:52 -0000 1.185
--- src/bin/psql/tab-complete.c 13 Sep 2009 02:57:18 -0000
***************
*** 1249,1255 ****
pg_strcasecmp(prev3_wd, "TO") == 0))
{
static const char *const list_CSV[] =
! {"HEADER", "QUOTE", "ESCAPE", "FORCE QUOTE", NULL};
COMPLETE_WITH_LIST(list_CSV);
}
--- 1249,1255 ----
pg_strcasecmp(prev3_wd, "TO") == 0))
{
static const char *const list_CSV[] =
! {"HEADER", "QUOTE", "ESCAPE", "FORCE QUOTE", "RAGGED", NULL};
COMPLETE_WITH_LIST(list_CSV);
}
Index: src/include/parser/kwlist.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/parser/kwlist.h,v
retrieving revision 1.2
diff -c -r1.2 kwlist.h
*** src/include/parser/kwlist.h 6 Apr 2009 08:42:53 -0000 1.2
--- src/include/parser/kwlist.h 13 Sep 2009 02:57:18 -0000
***************
*** 294,299 ****
--- 294,300 ----
PG_KEYWORD("procedural", PROCEDURAL, UNRESERVED_KEYWORD)
PG_KEYWORD("procedure", PROCEDURE, UNRESERVED_KEYWORD)
PG_KEYWORD("quote", QUOTE, UNRESERVED_KEYWORD)
+ PG_KEYWORD("ragged", RAGGED, UNRESERVED_KEYWORD)
PG_KEYWORD("range", RANGE, UNRESERVED_KEYWORD)
PG_KEYWORD("read", READ, UNRESERVED_KEYWORD)
PG_KEYWORD("real", REAL, COL_NAME_KEYWORD)
On Mon, 2009-11-23 at 18:31 -0500, Greg Smith wrote: > Robert Haas wrote: > > I'm fuzzy on what problem this is attempting to solve... as mentioned > > in the above guidelines, it's usually good to start with some design > > discussions before writing/submitting code. > This has been through some heavy design discussions with a few PG > hackers you know and some you don't, they just couldn't release the > result until now. As for what it's good for, if you look at what you > can do now with dblink, you can easily move rows between nodes using > dblink_build_sql_insert. This is perfectly fine for small bits of work, > but the performance isn't nearly good enough to do serious replication > with it. The upper level patch here allows using COPY as the mechanism > to move things between them, which is much faster for some use cases > (which includes most of the really useful ones). It dramatically > increases the scale of what you can move around using dblink as the > replication transport. > > The lower level patch is needed to build that layer, which is an > immediate proof of its utility. In addition, adding a user-defined > function as a COPY target opens up all sorts of possibilities for things > like efficient ETL implementation. And if this approach is accepted as > a reasonable one, as Dan suggested a next step might even be to > similarly allow passing COPY FROM through a UDF, which has the potential > to provide a new efficient implementation path for some of the custom > input filter requests that pop up here periodically. Can this easily be extended to do things like COPY stdin TO udf(); or COPY udf() FROM stdin; so that I could write a simple partitioning function, either local for partitioned tables or using pl/proxy for partitioned databases ? > -- > Greg Smith 2ndQuadrant Baltimore, MD > PostgreSQL Training, Services and Support > greg@2ndQuadrant.com www.2ndQuadrant.com > >
On Mon, 2009-11-23 at 16:25 -0800, Daniel Farina wrote: > On Mon, Nov 23, 2009 at 4:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > pgsql-hackers had some preliminary discussions a couple months back > > on refactoring COPY to allow things like this --- see the thread > > starting here: > > http://archives.postgresql.org/pgsql-hackers/2009-09/msg00486.php > > While I don't think we arrived at any final decisions, I would like > > to know how this design fits in with what was discussed then. > > This seems to be about importing/ingress, whereas this patch is about > exporting/egress...it is an interesting question on how much parsing > to do before on the ingress side before handing a row to a function > though, My suggestion for COPY func(rowtype) FROM stdin; would be to pass the function a fully processed row of that type with all fields resolved and converted to right types. it may be useful to also have forms like COPY func(text) FROM stdin; and COPY func(bytea[]) FROM stdin; for getting a less processed input > should we try to make these kinds of operations a bit more > symmetrical. -- Hannu Krosing http://www.2ndQuadrant.com PostgreSQL Scalability and Availability Services, Consulting and Training
On Tue, Nov 24, 2009 at 12:29 AM, Hannu Krosing <hannu@krosing.net> wrote: > COPY stdin TO udf(); If stdin becomes (is?) a legitimate source of records, then this patch will Just Work. The patch is already quite useful in the COPY (SELECT ...) TO FUNCTION ... scenario. > COPY udf() FROM stdin; This is unaddressed, but I think it would be a good idea to consider enabling this kind of thing prior to application. fdr
On Tue, Nov 24, 2009 at 12:38 AM, Hannu Krosing <hannu@2ndquadrant.com> wrote: > COPY func(rowtype) FROM stdin; I didn't consider rowtype...I did consider a type list, such as: COPY func(typea, typeb, typec) FROM ... Which would then operate just like a table, but be useless for the data-cleaning case, and would not allow type overloading to be the mechanism of selecting behavior. It was just an idea...I don't really know the use cases well enough, my anticipated used case was updating eagerly materialized quantities with the UDF. fdr
Hello I thing, so this patch is maybe good idea. I am missing better function specification. Specification by name isn't enough - we can have a overloaded functions. This syntax doesn't allow to use explicit cast - from my personal view, the syntax is ugly - with type specification we don't need to keyword FUNCTION what about:: var a) by type specification COPY table TO foo(varchar, int) var b) by value expression - it allows some changes in order, casting, constants COPY table TO foo($3, $1::date, $2::text, CURRENT_DATE, true); One question: We have a fast copy statement - ok., we have a fast function ok, but inside a function we have to call "slow" sql query. Personally What is advantage? We need pipes like like COPY table TO foo(..) TO table foo() should be a transformation function, or real pipe function Regards Pavel Stehule
On Tue, Nov 24, 2009 at 2:10 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > Hello > > I thing, so this patch is maybe good idea. I am missing better > function specification. Specification by name isn't enough - we can > have a overloaded functions. This syntax doesn't allow to use explicit > cast - from my personal view, the syntax is ugly - with type > specification we don't need to keyword FUNCTION As long as things continue to support the INTERNAL-type behavior for extremely low overhead bulk transfers I am open to suggestions about how to enrich things...but how would I do so under this proposal? I am especially fishing for suggestions in the direction of managing state for the function between rows though...I don't like how the current design seems to scream "use a global variable." > We have a fast copy statement - ok., we have a fast function ok, but > inside a function we have to call "slow" sql query. Personally What is > advantage? The implementation here uses a type 'internal' for performance. It doesn't even recompute the fcinfo because of the very particular circumstances of how the function is called. It doesn't do a memory copy of the argument buffer either, to the best of my knowledge. In the dblink patches you basically stream directly from the disk, format the COPY bytes, and shove it into a waiting COPY on another postgres node...there's almost no additional work in-between. All utilized time would be some combination of the normal COPY byte stream generation and libpq. This, of course, presumes that everyone who is interested in building on this is going to use some UDFs written in C... > > We need pipes like > > like COPY table TO foo(..) TO table > > foo() should be a transformation function, or real pipe function I've actually considered this pipe thing with a colleague while driving home from work...it occurred to us that it would be nice to have both pipes and tees (basically composition vs. mapping application of functions over the input) in some form. Not sure what an elegant way to express that is or how to control it. Since you can work around this by composing or applying functions on your own in another function, I'm not sure if that's as high priority for me personally. fdr
On Tue, 2009-11-24 at 02:37 -0800, Daniel Farina wrote: > On Tue, Nov 24, 2009 at 2:10 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > Hello > > > > I thing, so this patch is maybe good idea. I am missing better > > function specification. Specification by name isn't enough - we can > > have a overloaded functions. This syntax doesn't allow to use explicit > > cast - from my personal view, the syntax is ugly - with type > > specification we don't need to keyword FUNCTION > > As long as things continue to support the INTERNAL-type behavior for > extremely low overhead bulk transfers I am open to suggestions about > how to enrich things...but how would I do so under this proposal? > > I am especially fishing for suggestions in the direction of managing > state for the function between rows though...I don't like how the > current design seems to scream "use a global variable." Can't you use existing aggregate function design ? CREATE AGGREGATE name ( input_data_type [ , ... ] ) ( SFUNC = sfunc, STYPE = state_data_type [ , FINALFUNC = ffunc] [ , INITCOND = initial_condition ] [ , SORTOP = sort_operator ] ) and maybe use additional INITFUNC=, if you need it for dblink type things which don't do connection management it automatically like pl/proxy does. -- Hannu Krosing http://www.2ndQuadrant.com PostgreSQL Scalability and Availability Services, Consulting and Training
On Tue, Nov 24, 2009 at 2:50 AM, Hannu Krosing <hannu@2ndquadrant.com> wrote: > Can't you use existing aggregate function design ? > > CREATE AGGREGATE name ( input_data_type [ , ... ] ) ( > SFUNC = sfunc, > STYPE = state_data_type > [ , FINALFUNC = ffunc ] > [ , INITCOND = initial_condition ] > [ , SORTOP = sort_operator ] > ) Actually, yes. I just thought that this was an idea so crazy that no one would like it. fdr
On Tue, Nov 24, 2009 at 2:51 AM, Daniel Farina <drfarina@gmail.com> wrote:
> On Tue, Nov 24, 2009 at 2:50 AM, Hannu Krosing <hannu@2ndquadrant.com> wrote:
>> Can't you use existing aggregate function design ?
>>
>> CREATE AGGREGATE name ( input_data_type [ , ... ] ) (
>> SFUNC = sfunc,
>> STYPE = state_data_type
>> [ , FINALFUNC = ffunc ]
>> [ , INITCOND = initial_condition ]
>> [ , SORTOP = sort_operator ]
>> )
>
> Actually, yes. I just thought that this was an idea so crazy that no
> one would like it.
Oh, and the other elephant in the room: error handling. How to handle
error conditions...try/catch/finally type stuff. Aggregates do not
necessarily provide a slot for this one. I did consider using
aggregates though, but somehow it felt to me like "I need at least a
three-tuple, why not fish around for any random bundling of three
functions..."
After all, I would not want to actually call the nodeAgg stuff to
apply the function anyway...so it'd basically be abused as a
three-tuple of functions.
Also, what if you wanted, say, replace the mechanism for COPY TO
'file'? It'd be nice to make the following interaction (which uses
some implied global variables) not use such global variables:
BEGIN;
select open_file('/tmp/file', 'w+');
copy foo to function write_to_file;
-- what happens here if COPY aborts? Does the transaction being in
the error state mean that files will not get closed?
select close_file();
COMMIT;
fdr
On Tue, 2009-11-24 at 02:56 -0800, Daniel Farina wrote:
> On Tue, Nov 24, 2009 at 2:51 AM, Daniel Farina <drfarina@gmail.com> wrote:
> > On Tue, Nov 24, 2009 at 2:50 AM, Hannu Krosing <hannu@2ndquadrant.com> wrote:
> >> Can't you use existing aggregate function design ?
> >>
> >> CREATE AGGREGATE name ( input_data_type [ , ... ] ) (
> >> SFUNC = sfunc,
> >> STYPE = state_data_type
> >> [ , FINALFUNC = ffunc ]
> >> [ , INITCOND = initial_condition ]
> >> [ , SORTOP = sort_operator ]
> >> )
> >
> > Actually, yes. I just thought that this was an idea so crazy that no
> > one would like it.
seems kind of natural choice for me - in essence this is an aggregate
function, aggregating over rows/tuples supplied to it.
> Oh, and the other elephant in the room: error handling. How to handle
> error conditions...try/catch/finally type stuff.
Same as current aggregates - either ignore the error, logi it and
continue, or bail out
> Aggregates do not necessarily provide a slot for this one.
Neither do ordinary funtions, we have no "ON ERROR DO ..." clause for
function definitions
> I did consider using
> aggregates though, but somehow it felt to me like "I need at least a
> three-tuple, why not fish around for any random bundling of three
> functions..."
Why do you need three ?
> After all, I would not want to actually call the nodeAgg stuff to
> apply the function anyway...so it'd basically be abused as a
> three-tuple of functions.
Actually it would be best if it could use straight generic funtions, so
you could do something like
COPY stdin TO filterfunc(int) TO avg(int);
You can bypass using nodeAgg in your own C functions as an optimisation.
> Also, what if you wanted, say, replace the mechanism for COPY TO
> 'file'? It'd be nice to make the following interaction (which uses
> some implied global variables) not use such global variables:
>
> BEGIN;
> select open_file('/tmp/file', 'w+');
> copy foo to function write_to_file;
> -- what happens here if COPY aborts? Does the transaction being in
> the error state mean that files will not get closed?
> select close_file();
> COMMIT;
pass the file name in as an argument to SFUNC, open it on first call,
ignore later (if it stays the same ;)
for foreign connections use SQL-MED and pass the handle to "foreign
data"
--
Hannu Krosing http://www.2ndQuadrant.com
PostgreSQL Scalability and Availability Services, Consulting and Training
On Tue, Nov 24, 2009 at 3:25 AM, Hannu Krosing <hannu@2ndquadrant.com> wrote: > On Tue, 2009-11-24 at 02:56 -0800, Daniel Farina wrote: >> On Tue, Nov 24, 2009 at 2:51 AM, Daniel Farina <drfarina@gmail.com> wrote: >> > On Tue, Nov 24, 2009 at 2:50 AM, Hannu Krosing <hannu@2ndquadrant.com> wrote: >> >> Can't you use existing aggregate function design ? >> >> >> >> CREATE AGGREGATE name ( input_data_type [ , ... ] ) ( >> >> SFUNC = sfunc, >> >> STYPE = state_data_type >> >> [ , FINALFUNC = ffunc ] >> >> [ , INITCOND = initial_condition ] >> >> [ , SORTOP = sort_operator ] >> >> ) >> > >> > Actually, yes. I just thought that this was an idea so crazy that no >> > one would like it. > > seems kind of natural choice for me - in essence this is an aggregate > function, aggregating over rows/tuples supplied to it. Okay, well, maybe that wasn't such a crazy idea after all... >> Oh, and the other elephant in the room: error handling. How to handle >> error conditions...try/catch/finally type stuff. > > Same as current aggregates - either ignore the error, logi it and > continue, or bail out >[snip] > Neither do ordinary funtions, we have no "ON ERROR DO ..." clause for > function definitions It is assumed most functions do not have side effects outside the database, so this is gotten rather for free. The driving use case for this *is* side effects on other systems. I'm not sure if it's as easy to use this justification here...normally rollbacks just take care of all the error handling a function would want. Here I'm not so sure that is as common a case. > >> I did consider using >> aggregates though, but somehow it felt to me like "I need at least a >> three-tuple, why not fish around for any random bundling of three >> functions..." > > Why do you need three ? I'm counting the aggregate prototype itself to refer to the bundle, which I suppose would be more normally considered a two-tuple of functions. This is a self-referential tuple, I suppose... >> After all, I would not want to actually call the nodeAgg stuff to >> apply the function anyway...so it'd basically be abused as a >> three-tuple of functions. > > Actually it would be best if it could use straight generic funtions, so > you could do something like > > COPY stdin TO filterfunc(int) TO avg(int); Generic functions? Do you mean just scalar functions? That'd be neat, but as I said previously, composition could just be wrapped into a function of the user's choice. Also, what about use of multi-function-apply? COPY stdin TO replicant1(datum) AND replicant2(datum); You could imagine all sorts of new 2PC evil. But again, one could just write a little function to absorb the rows and dole them out without bloating COPY syntax... I am in no way suggesting that syntax seriously or unseriously. > pass the file name in as an argument to SFUNC, open it on first call, > ignore later (if it stays the same ;) So either you are going to pass it with every row and ignore it, or create a new initial aggregate state for each COPY TO FUNCTION...how are you going to get it passed to SFUNC? fdr
2009/11/24 Daniel Farina <drfarina@gmail.com>:
> On Tue, Nov 24, 2009 at 2:10 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> Hello
>>
>> I thing, so this patch is maybe good idea. I am missing better
>> function specification. Specification by name isn't enough - we can
>> have a overloaded functions. This syntax doesn't allow to use explicit
>> cast - from my personal view, the syntax is ugly - with type
>> specification we don't need to keyword FUNCTION
>
> As long as things continue to support the INTERNAL-type behavior for
> extremely low overhead bulk transfers I am open to suggestions about
> how to enrich things...but how would I do so under this proposal?
>
using an INTERNAL type is wrong. It breaks design these functions for
usual PL. I don't see any reason, why it's necessary.
> I am especially fishing for suggestions in the direction of managing
> state for the function between rows though...I don't like how the
> current design seems to scream "use a global variable."
>
>> We have a fast copy statement - ok., we have a fast function ok, but
>> inside a function we have to call "slow" sql query. Personally What is
>> advantage?
>
> The implementation here uses a type 'internal' for performance. It
> doesn't even recompute the fcinfo because of the very particular
> circumstances of how the function is called. It doesn't do a memory
> copy of the argument buffer either, to the best of my knowledge. In
> the dblink patches you basically stream directly from the disk, format
> the COPY bytes, and shove it into a waiting COPY on another postgres
> node...there's almost no additional work in-between. All utilized
> time would be some combination of the normal COPY byte stream
> generation and libpq.
>
I understand and I dislike it. This design isn't general - or it is
far from using a function. It doesn't use complete FUNCAPI interface.
I thing so you need different semantic. You are not use a function.
You are use some like "stream object". This stream object can have a
input, output function, and parameters should be internal (I don't
thing, so internal could to carry any significant performance here) or
standard. Syntax should be similar to CREATE AGGREGATE.
then syntax should be:
COPY table TO streamname(parameters)
COPY table TO filestream('/tmp/foo.dta') ...
COPY table TO dblinkstream(connectionstring) ...
This design is only ideas. It's not important.
What is important - limited design. There are not possible to use PL
mainly untrusted PL. Using an internal type is simple hack.
Pavel
> This, of course, presumes that everyone who is interested in building
> on this is going to use some UDFs written in C...
>
>>
>> We need pipes like
>>
>> like COPY table TO foo(..) TO table
>>
>> foo() should be a transformation function, or real pipe function
>
> I've actually considered this pipe thing with a colleague while
> driving home from work...it occurred to us that it would be nice to
> have both pipes and tees (basically composition vs. mapping
> application of functions over the input) in some form. Not sure what
> an elegant way to express that is or how to control it. Since you can
> work around this by composing or applying functions on your own in
> another function, I'm not sure if that's as high priority for me
> personally.
>
> fdr
>
On Tue, Nov 24, 2009 at 4:37 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2009/11/24 Daniel Farina <drfarina@gmail.com>:
>> On Tue, Nov 24, 2009 at 2:10 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>> Hello
>>>
>>> I thing, so this patch is maybe good idea. I am missing better
>>> function specification. Specification by name isn't enough - we can
>>> have a overloaded functions. This syntax doesn't allow to use explicit
>>> cast - from my personal view, the syntax is ugly - with type
>>> specification we don't need to keyword FUNCTION
>>
>> As long as things continue to support the INTERNAL-type behavior for
>> extremely low overhead bulk transfers I am open to suggestions about
>> how to enrich things...but how would I do so under this proposal?
>>
>
> using an INTERNAL type is wrong. It breaks design these functions for
> usual PL. I don't see any reason, why it's necessary.
>
>> I am especially fishing for suggestions in the direction of managing
>> state for the function between rows though...I don't like how the
>> current design seems to scream "use a global variable."
>>
>>> We have a fast copy statement - ok., we have a fast function ok, but
>>> inside a function we have to call "slow" sql query. Personally What is
>>> advantage?
>>
>> The implementation here uses a type 'internal' for performance. It
>> doesn't even recompute the fcinfo because of the very particular
>> circumstances of how the function is called. It doesn't do a memory
>> copy of the argument buffer either, to the best of my knowledge. In
>> the dblink patches you basically stream directly from the disk, format
>> the COPY bytes, and shove it into a waiting COPY on another postgres
>> node...there's almost no additional work in-between. All utilized
>> time would be some combination of the normal COPY byte stream
>> generation and libpq.
>>
>
> I understand and I dislike it. This design isn't general - or it is
> far from using a function. It doesn't use complete FUNCAPI interface.
> I thing so you need different semantic. You are not use a function.
> You are use some like "stream object". This stream object can have a
> input, output function, and parameters should be internal (I don't
> thing, so internal could to carry any significant performance here) or
> standard. Syntax should be similar to CREATE AGGREGATE.
I think you might be right about this. At the time I was too shy to
add a DDL command for this hack, though. But what I did want is a
form of currying, and that's not easily accomplished in SQL without
extension...
> then syntax should be:
>
> COPY table TO streamname(parameters)
>
> COPY table TO filestream('/tmp/foo.dta') ...
> COPY table TO dblinkstream(connectionstring) ...
I like this one quite a bit...it's a bit like an aggregate, except the
initial condition can be set in a rather function-callish way.
But that does seem to require making a DDL command, which leaves a
nice green field. In particular, we could then make as many hooks,
flags, and options as we wanted, but sometimes there is a paradox of
choice...I just did not want to anticipate on Postgres being friendly
to a new DDL command when writing this the first time.
fdr
On Tue, 2009-11-24 at 03:48 -0800, Daniel Farina wrote: > On Tue, Nov 24, 2009 at 3:25 AM, Hannu Krosing <hannu@2ndquadrant.com> wrote: > > On Tue, 2009-11-24 at 02:56 -0800, Daniel Farina wrote: > >> On Tue, Nov 24, 2009 at 2:51 AM, Daniel Farina <drfarina@gmail.com> wrote: > >> > On Tue, Nov 24, 2009 at 2:50 AM, Hannu Krosing <hannu@2ndquadrant.com> wrote: > >> >> Can't you use existing aggregate function design ? > >> >> > >> >> CREATE AGGREGATE name ( input_data_type [ , ... ] ) ( > >> >> SFUNC = sfunc, > >> >> STYPE = state_data_type > >> >> [ , FINALFUNC = ffunc ] > >> >> [ , INITCOND = initial_condition ] > >> >> [ , SORTOP = sort_operator ] > >> >> ) > >> > > >> > Actually, yes. I just thought that this was an idea so crazy that no > >> > one would like it. > > > > seems kind of natural choice for me - in essence this is an aggregate > > function, aggregating over rows/tuples supplied to it. > > Okay, well, maybe that wasn't such a crazy idea after all... > > >> Oh, and the other elephant in the room: error handling. How to handle > >> error conditions...try/catch/finally type stuff. > > > > Same as current aggregates - either ignore the error, logi it and > > continue, or bail out > >[snip] > > Neither do ordinary funtions, we have no "ON ERROR DO ..." clause for > > function definitions > > It is assumed most functions do not have side effects outside the > database, so this is gotten rather for free. The driving use case for > this *is* side effects on other systems. I'm not sure if it's as easy > to use this justification here...normally rollbacks just take care of > all the error handling a function would want. Here I'm not so sure > that is as common a case. A cleaner solution for undoing external effects would be ON ROLLBACK trigger, or maybe even extension to BEGIN BEGIN WORK ON ROLLBACK RUN externalCleanupFunction(); ROLLBACK trigger could also be done as SET parameter inside a session, so it wont bloat/pollute system tables if changed often; > > > >> I did consider using > >> aggregates though, but somehow it felt to me like "I need at least a > >> three-tuple, why not fish around for any random bundling of three > >> functions..." > > > > Why do you need three ? > > I'm counting the aggregate prototype itself to refer to the bundle, > which I suppose would be more normally considered a two-tuple of > functions. This is a self-referential tuple, I suppose... > > >> After all, I would not want to actually call the nodeAgg stuff to > >> apply the function anyway...so it'd basically be abused as a > >> three-tuple of functions. > > > > Actually it would be best if it could use straight generic funtions, so > > you could do something like > > > > COPY stdin TO filterfunc(int) TO avg(int); > > Generic functions? Do you mean just scalar functions? Type. Actually I meant our existing aggregate functions. > That'd be > neat, but as I said previously, composition could just be wrapped into > a function of the user's choice. Also, what about use of > multi-function-apply? > > COPY stdin TO replicant1(datum) AND replicant2(datum); seems like a rare case, but you could use a wrapper func CREATE FUNCTION replicants_1_and_2(datum) AS replicant1(datum) replicant2(datum) > You could imagine all sorts of new 2PC evil. 2PC is evil enyway, at least when performance is concerned ;) > But again, one could > just write a little function to absorb the rows and dole them out > without bloating COPY syntax... > > I am in no way suggesting that syntax seriously or unseriously. > > > pass the file name in as an argument to SFUNC, open it on first call, > > ignore later (if it stays the same ;) > > So either you are going to pass it with every row and ignore it, That would be my preferred way, yes > or create a new initial aggregate state for each COPY TO FUNCTION third, more hackish way would to set it as INITCOND = '/file/name' :) > ...how are you going to get it passed to SFUNC? keep the file handle in the aggregate node - it is for keeping state, and file handle sure is part of state. -- Hannu Krosing http://www.2ndQuadrant.com PostgreSQL Scalability and Availability Services, Consulting and Training
2009/11/24 Daniel Farina <drfarina@gmail.com>:
> On Tue, Nov 24, 2009 at 4:37 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> 2009/11/24 Daniel Farina <drfarina@gmail.com>:
>>> On Tue, Nov 24, 2009 at 2:10 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>>> Hello
>>>>
>>>> I thing, so this patch is maybe good idea. I am missing better
>>>> function specification. Specification by name isn't enough - we can
>>>> have a overloaded functions. This syntax doesn't allow to use explicit
>>>> cast - from my personal view, the syntax is ugly - with type
>>>> specification we don't need to keyword FUNCTION
>>>
>>> As long as things continue to support the INTERNAL-type behavior for
>>> extremely low overhead bulk transfers I am open to suggestions about
>>> how to enrich things...but how would I do so under this proposal?
>>>
>>
>> using an INTERNAL type is wrong. It breaks design these functions for
>> usual PL. I don't see any reason, why it's necessary.
>>
>>> I am especially fishing for suggestions in the direction of managing
>>> state for the function between rows though...I don't like how the
>>> current design seems to scream "use a global variable."
>>>
>>>> We have a fast copy statement - ok., we have a fast function ok, but
>>>> inside a function we have to call "slow" sql query. Personally What is
>>>> advantage?
>>>
>>> The implementation here uses a type 'internal' for performance. It
>>> doesn't even recompute the fcinfo because of the very particular
>>> circumstances of how the function is called. It doesn't do a memory
>>> copy of the argument buffer either, to the best of my knowledge. In
>>> the dblink patches you basically stream directly from the disk, format
>>> the COPY bytes, and shove it into a waiting COPY on another postgres
>>> node...there's almost no additional work in-between. All utilized
>>> time would be some combination of the normal COPY byte stream
>>> generation and libpq.
>>>
>>
>> I understand and I dislike it. This design isn't general - or it is
>> far from using a function. It doesn't use complete FUNCAPI interface.
>> I thing so you need different semantic. You are not use a function.
>> You are use some like "stream object". This stream object can have a
>> input, output function, and parameters should be internal (I don't
>> thing, so internal could to carry any significant performance here) or
>> standard. Syntax should be similar to CREATE AGGREGATE.
>
> I think you might be right about this. At the time I was too shy to
> add a DDL command for this hack, though. But what I did want is a
> form of currying, and that's not easily accomplished in SQL without
> extension...
>
COPY is a PostgreSQL extension. If there are other related extensions - why not?
PostgreSQL has lot of database objects over SQL standard - see
fulltext implementation. I am not sure if STREAM is good keyword now.
It could be in collision with STREAM from streaming databases.
>> then syntax should be:
>>
>> COPY table TO streamname(parameters)
>>
>> COPY table TO filestream('/tmp/foo.dta') ...
>> COPY table TO dblinkstream(connectionstring) ...
>
> I like this one quite a bit...it's a bit like an aggregate, except the
> initial condition can be set in a rather function-callish way.
>
> But that does seem to require making a DDL command, which leaves a
> nice green field. In particular, we could then make as many hooks,
> flags, and options as we wanted, but sometimes there is a paradox of
> choice...I just did not want to anticipate on Postgres being friendly
> to a new DDL command when writing this the first time.
>
sure - nobody like too much changes in gram.y. But well designed
general feature with related SQL enhancing is more acceptable, then
fast simply hack. Don't be a hurry. This idea is good - but it needs:
a) good designed C API like:
initialise_functions(fcinfo) -- std fcinfo consument_process_tuple(fcinfo) -- gets standard row -- Datum
dvalues[] + Row description producent_process_tuple(fcinfo) -- returns standard row -- Datum
dvalues[] + Row description (look on SRF API) terminate_funnction(fcinfo)
I am sure, so this could be similar to AGGREGATE api
+ some samples to contrib
b) good designed PLPerlu and PLPythonu interface
+ some samples to documentation
Regards
Pavel Stehule
>
>
On Tue, 2009-11-24 at 05:00 -0800, Daniel Farina wrote:
> On Tue, Nov 24, 2009 at 4:37 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > then syntax should be:
> >
> > COPY table TO streamname(parameters)
> >
> > COPY table TO filestream('/tmp/foo.dta') ...
> > COPY table TO dblinkstream(connectionstring) ...
You probably meant
COPY table TO dblinkstream(connectionstring, table)
?
> I like this one quite a bit...it's a bit like an aggregate, except the
> initial condition can be set in a rather function-callish way.
>
> But that does seem to require making a DDL command, which leaves a
> nice green field.
not necessarily DDL, maybe just a "copystream" type and a set of
functions creating objects of that type.
if you make it a proper type with input and output function, then you
can probably use it in statements like this
COPY table TO (select stream::copystream from streams where id = 7);
COPY table TO 'file:/tmp/outfile':: copystream;
COPY table TO 'dblink::<connectstring>':: copystream;
> In particular, we could then make as many hooks,
> flags, and options as we wanted, but sometimes there is a paradox of
> choice...I just did not want to anticipate on Postgres being friendly
> to a new DDL command when writing this the first time.
fulltext lived for quite some time as set of types and functions before
it was glorified with its own DDL syntax.
It may be good to have the same approach here - do it as a set of types
and functions first, think about adding DDL once it has stabilised
enough
--
Hannu Krosing http://www.2ndQuadrant.com
PostgreSQL Scalability and Availability Services, Consulting and Training
On Mon, Nov 23, 2009 at 8:46 PM, Greg Smith <greg@2ndquadrant.com> wrote: > You know how people complain about how new contributors are treated here? > Throwing out comments like this, that come off as belittling to other > people's work, doesn't help. All I was suggesting was that Dan wasn't > developing this in complete isolation from the hackers community as Robert > had feared, as will be obvious when we get to: I still think it's better to have discussion on the mailing list than elsewhere. But we're doing that now, so, good. > As far as other past discussion here that might be relevant, this patch > includes a direct change to gram.y to support the new syntax. You've > already suggested before that it might be time to update COPY the same way > EXPLAIN and now VACUUM have been overhauled to provide a more flexible > options interface: > http://archives.postgresql.org/pgsql-hackers/2009-09/msg00616.php This > patch might be more fuel for that idea. FWIW, Tom already committed a patch by Emmanuel and myself that did this. ...Robert
2009/11/24 Hannu Krosing <hannu@2ndquadrant.com>:
> On Tue, 2009-11-24 at 05:00 -0800, Daniel Farina wrote:
>> On Tue, Nov 24, 2009 at 4:37 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>> > then syntax should be:
>> >
>> > COPY table TO streamname(parameters)
>> >
>> > COPY table TO filestream('/tmp/foo.dta') ...
>> > COPY table TO dblinkstream(connectionstring) ...
>
> You probably meant
>
> COPY table TO dblinkstream(connectionstring, table)
>
> ?
>
>> I like this one quite a bit...it's a bit like an aggregate, except the
>> initial condition can be set in a rather function-callish way.
>>
>> But that does seem to require making a DDL command, which leaves a
>> nice green field.
>
> not necessarily DDL, maybe just a "copystream" type and a set of
> functions creating objects of that type.
>
> if you make it a proper type with input and output function, then you
> can probably use it in statements like this
>
> COPY table TO (select stream::copystream from streams where id = 7);
>
> COPY table TO 'file:/tmp/outfile':: copystream;
>
> COPY table TO 'dblink::<connectstring>':: copystream;
it interesting - but still you have to have DDL for declaring stream.
It is analogous to function:
CREATE FUNCTION ....
SELECT 'foo'::regprocedure
but syntax COPY table TO copystream is good idea. I like it.
>
>> In particular, we could then make as many hooks,
>> flags, and options as we wanted, but sometimes there is a paradox of
>> choice...I just did not want to anticipate on Postgres being friendly
>> to a new DDL command when writing this the first time.
>
> fulltext lived for quite some time as set of types and functions before
> it was glorified with its own DDL syntax.
What is DDL? Wrapper for insert to system catalog.
so we can have table pg_catalog.copystream
and for first testing
CREATE OR REPLACE FUNCTION register_copystream(regproc, regproc, regproc ...)
if we will happy - than it is one day work for support statement
CREATE COPYSTREAM ( ...
Regards
Pavel Stehule
>
> It may be good to have the same approach here - do it as a set of types
> and functions first, think about adding DDL once it has stabilised
> enough
>
>
> --
> Hannu Krosing http://www.2ndQuadrant.com
> PostgreSQL Scalability and Availability
> Services, Consulting and Training
>
>
>
On Mon, Nov 23, 2009 at 9:37 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > > > Greg Smith wrote: >> >> I haven't heard anything from Andrew about ragged CVS import either. I >> think that ultimately those features are useful, but just exceed what the >> existing code could be hacked to handle cleanly. > > The patch is attached for your edification/amusement. I have backpatched it > to 8.4 for the client that needed it, and it's working just fine. I didn't > pursue it when it was clear that it was not going to be accepted. COPY > returning text[] would allow us to achieve the same thing, a bit more > verbosely, but it would be a lot more work to develop. FWIW, I've somewhat come around to this idea. But I might be the only one. ...Robert
On Tue, 2009-11-24 at 00:54 -0800, Daniel Farina wrote: > On Tue, Nov 24, 2009 at 12:29 AM, Hannu Krosing <hannu@krosing.net> wrote: > > COPY stdin TO udf(); > > If stdin becomes (is?) a legitimate source of records, then this patch > will Just Work. > STDIN is a source of bytes representing a set of records. Currently, the first argument to COPY is a source or destination of records; and the second argument is a source or destination of bytes representing a set of records. I think we want the first argument to remain a source or destination of real records with types; that is, a table or perhaps a function. And we want the second argument to remain a source or destination of bytes; that is, a file or perhaps a function (albeit not the same kind as the former function). > > COPY udf() FROM stdin; > > This is unaddressed, but I think it would be a good idea to consider > enabling this kind of thing prior to application. This makes much more sense, but it is a very different type of function from the original proposal (which basically accepts a buffer). I agree that it sounds useful and would be good for the sake of symmetry. One use case may be a degree of data cleaning. For instance, you could use a "looser" function definition, like udf(cstring, cstring, ...), where all COPY does is break up the records into fields, and the function can recover from type input failures using subtransactions. Binary mode could do a similar thing with bytea. However, I recommend that you don't try to generalize this as a data cleanup feature that can handle ragged input. That seems like a separate problem that will distract from the original use case. Regards,Jeff Davis
On Tue, 2009-11-24 at 14:39 +0100, Pavel Stehule wrote: > a) good designed C API like: > > initialise_functions(fcinfo) -- std fcinfo > consument_process_tuple(fcinfo) -- gets standard row -- Datum > dvalues[] + Row description > producent_process_tuple(fcinfo) -- returns standard row -- Datum > dvalues[] + Row description (look on SRF API) > terminate_funnction(fcinfo) > Don't you still need the functions to accept an argument of type internal? Otherwise, we lose the ability to copy a buffer to the dblink connection, which was the original motivation. Regards,Jeff Davis
Jeff Davis <pgsql@j-davis.com> writes:
> Don't you still need the functions to accept an argument of type
> internal? Otherwise, we lose the ability to copy a buffer to the dblink
> connection, which was the original motivation.
If you do that, then there is no possibility of ever using this feature
except with C-coded functions, which seems to me to remove most of
whatever use-case there was.
regards, tom lane
2009/11/25 Jeff Davis <pgsql@j-davis.com>: > On Tue, 2009-11-24 at 14:39 +0100, Pavel Stehule wrote: >> a) good designed C API like: >> >> initialise_functions(fcinfo) -- std fcinfo >> consument_process_tuple(fcinfo) -- gets standard row -- Datum >> dvalues[] + Row description >> producent_process_tuple(fcinfo) -- returns standard row -- Datum >> dvalues[] + Row description (look on SRF API) >> terminate_funnction(fcinfo) >> > > Don't you still need the functions to accept an argument of type > internal? Otherwise, we lose the ability to copy a buffer to the dblink > connection, which was the original motivation. > It depends on design. I don't thing so internal is necessary. It is just wrong design. Pavel > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
On Tue, Nov 24, 2009 at 8:45 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > It depends on design. I don't thing so internal is necessary. It is > just wrong design. Depends on how lean you want to be when doing large COPY...right now the cost is restricted to having to call a function pointer and a few branches. If you want to take SQL values, then the semantics of function calling over a large number of rows is probably notably more expensive, although I make no argument against the fact that the non-INTERNAL version would give a lot more people more utility. fdr
On Tue, 2009-11-24 at 23:44 -0500, Tom Lane wrote: > If you do that, then there is no possibility of ever using this feature > except with C-coded functions, which seems to me to remove most of > whatever use-case there was. It fits the use case involving dblink (or dblink-like modules). Maybe the patch's performance should be tested with and without copying the buffer, to see if we're losing anything significant. If we can do almost as well copying the data and passing that as a bytea value to the function, then I agree that would be better. I still don't see any reason to force it to be record by record though. If the point is to push data from a table into a remote table, why should the copied data be translated out of binary format into a record, and then back into binary form to send to the remote system? Currently, the second argument to copy is a source or destination of bytes, not records. So forcing it to deal with records is inconsistent. Regards,Jeff Davis
On Tue, Nov 24, 2009 at 9:13 PM, Jeff Davis <pgsql@j-davis.com> wrote: > > I still don't see any reason to force it to be record by record though. > If the point is to push data from a table into a remote table, why > should the copied data be translated out of binary format into a record, > and then back into binary form to send to the remote system? > > Currently, the second argument to copy is a source or destination of > bytes, not records. So forcing it to deal with records is inconsistent. You are correct. It so happens as an artifact of how COPY is written that things are delivered row-by-row, but at some fundamental level it does not matter were that not the case... fdr
2009/11/25 Daniel Farina <drfarina@gmail.com>: > On Tue, Nov 24, 2009 at 8:45 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> It depends on design. I don't thing so internal is necessary. It is >> just wrong design. > > Depends on how lean you want to be when doing large COPY...right now > the cost is restricted to having to call a function pointer and a few > branches. If you want to take SQL values, then the semantics of > function calling over a large number of rows is probably notably more > expensive, although I make no argument against the fact that the > non-INTERNAL version would give a lot more people more utility. I believe so using an "internal" minimalize necessary changes in COPY implementation. Using a funcapi needs more work inside COPY - you have to take some functionality from COPY to stream functions. Probably the most slow operations is parsing - calling a input functions. This is called once every where. Second slow operation is reading from network - it is same. So I don't see too much reasons, why non internal implementation have to be significant slower than your actual implementation. I am sure, so it needs more work. What is significant - when I better join COPY and some streaming function, then I don't need use tuplestore - or SRF functions. COPY reads data directly. > > fdr >
On Tue, Nov 24, 2009 at 9:35 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > 2009/11/25 Daniel Farina <drfarina@gmail.com>: >> On Tue, Nov 24, 2009 at 8:45 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >>> It depends on design. I don't thing so internal is necessary. It is >>> just wrong design. >> >> Depends on how lean you want to be when doing large COPY...right now >> the cost is restricted to having to call a function pointer and a few >> branches. If you want to take SQL values, then the semantics of >> function calling over a large number of rows is probably notably more >> expensive, although I make no argument against the fact that the >> non-INTERNAL version would give a lot more people more utility. > > I believe so using an "internal" minimalize necessary changes in COPY > implementation. Using a funcapi needs more work inside COPY - you > have to take some functionality from COPY to stream functions. > Probably the most slow operations is parsing - calling a input > functions. This is called once every where. Second slow operation is > reading from network - it is same. So I don't see too much reasons, > why non internal implementation have to be significant slower than > your actual implementation. I am sure, so it needs more work. You are probably right. We could try coercing to bytea and back out to bytes, although it seems like a superfluous cost to force *everyone* to pay just to get the same bytes to a network buffer. fdr
2009/11/25 Daniel Farina <drfarina@gmail.com>: > On Tue, Nov 24, 2009 at 9:35 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> 2009/11/25 Daniel Farina <drfarina@gmail.com>: >>> On Tue, Nov 24, 2009 at 8:45 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >>>> It depends on design. I don't thing so internal is necessary. It is >>>> just wrong design. >>> >>> Depends on how lean you want to be when doing large COPY...right now >>> the cost is restricted to having to call a function pointer and a few >>> branches. If you want to take SQL values, then the semantics of >>> function calling over a large number of rows is probably notably more >>> expensive, although I make no argument against the fact that the >>> non-INTERNAL version would give a lot more people more utility. >> >> I believe so using an "internal" minimalize necessary changes in COPY >> implementation. Using a funcapi needs more work inside COPY - you >> have to take some functionality from COPY to stream functions. >> Probably the most slow operations is parsing - calling a input >> functions. This is called once every where. Second slow operation is >> reading from network - it is same. So I don't see too much reasons, >> why non internal implementation have to be significant slower than >> your actual implementation. I am sure, so it needs more work. > "internal" is important (for performance) for aggregation function - where is protection under repeated alloc/free memory - it work well and it is +/- ugly hack. We cannot do some things well - simply there are missing some support. Nobody calculated with very large string, array concatenation in design time - It is reason, why I am against to using it. > You are probably right. We could try coercing to bytea and back out > to bytes, although it seems like a superfluous cost to force > *everyone* to pay just to get the same bytes to a network buffer. > I am not sure if this is good analogy. Only "filestream" or "network" stream is stream of bytes. From any sophisticated stream I am taking tuples - database stream, SOAP stream. I agree, so dblink could to returns binary compatible records - but it is one special and exclusive case. Sure, important and have to calculated. Still I am thinking so dblink to postgres is other hack and should be replaced). > fdr >
On Tue, 2009-11-24 at 21:42 -0800, Daniel Farina wrote: > You are probably right. We could try coercing to bytea and back out > to bytes, although it seems like a superfluous cost to force > *everyone* to pay just to get the same bytes to a network buffer. Well, I suppose only performance will tell. Copying a buffer is sure to be faster than invoking all of the type input/output functions, or even send/recv, so perhaps it's not a huge penalty. My disagreement with the row-by-row approach is more semantics than performance. COPY translates records to bytes and vice-versa, and your original patch maintains those semantics. Regards,Jeff Davis
On Wed, 2009-11-25 at 06:35 +0100, Pavel Stehule wrote: > I believe so using an "internal" minimalize necessary changes in COPY > implementation. Using a funcapi needs more work inside COPY - you > have to take some functionality from COPY to stream functions. > Probably the most slow operations is parsing - calling a input > functions. This is called once every where. Second slow operation is > reading from network - it is same. So I don't see too much reasons, > why non internal implementation have to be significant slower than > your actual implementation. I am sure, so it needs more work. I apologize, but I don't understand what you're saying. Can you please restate with some examples? It seems like you're advocating that we move records from a table into a function using COPY. But that's not what COPY normally does: COPY normally translates records to bytes or bytes to records. Moving records from a table to a function can be done with: SELECT myfunc(mytable) FROM mytable; already. The only problem is if you want initialization/destruction. But I'm not convinced that COPY is the best tool to provide that. Moving records from a function to a table can be done with: INSERT INTO mytable SELECT * FROM myfunc(); And that already works fine. So what use case are you concerned about? Regards,Jeff Davis
2009/11/25 Jeff Davis <pgsql@j-davis.com>: > On Tue, 2009-11-24 at 21:42 -0800, Daniel Farina wrote: >> You are probably right. We could try coercing to bytea and back out >> to bytes, although it seems like a superfluous cost to force >> *everyone* to pay just to get the same bytes to a network buffer. > > Well, I suppose only performance will tell. Copying a buffer is sure to > be faster than invoking all of the type input/output functions, or even > send/recv, so perhaps it's not a huge penalty. > > My disagreement with the row-by-row approach is more semantics than > performance. COPY translates records to bytes and vice-versa, and your > original patch maintains those semantics. uff, really COPY CSV ? Pavel > > Regards, > Jeff Davis > >
On Tue, Nov 24, 2009 at 10:23 PM, Jeff Davis <pgsql@j-davis.com> wrote: > On Wed, 2009-11-25 at 06:35 +0100, Pavel Stehule wrote: >> I believe so using an "internal" minimalize necessary changes in COPY >> implementation. Using a funcapi needs more work inside COPY - you >> have to take some functionality from COPY to stream functions. >> Probably the most slow operations is parsing - calling a input >> functions. This is called once every where. Second slow operation is >> reading from network - it is same. So I don't see too much reasons, >> why non internal implementation have to be significant slower than >> your actual implementation. I am sure, so it needs more work. > > I apologize, but I don't understand what you're saying. Can you please > restate with some examples? > > It seems like you're advocating that we move records from a table into a > function using COPY. But that's not what COPY normally does: COPY > normally translates records to bytes or bytes to records. Perhaps what we want is pluggable transformation functions that can format the row any way that is desired, with the current behavior being some default. Putting COPY TO FUNCTION as submitted aside, what about something like this: COPY foo TO '/tmp/foo' USING postgres_builtin_formatter(csv = true); This is something completely different than what was submitted, so in some aspect: COPY foo TO FUNCTION dblink_send_row USING postgres_builtin_formatter(binary = true); Would compose the two features... (Again, very, very far from a real syntax suggestion) fdr
2009/11/25 Jeff Davis <pgsql@j-davis.com>: > On Wed, 2009-11-25 at 06:35 +0100, Pavel Stehule wrote: >> I believe so using an "internal" minimalize necessary changes in COPY >> implementation. Using a funcapi needs more work inside COPY - you >> have to take some functionality from COPY to stream functions. >> Probably the most slow operations is parsing - calling a input >> functions. This is called once every where. Second slow operation is >> reading from network - it is same. So I don't see too much reasons, >> why non internal implementation have to be significant slower than >> your actual implementation. I am sure, so it needs more work. > > I apologize, but I don't understand what you're saying. Can you please > restate with some examples? > > It seems like you're advocating that we move records from a table into a > function using COPY. But that's not what COPY normally does: COPY > normally translates records to bytes or bytes to records. > > Moving records from a table to a function can be done with: > SELECT myfunc(mytable) FROM mytable; > already. The only problem is if you want initialization/destruction. But > I'm not convinced that COPY is the best tool to provide that. > > Moving records from a function to a table can be done with: > INSERT INTO mytable SELECT * FROM myfunc(); > And that already works fine. It works, but COPY FROM myfunc() should be significantly faster. You can skip tuple store. Pavel > > So what use case are you concerned about? > > Regards, > Jeff Davis > >
2009/11/25 Daniel Farina <drfarina@gmail.com>: > On Tue, Nov 24, 2009 at 10:23 PM, Jeff Davis <pgsql@j-davis.com> wrote: >> On Wed, 2009-11-25 at 06:35 +0100, Pavel Stehule wrote: >>> I believe so using an "internal" minimalize necessary changes in COPY >>> implementation. Using a funcapi needs more work inside COPY - you >>> have to take some functionality from COPY to stream functions. >>> Probably the most slow operations is parsing - calling a input >>> functions. This is called once every where. Second slow operation is >>> reading from network - it is same. So I don't see too much reasons, >>> why non internal implementation have to be significant slower than >>> your actual implementation. I am sure, so it needs more work. >> >> I apologize, but I don't understand what you're saying. Can you please >> restate with some examples? >> >> It seems like you're advocating that we move records from a table into a >> function using COPY. But that's not what COPY normally does: COPY >> normally translates records to bytes or bytes to records. > > Perhaps what we want is pluggable transformation functions that can > format the row any way that is desired, with the current behavior > being some default. Putting COPY TO FUNCTION as submitted aside, what > about something like this: > > COPY foo TO '/tmp/foo' USING postgres_builtin_formatter(csv = true); > > This is something completely different than what was submitted, so in > some aspect: > > COPY foo TO FUNCTION dblink_send_row USING > postgres_builtin_formatter(binary = true); > > Would compose the two features... > yes - it is two features - and should be solved independently Pavel > (Again, very, very far from a real syntax suggestion) > > fdr > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
2009/11/25 Pavel Stehule <pavel.stehule@gmail.com>: > 2009/11/25 Daniel Farina <drfarina@gmail.com>: >> On Tue, Nov 24, 2009 at 10:23 PM, Jeff Davis <pgsql@j-davis.com> wrote: >>> On Wed, 2009-11-25 at 06:35 +0100, Pavel Stehule wrote: >>>> I believe so using an "internal" minimalize necessary changes in COPY >>>> implementation. Using a funcapi needs more work inside COPY - you >>>> have to take some functionality from COPY to stream functions. >>>> Probably the most slow operations is parsing - calling a input >>>> functions. This is called once every where. Second slow operation is >>>> reading from network - it is same. So I don't see too much reasons, >>>> why non internal implementation have to be significant slower than >>>> your actual implementation. I am sure, so it needs more work. >>> >>> I apologize, but I don't understand what you're saying. Can you please >>> restate with some examples? >>> >>> It seems like you're advocating that we move records from a table into a >>> function using COPY. But that's not what COPY normally does: COPY >>> normally translates records to bytes or bytes to records. >> >> Perhaps what we want is pluggable transformation functions that can >> format the row any way that is desired, with the current behavior >> being some default. Putting COPY TO FUNCTION as submitted aside, what >> about something like this: >> >> COPY foo TO '/tmp/foo' USING postgres_builtin_formatter(csv = true); >> >> This is something completely different than what was submitted, so in >> some aspect: >> >> COPY foo TO FUNCTION dblink_send_row USING >> postgres_builtin_formatter(binary = true); >> >> Would compose the two features... >> > > yes - it is two features - and should be solved independently it and it is not (some thinking) - smarter streams should to accept/returns tuples. Formating function has sense for text output - there are input/output formating (text based/bytea based) functions. I see one possible problem - when formater functions will be row based - then you cannot generate some prolog and epilog part of file - (xml). Pavel > > Pavel > >> (Again, very, very far from a real syntax suggestion) >> >> fdr >> >> -- >> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgsql-hackers >> >
On Wed, 2009-11-25 at 07:31 +0100, Pavel Stehule wrote: > > My disagreement with the row-by-row approach is more semantics than > > performance. COPY translates records to bytes and vice-versa, and your > > original patch maintains those semantics. > > uff, really > > COPY CSV ? CSV is like text or binary: just another format to _represent_ records as a sequence of bytes. A CSV file is not a set of postgresql records until COPY interprets it as such. Regards,Jeff Davis
On Wed, 2009-11-25 at 07:36 +0100, Pavel Stehule wrote: > > Moving records from a function to a table can be done with: > > INSERT INTO mytable SELECT * FROM myfunc(); > > And that already works fine. > > It works, but COPY FROM myfunc() should be significantly faster. You > can skip tuple store. If SRFs use a tuplestore in that situation, it sounds like that should be fixed. Why do we need to provide alternate syntax involving COPY? Regards,Jeff Davis
2009/11/25 Jeff Davis <pgsql@j-davis.com>: > On Wed, 2009-11-25 at 07:36 +0100, Pavel Stehule wrote: >> > Moving records from a function to a table can be done with: >> > INSERT INTO mytable SELECT * FROM myfunc(); >> > And that already works fine. >> >> It works, but COPY FROM myfunc() should be significantly faster. You >> can skip tuple store. > > If SRFs use a tuplestore in that situation, it sounds like that should > be fixed. Why do we need to provide alternate syntax involving COPY? It isn't problem of SRF function design. It allow both mode - row and tuplestor. This is problem of INSERT statement, resp. INSERT INTO SELECT implementation. Regards Pavel > > Regards, > Jeff Davis > >
On Wed, 2009-11-25 at 09:23 +0100, Pavel Stehule wrote: > > If SRFs use a tuplestore in that situation, it sounds like that should > > be fixed. Why do we need to provide alternate syntax involving COPY? > > It isn't problem of SRF function design. It allow both mode - row and > tuplestor. select * from generate_series(1,1000000000) limit 1; That statement takes a long time, which indicates to me that it's materializing the result of the SRF. And there's no insert there. > This is problem of INSERT statement, resp. INSERT INTO > SELECT implementation. If "tmp" is a new table, and "zero" is a table with a million zeros in it, then: insert into tmp select 1/i from zero; fails instantly. That tells me that it's not materializing the result of the select; rather, it's feeding the rows in one at a time. Can show me in more detail what you mean? I'm having difficulty understanding your short replies. Regards,Jeff Davis
On Tue, 2009-11-24 at 21:13 -0800, Jeff Davis wrote: > On Tue, 2009-11-24 at 23:44 -0500, Tom Lane wrote: > > If you do that, then there is no possibility of ever using this feature > > except with C-coded functions, which seems to me to remove most of > > whatever use-case there was. > > It fits the use case involving dblink (or dblink-like modules). > > Maybe the patch's performance should be tested with and without copying > the buffer, to see if we're losing anything significant. If we can do > almost as well copying the data and passing that as a bytea value to the > function, then I agree that would be better. I'd make this dependent on funtion signature- if it takes bytea or text, then call it with (binary) rows- if it takes rowtype(of some hypothetic table), then resolve rows to this rowtype > I still don't see any reason to force it to be record by record though. > If the point is to push data from a table into a remote table, why > should the copied data be translated out of binary format into a record, > and then back into binary form to send to the remote system? > > Currently, the second argument to copy is a source or destination of > bytes, not records. So forcing it to deal with records is inconsistent. > > Regards, > Jeff Davis > -- Hannu Krosing http://www.2ndQuadrant.com PostgreSQL Scalability and Availability Services, Consulting and Training
2009/11/25 Jeff Davis <pgsql@j-davis.com>: > On Wed, 2009-11-25 at 09:23 +0100, Pavel Stehule wrote: >> > If SRFs use a tuplestore in that situation, it sounds like that should >> > be fixed. Why do we need to provide alternate syntax involving COPY? >> >> It isn't problem of SRF function design. It allow both mode - row and >> tuplestor. > > select * from generate_series(1,1000000000) limit 1; > > That statement takes a long time, which indicates to me that it's > materializing the result of the SRF. And there's no insert there. This is missing optimalisation. If I understand - PostgreSQL wait for complete result set - so in this case materialisation is necessary. In your query pg do materialisation too early. postgres=# select * from generate_series(1,100000) limit 1;generate_series ───────────────── 1 (1 row) Time: 59,540 ms postgres=# select generate_series(1,100000) limit 1;generate_series ───────────────── 1 (1 row) Time: 1,107 ms But usually we can process all rows from SRF function - so problem with LIMIT isn't significant. I am testing: 1. postgres=# select count(*) from generate_series(1,1000000); count ─────────1000000 (1 row) Time: 930,720 ms 2. postgres=# select count(*) from (select generate_series(1,1000000)) x; count ─────────1000000 (1 row) Time: 276,511 ms 2. is significantly faster then 1 (there are not SRF materialisation) postgres=# create table foo(a integer); CREATE TABLE postgres=# insert into foo select generate_series(1,1000000); INSERT 0 1000000 Time: 4274,869 ms postgres=# insert into foo select * from generate_series(1,1000000); INSERT 0 1000000 Time: 4814,756 ms postgres=# copy foo to '/tmp/xxx'; COPY 1000000 Time: 1033,078 ms postgres=# set synchronous_commit to off; SET postgres=# copy foo from '/tmp/xxx'; COPY 1000000 Time: 2749,277 ms postgres=# insert into foo select generate_series(1,1000000); INSERT 0 1000000 Time: 3948,734 ms generate_function is fast and simple - but still COPY is about 30% faster > >> This is problem of INSERT statement, resp. INSERT INTO >> SELECT implementation. > > If "tmp" is a new table, and "zero" is a table with a million zeros in > it, then: > insert into tmp select 1/i from zero; > fails instantly. That tells me that it's not materializing the result of > the select; rather, it's feeding the rows in one at a time. > I thing, so materialisation is every time, when you use any SQL statement without cursor. > Can show me in more detail what you mean? I'm having difficulty > understanding your short replies. I thing, so COPY tab from/to fce() should be used for very large import export, where INSERT SELECT needs minimally one materialisation. p.s. I am sorry - I am not native speaker - so I am speaking in short replies. Pavel > > Regards, > Jeff Davis > >
On Tue, Nov 24, 2009 at 10:39 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > yes - it is two features - and should be solved independently There are some common problems though. I was thinking about this with some mind towards my existing mental model of thinking of specifying some parameters up-front and getting a stream of records or bytes (depending on what feature you are referring to) as a form of currying, combined with the not-complete revulsion as to using aggregates as a base for such functionality.... What if we extended aggregates to support a function as an initial condition which could be called with parameters when initializing the aggregate? If you squint at it just right, the current form is that of a value/constant -- effectively the zero-parameter function. Here's a gist of what I want to realize: SELECT (avg())(column_name) FROM ... This is a vanilla average. That's not very interesting since avg only has one default initial value. However, at Truviso we have encountered a real case where we wanted SUM to be initialized to "0" instead of "NULL". I had to create a new aggregate with that as an initial condition, which is fine because we only needed one extra standard behavior. But perhaps instead it could have been written this way: SELECT (sum(0))(column_name) FROM ... That way people could get 'zero' rather than NULL when their query yields no rows. You could also imagine some code out there that may have a running-sum of sorts, and may want to seed SUM to some non-zero, non-NULL initial value as set by the application. At that point we may be able to abuse the aggregate infrastructure to doing what we want in the case of these COPY extensions more easily... fdr
Jeff Davis <pgsql@j-davis.com> writes:
> On Wed, 2009-11-25 at 09:23 +0100, Pavel Stehule wrote:
>>> If SRFs use a tuplestore in that situation, it sounds like that should
>>> be fixed. Why do we need to provide alternate syntax involving COPY?
>>
>> It isn't problem of SRF function design. It allow both mode - row and
>> tuplestor.
> select * from generate_series(1,1000000000) limit 1;
> That statement takes a long time, which indicates to me that it's
> materializing the result of the SRF.
Yeah. This is certainly fixable if someone wants to do the legwork of
refactoring ExecMakeTableFunctionResult(). It was done that way
originally just to keep down the complexity of introducing the
function-as-table-source feature at all.
regards, tom lane
On Wed, 2009-11-25 at 11:32 +0100, Pavel Stehule wrote: > 1. > postgres=# select count(*) from generate_series(1,1000000); > count > ───────── > 1000000 > (1 row) > > Time: 930,720 ms > > 2. > postgres=# select count(*) from (select generate_series(1,1000000)) x; > count > ───────── > 1000000 > (1 row) > > Time: 276,511 ms > > 2. is significantly faster then 1 (there are not SRF materialisation) I think case #1 can be fixed. > generate_function is fast and simple - but still COPY is about 30% faster My quick tests are not consistent enough, so I will have to try with more data. The times look similar to me so far. If there is a difference, I wonder what it is? > I thing, so materialisation is every time, when you use any SQL > statement without cursor. I don't think that is true. Here's an expanded version of my previous example: create table zero(i int); create table tmp(j int); insert into zero select 0 from generate_series(1,1000000); -- all 0 insert into tmp select 1/i from zero; -- error immediately, doesn't wait The error would take longer if it materialized the table "zero". But instead, it passes the first tuple to the function for "/" before the other tuples are read, and gets an error immediately. So no materialization. I worry that we're getting further away from the original problem. Let's allow functions to get the bytes of data from a COPY, like the original proposal. I am not sure COPY is the best mechanism to move records around when INSERT ... SELECT already does that. Regards,Jeff Davis
2009/11/25 Jeff Davis <pgsql@j-davis.com>: > On Wed, 2009-11-25 at 11:32 +0100, Pavel Stehule wrote: >> 1. >> postgres=# select count(*) from generate_series(1,1000000); >> count >> ───────── >> 1000000 >> (1 row) >> >> Time: 930,720 ms >> >> 2. >> postgres=# select count(*) from (select generate_series(1,1000000)) x; >> count >> ───────── >> 1000000 >> (1 row) >> >> Time: 276,511 ms >> >> 2. is significantly faster then 1 (there are not SRF materialisation) > > I think case #1 can be fixed. > >> generate_function is fast and simple - but still COPY is about 30% faster > > My quick tests are not consistent enough, so I will have to try with > more data. The times look similar to me so far. > > If there is a difference, I wonder what it is? > >> I thing, so materialisation is every time, when you use any SQL >> statement without cursor. > > I don't think that is true. Here's an expanded version of my previous > example: > > create table zero(i int); > create table tmp(j int); > insert into zero select 0 from generate_series(1,1000000); -- all 0 > insert into tmp select 1/i from zero; -- error immediately, doesn't wait > > The error would take longer if it materialized the table "zero". But > instead, it passes the first tuple to the function for "/" before the > other tuples are read, and gets an error immediately. So no > materialization. this show nothing. It working like: 1. EXECUTE SELECT 0 FROM generate_series(1,...); 2. STORE RESULT TO TABLE zero; 3. EXECUTE SELECT 1/i FROM zero; 4. STORE RESULT TO TABLE tmp; Problem is in seq execution. Result is stored to destination after execution - so any materialisation is necessary, > > I worry that we're getting further away from the original problem. Let's > allow functions to get the bytes of data from a COPY, like the original > proposal. I am not sure COPY is the best mechanism to move records > around when INSERT ... SELECT already does that. > In one single case hack I prefer using any hook and feature stored contrib. I don't see a general using for this feature. Regards Pavel Stehule > Regards, > Jeff Davis > >
On Wed, November 25, 2009 3:56 pm, Jeff Davis wrote: > > I worry that we're getting further away from the original problem. Let's > allow functions to get the bytes of data from a COPY, like the original > proposal. I am not sure COPY is the best mechanism to move records > around when INSERT ... SELECT already does that. > I am not at all sure I think that's a good idea, though. We have pg_read_file() for getting raw bytes from files. Building that into COPY does not strike me as a good fit. cheers andrew
On Wed, Nov 25, 2009 at 9:35 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > On Wed, November 25, 2009 3:56 pm, Jeff Davis wrote: >> >> I worry that we're getting further away from the original problem. Let's >> allow functions to get the bytes of data from a COPY, like the original >> proposal. I am not sure COPY is the best mechanism to move records >> around when INSERT ... SELECT already does that. >> > > > I am not at all sure I think that's a good idea, though. We have > pg_read_file() for getting raw bytes from files. Building that into COPY > does not strike me as a good fit. I think we speak of the opposite direction... fdr
On Thu, 2009-11-26 at 05:01 +0100, Pavel Stehule wrote: > It working like: > > 1. EXECUTE SELECT 0 FROM generate_series(1,...); > 2. STORE RESULT TO TABLE zero; > 3. EXECUTE SELECT 1/i FROM zero; > 4. STORE RESULT TO TABLE tmp; > > Problem is in seq execution. Result is stored to destination after > execution - so any materialisation is necessary, > My example showed that steps 3 and 4 are not executed sequentially, but are executed together. If 3 was executed entirely before 4, then the statement: insert into tmp select 1/i from zero; would have to read the whole table "zero" before an error is encountered. However, the statement errors immediately, showing that steps 3 and 4 are pipelined. Regards,Jeff Davis
On Thu, 2009-11-26 at 00:35 -0500, Andrew Dunstan wrote: > On Wed, November 25, 2009 3:56 pm, Jeff Davis wrote: > > > > I worry that we're getting further away from the original problem. Let's > > allow functions to get the bytes of data from a COPY, like the original > > proposal. I am not sure COPY is the best mechanism to move records > > around when INSERT ... SELECT already does that. > > > > > I am not at all sure I think that's a good idea, though. We have > pg_read_file() for getting raw bytes from files. Building that into COPY > does not strike me as a good fit. I think we're in agreement. All I mean is that the second argument to COPY should produce/consume bytes and not records. I'm not discussing the internal implementation at all, only semantics. In other words, STDIN is not a source of records, it's a source of bytes; and likewise for STDOUT. Regards,Jeff Davis
On Thu, November 26, 2009 2:22 am, Jeff Davis wrote: > On Thu, 2009-11-26 at 00:35 -0500, Andrew Dunstan wrote: >> On Wed, November 25, 2009 3:56 pm, Jeff Davis wrote: >> > >> > I worry that we're getting further away from the original problem. >> Let's >> > allow functions to get the bytes of data from a COPY, like the >> original >> > proposal. I am not sure COPY is the best mechanism to move records >> > around when INSERT ... SELECT already does that. >> > >> >> >> I am not at all sure I think that's a good idea, though. We have >> pg_read_file() for getting raw bytes from files. Building that into COPY >> does not strike me as a good fit. > > I think we're in agreement. All I mean is that the second argument to > COPY should produce/consume bytes and not records. I'm not discussing > the internal implementation at all, only semantics. > > In other words, STDIN is not a source of records, it's a source of > bytes; and likewise for STDOUT. > Hmm. I can just imagine wanting to do that as a way of running COPY over dblink. Are there other use cases? cheers andrew
On Thu, November 26, 2009 2:22 am, Jeff Davis wrote: > On Thu, 2009-11-26 at 00:35 -0500, Andrew Dunstan wrote: >> On Wed, November 25, 2009 3:56 pm, Jeff Davis wrote: >> > >> > I worry that we're getting further away from the original problem. >> Let's >> > allow functions to get the bytes of data from a COPY, like the >> original >> > proposal. I am not sure COPY is the best mechanism to move records >> > around when INSERT ... SELECT already does that. >> > >> >> >> I am not at all sure I think that's a good idea, though. We have >> pg_read_file() for getting raw bytes from files. Building that into COPY >> does not strike me as a good fit. > > I think we're in agreement. All I mean is that the second argument to > COPY should produce/consume bytes and not records. I'm not discussing > the internal implementation at all, only semantics. > > In other words, STDIN is not a source of records, it's a source of > bytes; and likewise for STDOUT. > Hmm. I can just imagine wanting to do that as a way of running COPY over dblink. Are there other use cases? cheers andrew
2009/11/26 Jeff Davis <pgsql@j-davis.com>: > On Thu, 2009-11-26 at 05:01 +0100, Pavel Stehule wrote: >> It working like: >> >> 1. EXECUTE SELECT 0 FROM generate_series(1,...); >> 2. STORE RESULT TO TABLE zero; >> 3. EXECUTE SELECT 1/i FROM zero; >> 4. STORE RESULT TO TABLE tmp; >> >> Problem is in seq execution. Result is stored to destination after >> execution - so any materialisation is necessary, >> > > My example showed that steps 3 and 4 are not executed sequentially, but > are executed together. If 3 was executed entirely before 4, then the > statement: > insert into tmp select 1/i from zero; > would have to read the whole table "zero" before an error is > encountered. you have a true. I checked it with functions in plpgsql and before trigger postgres=# create or replace function generator() returns setof int as $$begin raise notice 'generator start'; for i in 1..10 loop raise notice 'generator %', i; return next i; end loop; raise notice 'generator end'; return; end$$ language plpgsql; CREATE FUNCTION postgres=# create or replace function rowfce(int) returns int as $$begin raise notice 'rowfce %i', $1; return $1 + 1; end; $$ language plpgsql; CREATE FUNCTION postgres=# create function trgbody() returns trigger as $$begin raise notice 'trgbody %', new; return new; end;$$ language plpgsql; CREATE FUNCTION postgres=# create trigger xxx before insert on dest for each row execute procedure trgbody(); CREATE TRIGGER then I checked postgres=# insert into dest select rowfce(i) from generator() g(i); NOTICE: generator start NOTICE: generator 1 NOTICE: generator 2 NOTICE: generator 3 NOTICE: generator 4 NOTICE: generator 5 NOTICE: generator 6 NOTICE: generator 7 NOTICE: generator 8 NOTICE: generator 9 NOTICE: generator 10 NOTICE: generator end NOTICE: rowfce 1i NOTICE: trgbody (2) NOTICE: rowfce 2i NOTICE: trgbody (3) NOTICE: rowfce 3i NOTICE: trgbody (4) NOTICE: rowfce 4i NOTICE: trgbody (5) NOTICE: rowfce 5i NOTICE: trgbody (6) NOTICE: rowfce 6i NOTICE: trgbody (7) NOTICE: rowfce 7i NOTICE: trgbody (8) NOTICE: rowfce 8i NOTICE: trgbody (9) NOTICE: rowfce 9i NOTICE: trgbody (10) NOTICE: rowfce 10i NOTICE: trgbody (11) so INSERT INTO SELECT works well. Problem is in func scan implementation. Regards Pavel Stehule > > However, the statement errors immediately, showing that steps 3 and 4 > are pipelined. > > Regards, > Jeff Davis > >
On Thu, Nov 26, 2009 at 10:11:12AM -0500, Andrew Dunstan wrote: > On Thu, November 26, 2009 2:22 am, Jeff Davis wrote: > > On Thu, 2009-11-26 at 00:35 -0500, Andrew Dunstan wrote: > >> On Wed, November 25, 2009 3:56 pm, Jeff Davis wrote: > >> > > >> > I worry that we're getting further away from the original problem. > >> Let's > >> > allow functions to get the bytes of data from a COPY, like the > >> original > >> > proposal. I am not sure COPY is the best mechanism to move records > >> > around when INSERT ... SELECT already does that. > >> > > >> > >> > >> I am not at all sure I think that's a good idea, though. We have > >> pg_read_file() for getting raw bytes from files. Building that into COPY > >> does not strike me as a good fit. > > > > I think we're in agreement. All I mean is that the second argument to > > COPY should produce/consume bytes and not records. I'm not discussing > > the internal implementation at all, only semantics. > > > > In other words, STDIN is not a source of records, it's a source of > > bytes; and likewise for STDOUT. > > Hmm. I can just imagine wanting to do that as a way of running COPY over > dblink. Are there other use cases? It'd be nice to make this available to the next revision of DBI-Link and it'll be pretty handy for our SQL/MED whenever that happens. 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 iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Thu, Nov 26, 2009 at 6:13 PM, David Fetter <david@fetter.org> wrote: > It'd be nice to make this available to the next revision of DBI-Link > and it'll be pretty handy for our SQL/MED whenever that happens. Okay, so this thread sort of wandered into how we could refactor other elements of COPY. Do we have a good sense on what we should do to the current patch (or at least the idea represented by it) to get it into a committable state within finite time? I think adding a bytea and/or text mode is once such improvement...I am still reluctant to give up on INTERNAL because the string buffer passed in the INTERNAL scenario is ideal for C programmers -- the interface is even simpler than dealing with varlena types. But I agree that auxiliary modes should exist to enable easier hacking. The thorniest issue in my mind is how state can be initialized retained and/or modified between calls to the bytestream-acceptance function. Arguably it is already in a state where it is no worse than dblink, which itself has a global hash table to manage state. Also, if you look carefully at the dblink test suite I submitted, you'll see an interesting trick: one can COPY from multiple sources consecutively to a single COPY on a remote node when in text mode (binary mode has a header that cannot be so neatly catenated). This is something that's pretty hard to enable with any automatic startup-work-cleanup approach. fdr
On Tue, 2009-11-24 at 22:13 -0800, Jeff Davis wrote: > My disagreement with the row-by-row approach is more semantics than > performance. COPY translates records to bytes and vice-versa, and your > original patch maintains those semantics. The bytes <-> records conversion is a costly one. Anything we can do to avoid that in either direction will be worth it. I would regard performance as being part/most of the reason to support this. -- Simon Riggs www.2ndQuadrant.com
On Fri, 2009-11-27 at 13:39 +0000, Simon Riggs wrote: > On Tue, 2009-11-24 at 22:13 -0800, Jeff Davis wrote: > > > My disagreement with the row-by-row approach is more semantics than > > performance. COPY translates records to bytes and vice-versa, and your > > original patch maintains those semantics. > > The bytes <-> records conversion is a costly one. Anything we can do to > avoid that in either direction will be worth it. I would regard > performance as being part/most of the reason to support this. > Right. I was responding to an idea that copy support sending records from a table to a function, or from a function to a table, which is something that INSERT/SELECT can already do. Our use case is a table to a remote table, so it would go something like:1. COPY TO WITH BINARY on local node2. stream output bytes from #1 to remote node3. COPY FROM WITH BINARY on remotenode The only faster mechanism that I could imagine is sending the records themselves, which would be machine-dependent. Regards,Jeff Davis
Jeff Davis wrote: > All I mean is that the second argument to > COPY should produce/consume bytes and not records. I'm not discussing > the internal implementation at all, only semantics. > > In other words, STDIN is not a source of records, it's a source of > bytes; and likewise for STDOUT. > In the context of the read case, I'm not as sure it's so black and white. While the current situation does map better to a function that produces a stream of bytes, that's not necessarily the optimal approach for all situations. It's easy to imagine a function intended for accelerating bulk loading that is internally going to produce a stream of already processed records. A good example would be a function that is actually reading from another database system for the purpose of converting its data into PostgreSQL. If those were then loaded by a fairly direct path, that would happen at a much higher rate than if one had to convert those back into a stream of bytes with delimiters and then re-parse. I think there's a very valid use-case for both approaches. Maybe it just turns into an option, so you can get a faster loading path record at a time or just produce a stream characters, depending on what your data source maps to better. Something like this: COPY target FROM FUNCTION foo() WITH RECORDS; COPY target FROM FUNCTION foo() WITH BYTES; Would seem to cover both situations. I'd think that the WITH BYTES situation would just do some basic parsing and then pass the result through the same basic code path as WITH RECORDS, so having both available shouldn't increase the size of the implementation that much. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On Fri, 2009-11-27 at 20:28 -0500, Greg Smith wrote: > In the context of the read case, I'm not as sure it's so black and > white. While the current situation does map better to a function that > produces a stream of bytes, that's not necessarily the optimal approach > for all situations. It's easy to imagine a function intended for > accelerating bulk loading that is internally going to produce a stream > of already processed records. The binary COPY mode is one of the closest things I can think of to "already-processed records". Is binary COPY slow? If so, the only thing faster would have to be machine-specific, I think. > I think there's a very valid use-case for both approaches. ... > COPY target FROM FUNCTION foo() WITH RECORDS; In what format would the records be? Also, this still doesn't really answer why INSERT ... SELECT isn't good enough. If the records really are in their internal format, then INSERT ... SELECT seems like the way to go. Regards,Jeff Davis
On Thu, 2009-11-26 at 18:30 -0800, Daniel Farina wrote: > Okay, so this thread sort of wandered into how we could refactor other > elements of COPY. Do we have a good sense on what we should do to the > current patch (or at least the idea represented by it) to get it into > a committable state within finite time? We're in the middle of a commitfest, so a lot of hackers are concentrating on other patches. In a week or two, when it winds down, people will be more willing to make decisions on new proposals and designs. I still think this thread has been productive. > I think adding a bytea and/or text mode is once such improvement...I > am still reluctant to give up on INTERNAL because the string buffer > passed in the INTERNAL scenario is ideal for C programmers -- the > interface is even simpler than dealing with varlena types. But I > agree that auxiliary modes should exist to enable easier hacking. I like the idea of an internal mode as well. We may need some kind of performance numbers to justify avoiding the extra memcpy, though. > The thorniest issue in my mind is how state can be initialized > retained and/or modified between calls to the bytestream-acceptance > function. > > Arguably it is already in a state where it is no worse than dblink, > which itself has a global hash table to manage state. The idea of using a separate type of object (e.g. "CREATE COPYSTREAM") to bundle the init/read/write/end functions together might work. That also allows room to specify what the functions should accept (internal/bytea/text). I think that's the most elegant solution (at least it sounds good to me), but others might not like the idea of a new object type just for this feature. Perhaps if it fits nicely within an overall SQL/MED-like infrastructure, it will be easier to justify. > Also, if you look carefully at the dblink test suite I submitted, > you'll see an interesting trick: one can COPY from multiple sources > consecutively to a single COPY on a remote node when in text mode > (binary mode has a header that cannot be so neatly catenated). This > is something that's pretty hard to enable with any automatic > startup-work-cleanup approach. What if the network buffer is flushed in the middle of a line? Is that possible, or is there a guard against that somewhere? Regards,Jeff Davis
On Sun, Nov 29, 2009 at 6:35 PM, Jeff Davis <pgsql@j-davis.com> wrote: > What if the network buffer is flushed in the middle of a line? Is that > possible, or is there a guard against that somewhere? What do you mean? They both catenate onto one stream of bytes, it shouldn't matter where the flush boundaries are... It so happens as a convenient property of the textual modes is that adding more payload is purely concatenative (not true for binary, where there's a header that would cause confusion to the receiving side) fdr
On Sun, 2009-11-29 at 18:53 -0800, Daniel Farina wrote: > On Sun, Nov 29, 2009 at 6:35 PM, Jeff Davis <pgsql@j-davis.com> wrote: > > What if the network buffer is flushed in the middle of a line? Is that > > possible, or is there a guard against that somewhere? > > What do you mean? They both catenate onto one stream of bytes, it > shouldn't matter where the flush boundaries are... Nevermind, for some reason I thought you were talking about interleaving the data rather than concatenating. Regards,Jeff Davis
Jeff Davis wrote: <blockquote cite="mid:1259547819.3355.31.camel@jdavis" type="cite"><blockquote type="cite"><pre wrap="">COPYtarget FROM FUNCTION foo() WITH RECORDS; </pre></blockquote><pre wrap=""> In what format would the records be? </pre></blockquote> What was your intended internal format for this form to process?<br/><br /><pre class="moz-signature" cols="72">-- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support <a class="moz-txt-link-abbreviated" href="mailto:greg@2ndQuadrant.com">greg@2ndQuadrant.com</a> <a class="moz-txt-link-abbreviated"href="http://www.2ndQuadrant.com">www.2ndQuadrant.com</a> </pre>
On Mon, Nov 30, 2009 at 12:14 PM, Greg Smith <greg@2ndquadrant.com> wrote:
> Jeff Davis wrote:
>
> COPY target FROM FUNCTION foo() WITH RECORDS;
>
>
> In what format would the records be?
As a not-quite aside, I think I have a better syntax suggestion. I
have recently been digging around in the grammar with the changes made
in the following commit:
commit a6833fbb85cb5212a9d8085849e7281807f732a6
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon Sep 21 20:10:21 2009 +0000
Define a new, more extensible syntax for COPY options.
This is intentionally similar to the recently revised syntax for EXPLAIN options, ie, (name value, ...). The old
syntaxis still supported for backwards compatibility, but we intend that any options added in future will be
providedonly in the new syntax.
Robert Haas, Emmanuel Cecchet
As it turns out, the following syntax may work pretty well:
COPY y TO FUNCTION (setup_function the_setup_function('some arg', 3, 7, 42))
Basically the func_expr reduction fits very neatly into the
copy_generic_opt_elem reduction:
copy_generic_opt_elem: ColLabel copy_generic_opt_arg {
$$ = (Node *) makeDefElem($1, $2); }
| ColLabel func_expr { $$ =
(Node*) $2; } ;
Now we can use more or less any reasonable number of symbol names and
function calls we desire. This makes life considerably easier, I
think...
We can also try to refactor COPY's internals to take advantage of
these features (and potentially reduce the number of mechanisms. For
example, the legacy "COPY ... TO '/place' WITH CSV" perhaps can be
more verbosely/generically expressed as:
COPY ... TO FUNCTION (setup_function to_file('/place'), record_converter csv_formatter,
stream_function fwrite end_function fclose);
We can also add auxiliary symbols for error handling behavior. For
example, were the COPY to fail for some reason maybe it would make
sense "on_error" to call "unlink" to clean up the partially finished
file.
I also have what I think is a somewhat interesting hack. Consider
some of the functions up there without arguments (presumably they'd be
called with a somewhat fixed contract the mechanics of COPY itself):
how does one disambiguate them? Ideally, one could sometimes use
literal arguments (when the return value of that function is desired
to be threaded through the other specified functions) and other times
it'd be nice to disambiguate functions via type names. That would
look something like the following:
COPY ... TO FUNCTION (setup_function to_file('/place'), record_converter csv_formatter(record),
stream_function fwrite(bytea), end_function fclose(text));
I think this is possible to implement without much ambiguity, drawing
on the observation that the COPY statement does not have -- and
probably will never have -- references via Var(iable) node, unlike
normal SQL statements such as SELECT, INSERT, et al. That means we
might be able disambiguate using the following rules when scanning the
funcexpr's arguments during the semantic analysis phase to figure out
what to do:
* Only literal list items found: it's a function call with the types of those literals. Ex:
my_setup_function('foo'::text,3)
* Only non-literal list items found: it's type specifiers. Ex: csv_formatter(record).
* Both literal and non-literal values found: report an error.
This only works because no cases where a non-literal quantity could be
confused with a type name come to mind. If one could name a type "3"
and being forced to double-quote "3" to get your type disambiguated
was just too ugly, then we are at an impasse. But otherwise I think
this may work quite well.
Common constellations of functions could perhaps be bound together
into a DDL to reduce the amount of symbol soup going on here, but that
seems like a pretty clean transition strategy at some later time.
Most of the functionality could still be captured with this simple
approach for now...
Also note that factoring out high-performance implementations of
things like csv_formatter (and friends: pg_binary_formatter) will
probably take some time, but ultimately I think all the existing
functionality could be realized as a layer of syntactic sugar over
this mechanism.
fdr
On Sat, Dec 5, 2009 at 3:32 AM, Daniel Farina <drfarina@acm.org> wrote:
> On Mon, Nov 30, 2009 at 12:14 PM, Greg Smith <greg@2ndquadrant.com> wrote:
>> Jeff Davis wrote:
>>
>> COPY target FROM FUNCTION foo() WITH RECORDS;
>>
>>
>> In what format would the records be?
>
> As a not-quite aside, I think I have a better syntax suggestion. I
> have recently been digging around in the grammar with the changes made
> in the following commit:
>
> commit a6833fbb85cb5212a9d8085849e7281807f732a6
> Author: Tom Lane <tgl@sss.pgh.pa.us>
> Date: Mon Sep 21 20:10:21 2009 +0000
>
> Define a new, more extensible syntax for COPY options.
>
> This is intentionally similar to the recently revised syntax for EXPLAIN
> options, ie, (name value, ...). The old syntax is still supported for
> backwards compatibility, but we intend that any options added in future
> will be provided only in the new syntax.
>
> Robert Haas, Emmanuel Cecchet
>
> As it turns out, the following syntax may work pretty well:
>
> COPY y TO FUNCTION (setup_function the_setup_function('some arg', 3, 7, 42))
>
> Basically the func_expr reduction fits very neatly into the
> copy_generic_opt_elem reduction:
>
> copy_generic_opt_elem:
> ColLabel copy_generic_opt_arg
> {
> $$ = (Node *) makeDefElem($1, $2);
> }
> | ColLabel func_expr
> {
> $$ = (Node *) $2;
> }
> ;
>
> Now we can use more or less any reasonable number of symbol names and
> function calls we desire. This makes life considerably easier, I
> think...
>
> We can also try to refactor COPY's internals to take advantage of
> these features (and potentially reduce the number of mechanisms. For
> example, the legacy "COPY ... TO '/place' WITH CSV" perhaps can be
> more verbosely/generically expressed as:
>
> COPY ... TO FUNCTION (setup_function to_file('/place'),
> record_converter csv_formatter,
> stream_function fwrite
> end_function fclose);
>
> We can also add auxiliary symbols for error handling behavior. For
> example, were the COPY to fail for some reason maybe it would make
> sense "on_error" to call "unlink" to clean up the partially finished
> file.
>
> I also have what I think is a somewhat interesting hack. Consider
> some of the functions up there without arguments (presumably they'd be
> called with a somewhat fixed contract the mechanics of COPY itself):
> how does one disambiguate them? Ideally, one could sometimes use
> literal arguments (when the return value of that function is desired
> to be threaded through the other specified functions) and other times
> it'd be nice to disambiguate functions via type names. That would
> look something like the following:
>
> COPY ... TO FUNCTION (setup_function to_file('/place'),
> record_converter csv_formatter(record),
> stream_function fwrite(bytea),
> end_function fclose(text));
>
> I think this is possible to implement without much ambiguity, drawing
> on the observation that the COPY statement does not have -- and
> probably will never have -- references via Var(iable) node, unlike
> normal SQL statements such as SELECT, INSERT, et al. That means we
> might be able disambiguate using the following rules when scanning the
> funcexpr's arguments during the semantic analysis phase to figure out
> what to do:
>
> * Only literal list items found: it's a function call with the types
> of those literals. Ex: my_setup_function('foo'::text, 3)
>
> * Only non-literal list items found: it's type specifiers. Ex:
> csv_formatter(record).
>
> * Both literal and non-literal values found: report an error.
>
> This only works because no cases where a non-literal quantity could be
> confused with a type name come to mind. If one could name a type "3"
> and being forced to double-quote "3" to get your type disambiguated
> was just too ugly, then we are at an impasse. But otherwise I think
> this may work quite well.
>
> Common constellations of functions could perhaps be bound together
> into a DDL to reduce the amount of symbol soup going on here, but that
> seems like a pretty clean transition strategy at some later time.
> Most of the functionality could still be captured with this simple
> approach for now...
>
> Also note that factoring out high-performance implementations of
> things like csv_formatter (and friends: pg_binary_formatter) will
> probably take some time, but ultimately I think all the existing
> functionality could be realized as a layer of syntactic sugar over
> this mechanism.
I am very fuzzy on where we stand with this patch. There's a link to
the initial version on the commitfest application, but there has been
so much discussion since then that I think there are probably some
revisions to be made, though I can't say I know exactly what they are.
I did also check the git branch, but it does not merge cleanly with the master.
Is there a more recent version? Is this still under development? Or
have we decided to go a different direction with it?
Thanks,
...Robert
On Tue, Dec 29, 2009 at 5:57 PM, Robert Haas <robertmhaas@gmail.com> wrote: > I am very fuzzy on where we stand with this patch. There's a link to > the initial version on the commitfest application, but there has been > so much discussion since then that I think there are probably some > revisions to be made, though I can't say I know exactly what they are. > > I did also check the git branch, but it does not merge cleanly with the master. > > Is there a more recent version? Is this still under development? Or > have we decided to go a different direction with it? I think we've decided to go in a different direction for implementation, and my last communication was to suggest a mechanism that would allow for something more clean using the copy options refactoring. I wouldn't even attempt to merge it unless there's big outcry for the feature as-is, which I doubt there is. But perhaps it's worthy TODO fodder. The mechanics in the email you replied to probably need more feedback to act. fdr
On Tue, Dec 29, 2009 at 9:23 PM, Daniel Farina <drfarina@acm.org> wrote: > On Tue, Dec 29, 2009 at 5:57 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> I am very fuzzy on where we stand with this patch. There's a link to >> the initial version on the commitfest application, but there has been >> so much discussion since then that I think there are probably some >> revisions to be made, though I can't say I know exactly what they are. >> >> I did also check the git branch, but it does not merge cleanly with the master. >> >> Is there a more recent version? Is this still under development? Or >> have we decided to go a different direction with it? > > I think we've decided to go in a different direction for > implementation, and my last communication was to suggest a mechanism > that would allow for something more clean using the copy options > refactoring. I wouldn't even attempt to merge it unless there's big > outcry for the feature as-is, which I doubt there is. But perhaps > it's worthy TODO fodder. > > The mechanics in the email you replied to probably need more feedback to act. I think there's clear support for a version of COPY that returns rows like a SELECT statement, particularly for use with CTEs. There seems to be support both for a mode that returns text[] or something like it and also for a mode that returns a defined record type. But that all seems separate from what you're proposing here, which is a considerably lower-level facility which seems like it would not be of much use to ordinary users, but might be of some value to tool implementors - or perhaps you'd disagree with that characterization? Anyway, my specific reaction to your suggestions in the email that I quoted is that it seems a bit baroque and that I'm not really sure what it's useful for in practice. I'm certainly not saying it ISN'T useful, because I can't believe that you would have gone to the trouble to work through all of this unless you had some ideas about nifty things that could be done with it, but I think maybe we need to back up and start by talking about the problems you're trying to solve, before we get too far down into a discussion of implementation details. It doesn't appear to me that's been discussed too much so far, although there's enough enthusiasm here to make me suspect that other people may understand it better than I do. Based on your comments above, I'm going to go ahead and mark this particular patch as Returned with Feedback, since it seems you don't intend to continue with it and therefore it won't need to be reviewed during the January CommitFest. But I'm interesting in continuing the discussion and in any new patches that come out of it. ...Robert
On Tue, Dec 29, 2009 at 6:48 PM, Robert Haas <robertmhaas@gmail.com> wrote: > I think there's clear support for a version of COPY that returns rows > like a SELECT statement, particularly for use with CTEs. There seems > to be support both for a mode that returns text[] or something like it > and also for a mode that returns a defined record type. But that all > seems separate from what you're proposing here, which is a > considerably lower-level facility which seems like it would not be of > much use to ordinary users, but might be of some value to tool > implementors - or perhaps you'd disagree with that characterization? > This is in the other direction: freeing COPY from the restriction that it can only put bytes into two places: * A network socket (e.g. stdout) * A file (as supseruser) Instead, it can hand off bytes to an arbitrary UDF that can handle it in any way. A clean design should be able to subsume at least the existing simple behaviors, plus enabling more, as well as potentially providing inspiration for how to decouple at least a few components of COPY that perhaps can benefit the long-term cleanup effort there. > Anyway, my specific reaction to your suggestions in the email that I > quoted is that it seems a bit baroque and that I'm not really sure > what it's useful for in practice. I'm certainly not saying it ISN'T > useful, because I can't believe that you would have gone to the > trouble to work through all of this unless you had some ideas about > nifty things that could be done with it, but I think maybe we need to > back up and start by talking about the problems you're trying to > solve, before we get too far down into a discussion of implementation > details. At Truviso this is used a piece of our replication solution. In the patches submitted we see how enhancing dblink allows postgres to copy directly from one node to another. Truviso uses it to directly write bytes to a libpq connection (see the dblink patch) in the open COPY state to achieve direct cross-node bulk loading for the purposes of replication. One could imagine a lot of ETL or data warehouse offloading applications that can be enabled by allowing bytes to be handled by arbitrary code, although this patch achieves nothing that writing some middleware could not accomplish: it's just convenient to have and likely more efficient than writing some application middleware to do the same thing. fdr
On Tue, 2009-12-29 at 21:48 -0500, Robert Haas wrote: > Anyway, my specific reaction to your suggestions in the email that I > quoted is that it seems a bit baroque and that I'm not really sure > what it's useful for in practice. I'm certainly not saying it ISN'T > useful, because I can't believe that you would have gone to the > trouble to work through all of this unless you had some ideas about > nifty things that could be done with it, but I think maybe we need to > back up and start by talking about the problems you're trying to > solve, before we get too far down into a discussion of implementation > details. It doesn't appear to me that's been discussed too much so > far, although there's enough enthusiasm here to make me suspect that > other people may understand it better than I do. Dan made the use case fairly clear: "I have extended COPY to support using a UDF as a target instead of the normal 'file' or STDOUT targets. This dovetails nicely with a couple of extensions I have also written for dblink for the purposes of enabling direct cross-node bulk loading and replication." http://archives.postgresql.org/pgsql-hackers/2009-11/msg01555.php David Fetter had some ideas as well: http://archives.postgresql.org/pgsql-hackers/2009-11/msg01826.php Regards,Jeff Davis
On Tue, Dec 29, 2009 at 9:56 PM, Daniel Farina <drfarina@acm.org> wrote: > On Tue, Dec 29, 2009 at 6:48 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> I think there's clear support for a version of COPY that returns rows >> like a SELECT statement, particularly for use with CTEs. There seems >> to be support both for a mode that returns text[] or something like it >> and also for a mode that returns a defined record type. But that all >> seems separate from what you're proposing here, which is a >> considerably lower-level facility which seems like it would not be of >> much use to ordinary users, but might be of some value to tool >> implementors - or perhaps you'd disagree with that characterization? >> > > This is in the other direction: freeing COPY from the restriction that > it can only put bytes into two places: > > * A network socket (e.g. stdout) > * A file (as supseruser) Oh, duh. Actually, that seems like a pretty solid idea. I fear that to make this really useful we would need to define some new SQL syntax, like: CREATE [OR REPLACE] COPY TARGET name (STARTUP function_name, STREAM function_name, SHUTDOWN function_name); DROP COPY TARGET name; GRANT USAGE ON COPY TARGET TO ...; COPY ... TO/FROM TARGET name (generic_option_list) WITH (options); We could define the startup function to get the parameter list as a list of DefElems and return an internal structure of its own devising which would then be passed to the stream and shutdown functions. It might be possible to do this without introducing a notion of an explicit object, but there are a couple of problems with that. First, it requires the user to specify a lot of details on every COPY invocation, which is awkward. Second, there's a security issue to think about here. If we were just copying to a UDF that took a string as an argument, that would be fine, but it's not safe to let unprivileged users to directly invoke functions that take a type-internal argument. Introducing an explicit object type would allow the creation of copy targets to be restricted to super-users but then granted out to whom the super-user chooses. Thoughts? ...Robert
On Tue, 2009-12-29 at 23:11 -0500, Robert Haas wrote: > I fear that to make this really useful we would need to define some > new SQL syntax, like: > > CREATE [OR REPLACE] COPY TARGET name (STARTUP function_name, STREAM > function_name, SHUTDOWN function_name); > DROP COPY TARGET name; > GRANT USAGE ON COPY TARGET TO ...; > > COPY ... TO/FROM TARGET name (generic_option_list) WITH (options); Similar ideas were already suggested: http://archives.postgresql.org/pgsql-hackers/2009-11/msg01601.php http://archives.postgresql.org/pgsql-hackers/2009-11/msg01610.php Regardless, I think there needs to be a way to pass arguments to the functions (at least the startup one). The obvious use case is to pass the destination table name, so that you don't have to define a separate target for each destination table. Regards,Jeff Davis
On Tue, Dec 29, 2009 at 8:11 PM, Robert Haas <robertmhaas@gmail.com> wrote: > It might be possible to do this without introducing a notion of an > explicit object, but there are a couple of problems with that. First, > it requires the user to specify a lot of details on every COPY > invocation, which is awkward. Second, there's a security issue to > think about here. If we were just copying to a UDF that took a string > as an argument, that would be fine, but it's not safe to let > unprivileged users to directly invoke functions that take a > type-internal argument. Introducing an explicit object type would > allow the creation of copy targets to be restricted to super-users but > then granted out to whom the super-user chooses. > > Thoughts? Agree on the type internal and superuser access -- indeed, if one were to refactor the two existing COPY modes into external functions, the stdout behavior would be marked with SECURITY DEFINER and the to-file functions would only be superuser-accessible. (Interesting note: that means copy.c could theoretically lose the special check for superuser privilege for this mode, relying on standard function permissions...). I was mostly satisfied with a byzantine but otherwise semantically simple interface until the idea matures some more -- perhaps in practice -- to inform the most useful kind of convenience to support it. I don't see a strong reason personally to rush into defining such an interface just yet, although an important interface we would have to define is contract a user would have to follow to enable their very own fully featured COPY output mode. Still, the patch as-submitted is quite far from achieving one of my main litmus test goals: subsumption of existing COPY behavior. Particularly thorny was how to support the copying-to-a-file semantics, but I believe that the copy-options patch provide a nice avenue to solve this problem, as one can call a function in the options list and somehow pass the return value of that initializer -- which may include a file handle -- to the byte-handling function. Finally, I think a valid point was made that the patch is much more powerful to end users if it supports byte arrays, and there are some open questions as to whether this should be the only/primary supported mode. I personally like the INTERNAL-type interface, as dealing with the StringInfo buffer used by current COPY code is very convenient from C and the current implementation is not very complicated yet avoids unnecessary copying/value coercions, but I agree there is definitely value in enabling the use of more normal data types. fdr
On Tue, Dec 29, 2009 at 11:44 PM, Jeff Davis <pgsql@j-davis.com> wrote: > On Tue, 2009-12-29 at 23:11 -0500, Robert Haas wrote: >> I fear that to make this really useful we would need to define some >> new SQL syntax, like: >> >> CREATE [OR REPLACE] COPY TARGET name (STARTUP function_name, STREAM >> function_name, SHUTDOWN function_name); >> DROP COPY TARGET name; >> GRANT USAGE ON COPY TARGET TO ...; >> >> COPY ... TO/FROM TARGET name (generic_option_list) WITH (options); > > Similar ideas were already suggested: > > http://archives.postgresql.org/pgsql-hackers/2009-11/msg01601.php > http://archives.postgresql.org/pgsql-hackers/2009-11/msg01610.php Sorry, it's been a while since I've read through this thread and I am not as up on it as perhaps I should be. I generally agree with those ideas, although I think that trying to make the existing aggregate interface serve this purpose will probably turn out to be trying to make a square peg fit a round hole. > Regardless, I think there needs to be a way to pass arguments to the > functions (at least the startup one). The obvious use case is to pass > the destination table name, so that you don't have to define a separate > target for each destination table. Agreed, note that I suggested a way to do that. ...Robert
On Tue, Dec 29, 2009 at 11:47 PM, Daniel Farina <drfarina@acm.org> wrote: > On Tue, Dec 29, 2009 at 8:11 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> It might be possible to do this without introducing a notion of an >> explicit object, but there are a couple of problems with that. First, >> it requires the user to specify a lot of details on every COPY >> invocation, which is awkward. Second, there's a security issue to >> think about here. If we were just copying to a UDF that took a string >> as an argument, that would be fine, but it's not safe to let >> unprivileged users to directly invoke functions that take a >> type-internal argument. Introducing an explicit object type would >> allow the creation of copy targets to be restricted to super-users but >> then granted out to whom the super-user chooses. >> >> Thoughts? > > Agree on the type internal and superuser access -- indeed, if one were > to refactor the two existing COPY modes into external functions, the > stdout behavior would be marked with SECURITY DEFINER and the to-file > functions would only be superuser-accessible. (Interesting note: that > means copy.c could theoretically lose the special check for superuser > privilege for this mode, relying on standard function permissions...). > > I was mostly satisfied with a byzantine but otherwise semantically > simple interface until the idea matures some more -- perhaps in > practice -- to inform the most useful kind of convenience to support > it. I don't see a strong reason personally to rush into defining such > an interface just yet, although an important interface we would have > to define is contract a user would have to follow to enable their very > own fully featured COPY output mode. I think we should try to put an interface layer in place in the first iteration. I don't really see this as having much value without that.If we implement this strictly as a series of COPY options,we're going to be committed to supporting that interface forever whether it turns out to be good for anything or not. Since any such interface would pretty much have to be superuser-only, I'm inclined to think that there is not enough bang for the buck to justify the ongoing maintenance effort. One way to figure out whether we've define the COPY TARGET interface correctly is to put together a few sample implementations and see whether they seem functional and useful, without too many lacunas. I think it should be possible to assess from a few such implementations whether we have basically the right design. > Still, the patch as-submitted is quite far from achieving one of my > main litmus test goals: subsumption of existing COPY behavior. > Particularly thorny was how to support the copying-to-a-file > semantics, but I believe that the copy-options patch provide a nice > avenue to solve this problem, as one can call a function in the > options list and somehow pass the return value of that initializer -- > which may include a file handle -- to the byte-handling function. It may be useful to consider whether the current behavior could be re-implemented using some proposed extension mechanism, but I would not put much emphasis on trying to really subsume the current behavior into such an implementation. Certainly, we will need to continue support the existing syntax forever, so the existing functionality will need to be special-cased at least to that extent. > Finally, I think a valid point was made that the patch is much more > powerful to end users if it supports byte arrays, and there are some > open questions as to whether this should be the only/primary supported > mode. I personally like the INTERNAL-type interface, as dealing with > the StringInfo buffer used by current COPY code is very convenient > from C and the current implementation is not very complicated yet > avoids unnecessary copying/value coercions, but I agree there is > definitely value in enabling the use of more normal data types. I agree that needs some further thought. Again, a few sample implementations might help provide clarity here. I agree that the StringInfo representation is a bit random, but OTOH I mostly see someone installing a few COPY TARGET implementations into their DB and then using them over and over again. Creating a new COPY TARGET should be relatively rare, so if they have to be written in C, we may not care that much. On the flip side we should be looking for some concrete gain from putting that restriction into the mix. ...Robert
On Tue, Dec 29, 2009 at 9:11 PM, Robert Haas <robertmhaas@gmail.com> wrote: > I think we should try to put an interface layer in place in the first > iteration. I don't really see this as having much value without that. > If we implement this strictly as a series of COPY options, we're > going to be committed to supporting that interface forever whether it > turns out to be good for anything or not. Since any such interface > would pretty much have to be superuser-only, I'm inclined to think > that there is not enough bang for the buck to justify the ongoing > maintenance effort. Interestingly, I came to the opposite conclusion you did: I thought supporting new top-level syntax might be the more invasive and support-heavy change as opposed to just changing the option handling grammar to support funcall-looking things on the right-hand-side. True, the semantics and interfaces between symbols would need to be somewhat frozen and documented in either case. I do value in supporting a notion of "this constellation of functions is OK, even if one of them does take type INTERNAL", whereas my proposal has no mechanism to address such a constellation: everything is independent, which does not capture all the information necessary to determine if it's safe to execute a particular combination of functions to perform a COPY. I think you may be right about this, so we could basically move most things from the COPY option list into a new DDL option list...I am more ambivalent, but it seems that would require a catalog, and so on, which is why I did not leap to do that initially for the very support-reasons you mentioned. > One way to figure out whether we've define the COPY TARGET interface > correctly is to put together a few sample implementations and see > whether they seem functional and useful, without too many lacunas. I > think it should be possible to assess from a few such implementations > whether we have basically the right design. Okay, I was simply less optimistic than that (that we could get it right so easily), and was content to put out something that had reasonable design but perhaps byzantine (but well-defined) interfaces and let complaints/wishes drive completion. But given that the main difference between your proposal and mine is to move things out of COPY's option list and into a toplevel DDL option list, under the covers things are pretty samey, except I would imagine your proposal requires committing to a new catalog(?) which also enables addressing the combination of functions as a unit and a new top-level DDL (but avoids committing to many new COPY options). > I agree that needs some further thought. Again, a few sample > implementations might help provide clarity here. I agree that the > StringInfo representation is a bit random, but OTOH I mostly see > someone installing a few COPY TARGET implementations into their DB and > then using them over and over again. Creating a new COPY TARGET > should be relatively rare, so if they have to be written in C, we may > not care that much. On the flip side we should be looking for some > concrete gain from putting that restriction into the mix. I was originally inclined to agree, but I believe there are others who the most interesting and use applications are only captured if userland types are supported, and I can see good reason for that belief... Even something as simple as authoring "tee" for COPY will require writing C here, whereas when supporting userland types people can hack out a PL function that calls some other existing C-written functions (we can provide a BYTEA veneer on top of the INTERNAL-version of a function rather trivially express for this purpose...) fdr
On Wed, Dec 30, 2009 at 12:26 AM, Daniel Farina <drfarina@acm.org> wrote: > I think you may be right about this, so we could basically move most > things from the COPY option list into a new DDL option list...I am > more ambivalent, but it seems that would require a catalog, and so on, > which is why I did not leap to do that initially for the very > support-reasons you mentioned. Adding a catalog doesn't bother me a bit. That doesn't really cost anything. What I'm concerned about in terms of maintenance is that anything we support in 8.5 will likely have to be supported in 8.6, 8.7, 8.8, 8.9, and 8.10, and probably beyond that. If we do something that turns out to be CLEARLY a bad idea, then we MIGHT eventually be able to get rid of it. But we probably won't be that (un)lucky. We're more likely to do something that's sorta-useful, but makes future enhancements that we care about more difficult. And then we'll be stuck with it. That's really what I care about avoiding. In other words, it's OK if we add something now that may need to be further extended in the future, but it's BAD if we add something now that we want to change incompatibly or take out in the future. > But given that the main difference between your proposal and mine is > to move things out of COPY's option list and into a toplevel DDL > option list, under the covers things are pretty samey, except I would > imagine your proposal requires committing to a new catalog(?) which > also enables addressing the combination of functions as a unit and a > new top-level DDL (but avoids committing to many new COPY options). I agree with that summary. I don't think the catalog is a problem (though we need to make sure to get the dependency handling correct). The additional DDL syntax is more likely to draw objections, although perhaps unsurprisingly I don't have a problem with it personally. >> I agree that needs some further thought. Again, a few sample >> implementations might help provide clarity here. I agree that the >> StringInfo representation is a bit random, but OTOH I mostly see >> someone installing a few COPY TARGET implementations into their DB and >> then using them over and over again. Creating a new COPY TARGET >> should be relatively rare, so if they have to be written in C, we may >> not care that much. On the flip side we should be looking for some >> concrete gain from putting that restriction into the mix. > > I was originally inclined to agree, but I believe there are others who > the most interesting and use applications are only captured if > userland types are supported, and I can see good reason for that > belief... > > Even something as simple as authoring "tee" for COPY will require > writing C here, whereas when supporting userland types people can hack > out a PL function that calls some other existing C-written functions > (we can provide a BYTEA veneer on top of the INTERNAL-version of a > function rather trivially express for this purpose...) Sure. If you think you can make it work using bytea, that seems clearly better than using an internal type, all things being equal. ...Robert
On Tue, Dec 29, 2009 at 9:46 PM, Robert Haas <robertmhaas@gmail.com> wrote: > Sure. If you think you can make it work using bytea, that seems > clearly better than using an internal type, all things being equal. I think both are perfectly viable and can be supported simultaneously, actually...I simply like INTERNAL because the mechanism and passed data structure for when you *do* want to write a C function is just really crisp and simple, and is not going to be a source of overhead. fdr
On Wed, Dec 30, 2009 at 12:56 AM, Daniel Farina <drfarina@acm.org> wrote: > On Tue, Dec 29, 2009 at 9:46 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> Sure. If you think you can make it work using bytea, that seems >> clearly better than using an internal type, all things being equal. > > I think both are perfectly viable and can be supported simultaneously, > actually...I simply like INTERNAL because the mechanism and passed > data structure for when you *do* want to write a C function is just > really crisp and simple, and is not going to be a source of overhead. OK. I think we'll have to see the proposed patch before we can really make a final judgement on this, but hopefully I've at least given you enough feedback/food for thought to move this forward in a meaningful way... ...Robert
On Wed, 2009-12-30 at 00:00 -0500, Robert Haas wrote: > I generally agree with those > ideas, although I think that trying to make the existing aggregate > interface serve this purpose will probably turn out to be trying to > make a square peg fit a round hole. I didn't think that they intended to actually use CREATE AGGREGATE, although I could be mistaken. Regards,Jeff Davis