Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

>  Here is a very straw-man-level sketch of what I think might work.
>  The option to COPY FROM looks something like
>
>   ERRORS TO other_table_name (item [, item [, ...]])
>

I tried to implement the patch using a table and came across a number of
questions.

Which table should we implement for this feature: a system catalog table or
store this table as a file or create a new table?

In these cases, security and user rights management issues arise.
It is better for other users not to see error lines from another user. It
is also not clear how access rights to this table are inherited and be
given.


--
Regards,
Damir Belyalov
Postgres Professional

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Hello everyone!
Thanks for turning back to this patch.


I had already thought about storing errors in the table / separate file /
logfile and it seems to me that the best way is to output errors in
logfile. As for user it is more convenient to look for errors in the place
where they are usually generated - in logfile and if he wants to intercept
them he could easily do that by few commands.


The analogues of this feature in other DBSM usually had additional files
for storing errors, but their features had too many options (see attached
files).
I also think that the best way is to simplify this feature for the first
version and don't use redundant adjustments such as additional files and
other options.
IMHO for more complicated operations with loading tables files pgloader
exists: https://github.com/dimitri/pgloader


Links of analogues of COPY IGNORE_DATATYPE_ERRORS
https://dev.mysql.com/doc/refman/8.0/en/load-data.html
https://docs.aws.amazon.com/redshift/latest/dg/r_COPY_command_examples.html

Regards,
Damir Belyalov
Postgres Professional

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir]

Although v7 patch doesn't have commit messages on the patch, I think 
leave commit message is good for reviewers.


Sure, didn't notice it. Added the commit message to the updated patch.



  * Note: DON'T convert this error to "soft" style (errsave/ereturn). We
  * want this data type to stay permanently in the hard-error world 
so that

  * it can be used for testing that such cases still work reasonably.


From this point of view, I think this is a supposed way of using widget.


I agree, it's a good approach for checking datatype errors, because 
that's what was intended.



OTOH widget is declared in create_type.sql and I'm not sure it's ok to 
use it in another test copy2.sql.


I think that other regress tests with 'widget' type that will be created 
in the future can be not only in the create_type.sql. So it's not a 
problem that some type or function is taken from another regress test. 
For example, the table 'onek' is used in many regress tests.



Regards,

Damir Belyalov

Postgres Professional
From 0e1193e00bb5ee810a015a2baaf7c79e395a54c7 Mon Sep 17 00:00:00 2001
From: Damir Belyalov 
Date: Fri, 15 Sep 2023 11:14:57 +0300
Subject: [PATCH v7] Add new COPY option IGNORE_DATATYPE_ERRORS

Currently entire COPY fails even when there is one unexpected data
regarding data type or range.
IGNORE_DATATYPE_ERRORS ignores these errors and skips them and COPY
data which don't contain problem.

This patch uses the soft error handling infrastructure, which is
introduced by d9f7f5d32f20.

Author: Damir Belyalov, Atsushi Torikoshi

---
 doc/src/sgml/ref/copy.sgml   | 13 +
 src/backend/commands/copy.c  | 13 +
 src/backend/commands/copyfrom.c  | 37 
 src/backend/commands/copyfromparse.c | 20 ++---
 src/bin/psql/tab-complete.c  |  3 +-
 src/include/commands/copy.h  |  1 +
 src/include/commands/copyfrom_internal.h |  3 ++
 src/test/regress/expected/copy2.out  | 28 ++
 src/test/regress/sql/copy2.sql   | 26 +
 9 files changed, 139 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 4d614a0225..d5cdbb4025 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -43,6 +43,7 @@ COPY { table_name [ ( column_name [, ...] ) | * }
 FORCE_NOT_NULL ( column_name [, ...] )
 FORCE_NULL ( column_name [, ...] )
+IGNORE_DATATYPE_ERRORS [ boolean ]
 ENCODING 'encoding_name'
 
  
@@ -370,6 +371,18 @@ COPY { table_name [ ( 

 
+   
+IGNORE_DATATYPE_ERRORS
+
+ 
+  Drops rows that contain malformed data while copying. These are rows
+  with columns where the data type's input-function raises an error.
+  This option is not allowed when using binary format.  Note that this
+  is only supported in current COPY syntax.
+ 
+
+   
+

 ENCODING
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f14fae3308..beb73f5357 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -419,6 +419,7 @@ ProcessCopyOptions(ParseState *pstate,
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
+	bool		ignore_datatype_errors_specified = false;
 	ListCell   *option;
 
 	/* Support external use for option sanity checking */
@@ -458,6 +459,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_datatype_errors") == 0)
+		{
+			if (ignore_datatype_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_datatype_errors_specified = true;
+			opts_out->ignore_datatype_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
@@ -594,6 +602,11 @@ ProcessCopyOptions(ParseState *pstate,
 (errcode(ERRCODE_SYNTAX_ERROR),
  errmsg("cannot specify DEFAULT in BINARY mode")));
 
+	if (opts_out->binary && opts_out->ignore_datatype_errors)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("cannot specify IGNORE_DATATYPE_ERRORS in BINARY mode")));
+
 	/* Set defaults for omitted options */
 	if (!opts_out->delim)
 		opts_out->delim = opts_out->csv_mode ? "," : "\t";
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 70871ed819..b18aea6376 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -752,6 +752,14 @@ CopyFrom(CopyFromState cstate)
 		ti_options |= TABLE_INSERT_FROZEN;
 	}
 
+	/* Set up soft error handler for IGNORE_DATATYPE_ERRORS */
+	if (cstate->opts.ignore_datatype_errors)
+	{
+		ErrorSaveContext escontext = {T_ErrorSaveContext};
+		escontext.details_wanted = true;
+		cstate->escontext

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

> Since v5 patch failed applying anymore, updated the patch.
Thank you for updating the patch . I made a little review on it where
corrected some formatting.


> - COPY with a datatype error that can't be handled as a soft error
>
> I didn't know proper way to test this, but I've found data type widget's
> input function widget_in() defined to occur hard-error in regress.c,
> attached patch added a test using it.
>

This test seems to be weird a bit, because of the "widget" type. The hard
error is thrown by the previous test with missing data. Also it'll be
interesting for me to list all cases when a hard error can be thrown.

Regards,
Damir Belyalov
Postgres Professional
From 0e1193e00bb5ee810a015a2baaf7c79e395a54c7 Mon Sep 17 00:00:00 2001
From: Damir Belyalov 
Date: Fri, 15 Sep 2023 11:14:57 +0300
Subject: [PATCH] ignore errors

---
 doc/src/sgml/ref/copy.sgml   | 13 +
 src/backend/commands/copy.c  | 13 +
 src/backend/commands/copyfrom.c  | 37 
 src/backend/commands/copyfromparse.c | 20 ++---
 src/bin/psql/tab-complete.c  |  3 +-
 src/include/commands/copy.h  |  1 +
 src/include/commands/copyfrom_internal.h |  3 ++
 src/test/regress/expected/copy2.out  | 28 ++
 src/test/regress/sql/copy2.sql   | 26 +
 9 files changed, 139 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 4d614a0225..d5cdbb4025 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -43,6 +43,7 @@ COPY { table_name [ ( column_name [, ...] ) | * }
 FORCE_NOT_NULL ( column_name [, ...] )
 FORCE_NULL ( column_name [, ...] )
+IGNORE_DATATYPE_ERRORS [ boolean ]
 ENCODING 'encoding_name'
 
  
@@ -370,6 +371,18 @@ COPY { table_name [ ( 

 
+   
+IGNORE_DATATYPE_ERRORS
+
+ 
+  Drops rows that contain malformed data while copying. These are rows
+  with columns where the data type's input-function raises an error.
+  This option is not allowed when using binary format.  Note that this
+  is only supported in current COPY syntax.
+ 
+
+   
+

 ENCODING
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f14fae3308..beb73f5357 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -419,6 +419,7 @@ ProcessCopyOptions(ParseState *pstate,
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
+	bool		ignore_datatype_errors_specified = false;
 	ListCell   *option;
 
 	/* Support external use for option sanity checking */
@@ -458,6 +459,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_datatype_errors") == 0)
+		{
+			if (ignore_datatype_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_datatype_errors_specified = true;
+			opts_out->ignore_datatype_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
@@ -594,6 +602,11 @@ ProcessCopyOptions(ParseState *pstate,
 (errcode(ERRCODE_SYNTAX_ERROR),
  errmsg("cannot specify DEFAULT in BINARY mode")));
 
+	if (opts_out->binary && opts_out->ignore_datatype_errors)
+		ereport(ERROR,
+(errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("cannot specify IGNORE_DATATYPE_ERRORS in BINARY mode")));
+
 	/* Set defaults for omitted options */
 	if (!opts_out->delim)
 		opts_out->delim = opts_out->csv_mode ? "," : "\t";
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 70871ed819..b18aea6376 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -752,6 +752,14 @@ CopyFrom(CopyFromState cstate)
 		ti_options |= TABLE_INSERT_FROZEN;
 	}
 
+	/* Set up soft error handler for IGNORE_DATATYPE_ERRORS */
+	if (cstate->opts.ignore_datatype_errors)
+	{
+		ErrorSaveContext escontext = {T_ErrorSaveContext};
+		escontext.details_wanted = true;
+		cstate->escontext = escontext;
+	}
+
 	/*
 	 * We need a ResultRelInfo so we can use the regular executor's
 	 * index-entry-making machinery.  (There used to be a huge amount of code
@@ -987,7 +995,36 @@ CopyFrom(CopyFromState cstate)
 
 		/* Directly store the values/nulls array in the slot */
 		if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
+		{
+			if (cstate->opts.ignore_datatype_errors &&
+cstate->ignored_errors_count > 0)
+ereport(WARNING,
+		errmsg("%zd rows were skipped due to data type incompatibility",
+			   cstate->ignored_errors_count));
 			break;
+		}
+
+		/* Soft error occured, skip this tuple

Re: Redundant Unique plan node for table with a unique index

[Damir Belyalov]

Thank you for feedback and thread [1].

Regards,
Damir Belyalov
Postgres Professional

Redundant Unique plan node for table with a unique index

[Damir Belyalov]

Hello!

There is a table with a unique index on it and we have a query that
searching DISTINCT values on this table on columns of unique index. Example:


create table a (n int);
insert into a (n) select x from generate_series(1, 14) as g(x);
create unique index on a (n);
explain select distinct n from a;
 QUERY PLAN


 Unique  (cost=0.42..6478.42 rows=14 width=4)
   ->  Index Only Scan using a_n_idx on a  (cost=0.42..6128.42 rows=14
width=4)
(2 rows)


We can see that Unique node is redundant for this case. So I implemented a
simple patch that removes Unique node from the plan.
After patch:


explain select distinct n from a;
   QUERY PLAN
-
 Seq Scan on a  (cost=0.00..2020.00 rows=14 width=4)
(1 row)


The patch is rather simple and doesn't consider queries with joins. The
criteria when Unique node is should be removed is a case when a set of Vars
in DISTINCT clause contains unique index columns from the same table.
Another example:
CREATE TABLE a (n int, m int);
CRETE UNIQUE INDEX ON a (n);
SELECT DISTINCT (n,m) FROM a;
The Unique node should be deleted because n is contained in (n,m).


The patch doesn't consider these cases:
1. DISTINCT ON [EXPR]
   Because this case can need grouping.
2. Subqueries.
   Because this case can need grouping:
   CREATE TABLE a (n int);
   CREA UNIQUE INDEX ON a (n);
   SELECT DISTINCT g FROM (SELECT * FROM a) as g;
3. Joins, because it demands complication of code.
   Example:
   SELECT DISTINCT a.n1 JOIN b where a.n1 = b.n1;
   where a.n1 and b.n1 should be unique indexes and join qual should be
on this index columns.
   or
   a have a unique index on n1 and b is "unique for a" on join qual.


I am wondering if there are opportunities for further development of this
patch, in particular for JOIN cases.
For several levels of JOINs we should understand which set columns is
unique for the every joinrel in query. In general terms I identified two
cases when joinrel "saves" unique index from table: when tables are joined
by unique index columns and when one table has unique index and it is
"unique_for" (has one common tuple) another table.


Regards,
Damir Belyalov
Postgres Professional
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 44efb1f4ebc..8f9912f48ad 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1266,6 +1266,47 @@ preprocess_phv_expression(PlannerInfo *root, Expr *expr)
 	return (Expr *) preprocess_expression(root, (Node *) expr, EXPRKIND_PHV);
 }
 
+static bool
+is_distinct_redundant(Query *parse, RelOptInfo *current_rel, PathTarget *sort_input_target)
+{
+	List	 *distinct_vars = NIL;
+	ListCell *lc1, *lc2;
+
+	/* Doesn't process DISTINCT ON because it can need grouping */
+	if (parse->hasDistinctOn)
+		return false;
+
+	/* Fetch Vars from DISTINCT clause */
+	foreach(lc1, sort_input_target->exprs)
+	{
+		Var *distinct_expr = (Var *) lfirst(lc1);
+
+		if (IsA(distinct_expr, Var))
+			distinct_vars = list_append_unique_int(distinct_vars, distinct_expr->varattno);
+		else
+			/* Doesn't process this case because it can need grouping */
+			return false;
+	}
+
+	foreach(lc2, current_rel->indexlist)
+	{
+		IndexOptInfo *indexinfo = (IndexOptInfo *) lfirst(lc2);
+		List		 *unique_indexkeys = NIL;
+		int			  i;
+
+		if (indexinfo->unique)
+		{
+			for (i = 0; i < indexinfo->ncolumns; i++)
+unique_indexkeys = list_append_unique_int(unique_indexkeys, indexinfo->indexkeys[i]);
+
+			if (list_difference_int(unique_indexkeys, distinct_vars) == NIL)
+return true;
+		}
+	}
+
+	return false;
+}
+
 /*
  * grouping_planner
  *	  Perform planning steps related to grouping, aggregation, etc.
@@ -1694,8 +1735,9 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
 		 */
 		if (parse->distinctClause)
 		{
-			current_rel = create_distinct_paths(root,
-current_rel);
+			if (!is_distinct_redundant(parse, current_rel, sort_input_target))
+current_rel = create_distinct_paths(root,
+	current_rel);
 		}
 	}			/* end of if (setOperations) */
 
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 9b8638f286a..49223c5be10 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -5645,18 +5645,15 @@ select d.* from d left join (select * from b group by b.id, b.c_id) s
 explain (costs off)
 select d.* from d left join (select distinct * from b) s
   on d.a = s.id;
-  QUERY PLAN  
---
+ QUERY PLAN 
+
  Merge Right Join
  

Re: Output affected rows in EXPLAIN

[Damir Belyalov]

> This creates a bug, not fixes one.  It's intentional that "insert into a"
> is shown as returning zero rows, because that's what it did.  If you'd
> written "insert ... returning", you'd have gotten a different result:
>
Maybe I didn't understand you correctly, but I didn't touch the number of
affected rows in EXPLAIN output.
It's just a simple patch that adds 1 row after using commands: EXPLAIN
INSERT, EXPLAIN UPDATE, EXPLAIN DELETE.
It was done because the commands INSERT/UPDATE/DELETE return one row after
execution: "UPDATE 7" or "INSERT 0 4".
EXPLAIN (ANALYZE) INSERT/UPDATE/DELETE does the same thing as these
commands, but doesn't output this row. So I added it.


Patch is fixed. There is no row "EXPLAIN" in queries like:
postgres=# explain (analyze) select * from t;
  QUERY PLAN
---
 Seq Scan on t  (cost=0.00..35.50 rows=2550 width=4) (actual
time=0.064..0.075 rows=5 loops=1)
 Planning Time: 1.639 ms
 Execution Time: 0.215 ms
(3 rows)

EXPLAIN


What is about queries EXPLAIN INSERT/UPDATE/DELETE without ANALYZE?
Now it is outputting a row with 0 affected (inserted) rows at the end:
"INSERT 0 0", "UPDATE 0". Example:
explain update a set n = 2;
 QUERY PLAN

 Update on a  (cost=0.00..35.50 rows=0 width=0)
   ->  Seq Scan on a  (cost=0.00..35.50 rows=2550 width=10)
(2 rows)

UPDATE 0

Regards,
Damir Belyalov
Postgres Professional
From c6cbc6fa9ddf24f29bc19ff115224dd76e351db1 Mon Sep 17 00:00:00 2001
From: Damir Belyalov 
Date: Tue, 5 Sep 2023 15:04:01 +0300
Subject: [PATCH 1/2] Output affected rows in EXPLAIN.

---
 src/backend/commands/explain.c | 10 +-
 src/backend/tcop/cmdtag.c  |  2 +-
 src/backend/tcop/pquery.c  |  8 +++-
 src/backend/tcop/utility.c | 27 ++-
 src/bin/psql/common.c  |  5 +++--
 src/include/commands/explain.h |  3 ++-
 src/include/tcop/cmdtag.h  |  1 +
 src/include/tcop/cmdtaglist.h  |  3 +++
 src/interfaces/libpq/fe-exec.c |  4 +++-
 9 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 8570b14f62..453e545ba5 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -162,7 +162,7 @@ static void escape_yaml(StringInfo buf, const char *str);
  */
 void
 ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
-			 ParamListInfo params, DestReceiver *dest)
+			 ParamListInfo params, DestReceiver *dest, uint64 *processed)
 {
 	ExplainState *es = NewExplainState();
 	TupOutputState *tstate;
@@ -173,6 +173,9 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 	bool		timing_set = false;
 	bool		summary_set = false;
 
+	if (processed)
+		*processed = 0;
+
 	/* Parse options list. */
 	foreach(lc, stmt->options)
 	{
@@ -311,6 +314,9 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 	end_tup_output(tstate);
 
 	pfree(es->str->data);
+
+	if (processed)
+		*processed = es->es_processed;
 }
 
 /*
@@ -649,6 +655,8 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
 	 */
 	INSTR_TIME_SET_CURRENT(starttime);
 
+	es->es_processed += queryDesc->estate->es_processed;
+
 	ExecutorEnd(queryDesc);
 
 	FreeQueryDesc(queryDesc);
diff --git a/src/backend/tcop/cmdtag.c b/src/backend/tcop/cmdtag.c
index 4bd713a0b4..9e6fdbd8af 100644
--- a/src/backend/tcop/cmdtag.c
+++ b/src/backend/tcop/cmdtag.c
@@ -146,7 +146,7 @@ BuildQueryCompletionString(char *buff, const QueryCompletion *qc,
 	 */
 	if (command_tag_display_rowcount(tag) && !nameonly)
 	{
-		if (tag == CMDTAG_INSERT)
+		if (tag == CMDTAG_INSERT || tag == CMDTAG_EXPLAIN_INSERT)
 		{
 			*bufp++ = ' ';
 			*bufp++ = '0';
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 5565f200c3..ba0b33cc67 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -775,7 +775,13 @@ PortalRun(Portal portal, long count, bool isTopLevel, bool run_once,
 if (qc && portal->qc.commandTag != CMDTAG_UNKNOWN)
 {
 	CopyQueryCompletion(qc, >qc);
-	qc->nprocessed = nprocessed;
+	if (portal->qc.commandTag == CMDTAG_EXPLAIN ||
+		portal->qc.commandTag == CMDTAG_EXPLAIN_INSERT ||
+		portal->qc.commandTag == CMDTAG_EXPLAIN_UPDATE ||
+		portal->qc.commandTag == CMDTAG_EXPLAIN_DELETE)
+		qc->nprocessed = portal->qc.nprocessed;
+	else
+		qc->nprocessed = nprocessed;
 }
 
 /* Mark portal not active */
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index e3ccf6c7f7..8975d046f9 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -867,7 +867,32 @@ standard_ProcessUtility(PlannedStmt *pstmt,
 

Output affected rows in EXPLAIN

[Damir Belyalov]

I create a patch that outputs affected rows in EXPLAIN that occur by
INSERT/UPDATE/DELETE.
Despite the fact that commands in EXPLAIN ANALYZE query are executed as
usual, EXPLAIN doesn't show outputting affected rows as in these commands.
The patch fixes this problem.

Examples:
explain analyze insert into a values (1);
QUERY PLAN

--
 Insert on a  (cost=0.00..0.01 rows=0 width=0) (actual time=0.076..0.077
rows=0 loops=1)
   ->  Result  (cost=0.00..0.01 rows=1 width=4) (actual time=0.002..0.002
rows=1 loops=1)
 Planning Time: 0.025 ms
 Execution Time: 0.412 ms
(4 rows)

INSERT 0 1

  QUERY PLAN

--
 Update on a  (cost=0.00..35.50 rows=0 width=0) (actual time=0.059..0.060
rows=0 loops=1)
   ->  Seq Scan on a  (cost=0.00..35.50 rows=2550 width=10) (actual
time=0.012..0.013 rows=7 loops=1)
 Planning Time: 0.142 ms
 Execution Time: 0.666 ms
(4 rows)

UPDATE 7

explain analyze delete from a where n = 1;
QUERY PLAN

---
 Delete on a  (cost=0.00..41.88 rows=0 width=0) (actual time=0.147..0.147
rows=0 loops=1)
   ->  Seq Scan on a  (cost=0.00..41.88 rows=13 width=6) (actual
time=0.120..0.123 rows=7 loops=1)
 Filter: (n = 1)
 Planning Time: 1.073 ms
 Execution Time: 0.178 ms
(5 rows)

DELETE 7

EXPLAIN queries without ANALYZE don't affect rows, so the output number is
0.

explain update a set n = 2;
 QUERY PLAN

 Update on a  (cost=0.00..35.50 rows=0 width=0)
   ->  Seq Scan on a  (cost=0.00..35.50 rows=2550 width=10)
(2 rows)

UPDATE 0

Maybe there is no need to add this row when EXPLAIN has no ANALYZE. So it
is a discussion question.
Also haven't fixed regress tests yet.

Regards,
Damir Belyalov
Postgres Professional
From c6cbc6fa9ddf24f29bc19ff115224dd76e351db1 Mon Sep 17 00:00:00 2001
From: Damir Belyalov 
Date: Tue, 5 Sep 2023 15:04:01 +0300
Subject: [PATCH] Output affected rows in EXPLAIN.

---
 src/backend/commands/explain.c | 10 +-
 src/backend/tcop/cmdtag.c  |  2 +-
 src/backend/tcop/pquery.c  |  8 +++-
 src/backend/tcop/utility.c | 27 ++-
 src/bin/psql/common.c  |  5 +++--
 src/include/commands/explain.h |  3 ++-
 src/include/tcop/cmdtag.h  |  1 +
 src/include/tcop/cmdtaglist.h  |  3 +++
 src/interfaces/libpq/fe-exec.c |  4 +++-
 9 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 8570b14f62..453e545ba5 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -162,7 +162,7 @@ static void escape_yaml(StringInfo buf, const char *str);
  */
 void
 ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
-			 ParamListInfo params, DestReceiver *dest)
+			 ParamListInfo params, DestReceiver *dest, uint64 *processed)
 {
 	ExplainState *es = NewExplainState();
 	TupOutputState *tstate;
@@ -173,6 +173,9 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 	bool		timing_set = false;
 	bool		summary_set = false;
 
+	if (processed)
+		*processed = 0;
+
 	/* Parse options list. */
 	foreach(lc, stmt->options)
 	{
@@ -311,6 +314,9 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 	end_tup_output(tstate);
 
 	pfree(es->str->data);
+
+	if (processed)
+		*processed = es->es_processed;
 }
 
 /*
@@ -649,6 +655,8 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
 	 */
 	INSTR_TIME_SET_CURRENT(starttime);
 
+	es->es_processed += queryDesc->estate->es_processed;
+
 	ExecutorEnd(queryDesc);
 
 	FreeQueryDesc(queryDesc);
diff --git a/src/backend/tcop/cmdtag.c b/src/backend/tcop/cmdtag.c
index 4bd713a0b4..9e6fdbd8af 100644
--- a/src/backend/tcop/cmdtag.c
+++ b/src/backend/tcop/cmdtag.c
@@ -146,7 +146,7 @@ BuildQueryCompletionString(char *buff, const QueryCompletion *qc,
 	 */
 	if (command_tag_display_rowcount(tag) && !nameonly)
 	{
-		if (tag == CMDTAG_INSERT)
+		if (tag == CMDTAG_INSERT || tag == CMDTAG_EXPLAIN_INSERT)
 		{
 			*bufp++ = ' ';
 			*bufp++ = '0';
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 5565f200c3..ba0b33cc67 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -775,7 +775,13 @@ PortalRun(Portal portal, long count, bool isTopLevel, bool run_once,
 if (qc && portal->qc.commandTag != CMDTAG_UNKNOWN)
 {
 	CopyQueryCompletion(qc, >qc);
-	qc->nprocessed = nprocessed;
+	if (portal->qc.commandTag == CMDTAG_EXPLAIN ||
+		portal->qc.commandTag == CMDTAG_EXPLAIN_INSERT ||
+	

Re: [feature]COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns

[Damir Belyalov]

Hello!

The patch does not work for the current version of postgres, it needs to be
updated.
I tested your patch. Everything looks simple and works well.

There is a suggestion to simplify the code: instead of using

if (cstate->opts.force_notnull_all)
{
int i;
for(i = 0; i < num_phys_attrs; i++)
cstate->opt.force_notnull_flags[i] = true;
}

you can use MemSet():

if (cstate->opts.force_notnull_all)
MemSet(cstate->opt.force_notnull_flags, true, num_phys_attrs *
sizeof(bool));

The same for the force_null case.

Regards,
Damir Belyalov,
Postgres Professional

Re: Implement missing join selectivity estimation for range types

[Damir Belyalov]

Hello!

Thank you for the patch, very interesting article.
The patch doesn't apply to the current postgres version. Could you please
update it?

Regards,
Damir Belyalov,
Postgres Professional

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

>
> I might misunderstand something, but I believe the v5 patch uses
> copy_generic_opt_list and it does not add IGNORE_DATATYPE_ERRORS as a
> keyword.
> It modifies neither kwlist.h nor gram.y.
>

Sorry, didn't notice that. I think everything is alright now.

Regards,
Damir Belyalov
Postgres Professional

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Hi!

I made the specified changes and my patch turned out the same as yours. The
performance measurements were the same too.

The only thing left to do is how not to add IGNORE_DATATYPE_ERRORS as a
keyword. See how this is done for parameters such as FORCE_NOT_NULL,
FORCE_NULL, FORCE_QUOTE. They are not in kwlist.h and are not as keywords
in gram.y.

Regards,
Damir Belyalov
Postgres Professional

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

>
> FWIW, Greenplum has a similar construct (but which also logs the errors
> in the
> db) where data type errors are skipped as long as the number of errors
> don't
> exceed a reject limit.  If the reject limit is reached then the COPY
> fails:
> >
> >   LOG ERRORS [ SEGMENT REJECT LIMIT  [ ROWS | PERCENT ]]
> >
> IIRC the gist of this was to catch then the user copies the wrong input
> data or
> plain has a broken file.  Rather than finding out after copying n rows
> which
> are likely to be garbage the process can be restarted.
>

I think this is a matter for discussion. The same question is: "Where to
log errors to separate files or to the system logfile?".
IMO it's better for users to log short-detailed error message to system
logfile and not output errors to the terminal.


This version of the patch has a compiler error in the error message:
>
Yes, corrected it. Changed "ignored_errors" to int64 because "processed"
(used for counting copy rows) is int64.


I felt just logging "Error: %ld" would make people wonder the meaning of
> the %ld. Logging something like ""Error: %ld data type errors were
> found" might be clearer.
>

Thanks. For more clearance change the message to: "Errors were found: %".

Regards, Damir Belyalov
Postgres Professional
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index c25b52d0cb..706b929947 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { table_name [ ( format_name
 FREEZE [ boolean ]
+IGNORE_DATATYPE_ERRORS [ boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
 HEADER [ boolean | MATCH ]
@@ -233,6 +234,17 @@ COPY { table_name [ ( 

 
+   
+IGNORE_DATATYPE_ERRORS
+
+ 
+  Drops rows that contain malformed data while copying. These are rows
+  with columns where the data type's input-function raises an error.
+  Outputs warnings about rows with incorrect data to system logfile.
+ 
+
+   
+

 DELIMITER
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e34f583ea7..0334894014 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -410,6 +410,7 @@ ProcessCopyOptions(ParseState *pstate,
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
+	bool		ignore_datatype_errors_specified= false;
 	ListCell   *option;
 
 	/* Support external use for option sanity checking */
@@ -449,6 +450,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_datatype_errors") == 0)
+		{
+			if (ignore_datatype_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_datatype_errors_specified = true;
+			opts_out->ignore_datatype_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 29cd1cf4a6..facfc44def 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -949,10 +949,14 @@ CopyFrom(CopyFromState cstate)
 	errcallback.previous = error_context_stack;
 	error_context_stack = 
 
+	if (cstate->opts.ignore_datatype_errors)
+		cstate->ignored_errors = 0;
+
 	for (;;)
 	{
 		TupleTableSlot *myslot;
 		bool		skip_tuple;
+		ErrorSaveContext escontext = {T_ErrorSaveContext};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -985,9 +989,26 @@ CopyFrom(CopyFromState cstate)
 
 		ExecClearTuple(myslot);
 
+		if (cstate->opts.ignore_datatype_errors)
+		{
+			escontext.details_wanted = true;
+			cstate->escontext = escontext;
+		}
+
 		/* Directly store the values/nulls array in the slot */
 		if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
+		{
+			if (cstate->opts.ignore_datatype_errors && cstate->ignored_errors > 0)
+ereport(WARNING, errmsg("Errors were found: %lld", (long long) cstate->ignored_errors));
 			break;
+		}
+
+		/* Soft error occured, skip this tuple */
+		if (cstate->escontext.error_occurred)
+		{
+			ExecClearTuple(myslot);
+			continue;
+		}
 
 		ExecStoreVirtualTuple(myslot);
 
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 91b564c2bc..9c36b0dc8b 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -70,6 +70,7 @@
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
+#include "nodes/miscnodes.h"
 #include "pgstat.h"
 #include "port/pg_bswap.h"
 #include "utils/builtins.h"
@@ -938,10 +939,23 @@ NextCopyFrom(CopyFromState csta

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Hello

Tested patch on all cases: CIM_SINGLE, CIM_MULTI, CIM_MULTI_CONDITION. As
expected it works.
Also added a description to copy.sgml and made a review on patch.

I added 'ignored_errors' integer parameter that should be output after the
option is finished.
All errors were added to the system logfile with full detailed context.
Maybe it's better to log only error message.
file:///home/abc13/Documents/todo_copy/postgres/v2-0001-Add-COPY-option-IGNORE_DATATYPE_ERRORS.patch



Regards, Damir Belyalov
Postgres Professional
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index c25b52d0cb..706b929947 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { table_name [ ( format_name
 FREEZE [ boolean ]
+IGNORE_DATATYPE_ERRORS [ boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
 HEADER [ boolean | MATCH ]
@@ -233,6 +234,17 @@ COPY { table_name [ ( 

 
+   
+IGNORE_DATATYPE_ERRORS
+
+ 
+  Drops rows that contain malformed data while copying. These are rows
+  with columns where the data type's input-function raises an error.
+  Outputs warnings about rows with incorrect data to system logfile.
+ 
+
+   
+

 DELIMITER
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e34f583ea7..0334894014 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -410,6 +410,7 @@ ProcessCopyOptions(ParseState *pstate,
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
+	bool		ignore_datatype_errors_specified= false;
 	ListCell   *option;
 
 	/* Support external use for option sanity checking */
@@ -449,6 +450,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_datatype_errors") == 0)
+		{
+			if (ignore_datatype_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_datatype_errors_specified = true;
+			opts_out->ignore_datatype_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index af52faca6d..ecaa750568 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -955,10 +955,14 @@ CopyFrom(CopyFromState cstate)
 	errcallback.previous = error_context_stack;
 	error_context_stack = 
 
+	if (cstate->opts.ignore_datatype_errors)
+		cstate->ignored_errors = 0;
+
 	for (;;)
 	{
 		TupleTableSlot *myslot;
 		bool		skip_tuple;
+		ErrorSaveContext escontext = {T_ErrorSaveContext};
 
 		CHECK_FOR_INTERRUPTS();
 
@@ -991,9 +995,26 @@ CopyFrom(CopyFromState cstate)
 
 		ExecClearTuple(myslot);
 
+		if (cstate->opts.ignore_datatype_errors)
+		{
+			escontext.details_wanted = true;
+			cstate->escontext = escontext;
+		}
+
 		/* Directly store the values/nulls array in the slot */
 		if (!NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull))
+		{
+			if (cstate->opts.ignore_datatype_errors && cstate->ignored_errors > 0)
+ereport(WARNING, errmsg("Errors: %ld", cstate->ignored_errors));
 			break;
+		}
+
+		/* Soft error occured, skip this tuple */
+		if (cstate->escontext.error_occurred)
+		{
+			ExecClearTuple(myslot);
+			continue;
+		}
 
 		ExecStoreVirtualTuple(myslot);
 
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 91b564c2bc..9c36b0dc8b 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -70,6 +70,7 @@
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
+#include "nodes/miscnodes.h"
 #include "pgstat.h"
 #include "port/pg_bswap.h"
 #include "utils/builtins.h"
@@ -938,10 +939,23 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
 
 			cstate->cur_attname = NameStr(att->attname);
 			cstate->cur_attval = string;
-			values[m] = InputFunctionCall(_functions[m],
-		  string,
-		  typioparams[m],
-		  att->atttypmod);
+
+			/* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype errors */
+			if (!InputFunctionCallSafe(_functions[m],
+	   string,
+	   typioparams[m],
+	   att->atttypmod,
+	   (Node *) >escontext,
+	   [m]))
+			{
+cstate->ignored_errors++;
+
+ereport(LOG,
+		errmsg("%s", cstate->escontext.error_data->message));
+
+return true;
+			}
+
 			if (string != NULL)
 nulls[m] = false;
 			cstate->cur_attname = NULL;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a0138382a1..d79d293c0d 100644
--- a/src/backend/parser/gram.y
++

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Hi, Andres!

Thank you for reviewing.


> I don't think this is the right approach. Creating a subtransaction for
> each row will cause substantial performance issues.
>

Subtransactions aren't created for each row. The block of rows in one
subtransaction is 1000 (SAFE_BUFFER_SIZE) and can be changed. There is also
a constraint for the number of bytes MAX_SAFE_BUFFER_BYTES in safe_buffer:
 while (sfcstate->saved_tuples < SAFE_BUFFER_SIZE &&
 sfcstate->safeBufferBytes < MAX_SAFE_BUFFER_BYTES)



We now can call data type input functions without throwing errors, see
> InputFunctionCallSafe(). Use that to avoid throwing an error instead of
> catching it.
>
InputFunctionCallSafe() is good for detecting errors from input-functions
but there are such errors from NextCopyFrom () that can not be detected
with InputFunctionCallSafe(), e.g. "wrong number of columns in row''. Do
you offer to process input-function errors separately from other errors?
Now all errors are processed in one "switch" loop in PG_CATCH, so this
change can complicate code.



Regards,
Damir Belyalov
Postgres Professional

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Hi, Danil and Nikita!
Thank you for reviewing.

Why is there no static keyword in the definition of the SafeCopying()
> function, but it is in the function declaration.
>
Correct this.

675: MemoryContext cxt =
> MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
> 676:
> 677: valid_row = NextCopyFrom(cstate, econtext, myslot->tts_values,
> myslot->tts_isnull);
> 678: tuple_is_valid = valid_row;
> 679:
> 680: if (valid_row)
> 681: sfcstate->safeBufferBytes += cstate->line_buf.len;
> 682:
> 683: CurrentMemoryContext = cxt;
>
> Why are you using a direct assignment to CurrentMemoryContext instead of
> using the MemoryContextSwitchTo function in the SafeCopying() routine?
>

"CurrentMemoryContext = cxt" is the same as "MemoryContextSwitchTo(cxt)",
you can see it in the implementation of MemoryContextSwitchTo(). Also
correct this.


When you initialize the cstate->sfcstate structure, you create a
> cstate->sfcstate->safe_cxt memory context that inherits from oldcontext.
> Was it intended to use cstate->copycontext as the parent context here?
>

Good remark, correct this.



Thanks Nikita Malakhov for advice to create file with errors. But I decided
to to log errors in the system logfile and don't print them to the
terminal. The error's output in logfile is rather simple - only error
context logs (maybe it's better to log all error information?).
There are 2 points why logging errors in logfile is better than logging
errors in another file (e.g. PGDATA/copy_ignore_errors.txt). The user is
used to looking for errors in logfile. Creating another file entails
problems like: 'what file name to create?', 'do we need to make file
rotation?', 'where does this file create?' (we can't create it in PGDATA
cause of memory constraints)



Regards,
Damir Belyalov
Postgres Professional
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index c25b52d0cb..50151aec54 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { table_name [ ( format_name
 FREEZE [ boolean ]
+IGNORE_ERRORS [ boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
 HEADER [ boolean | MATCH ]
@@ -233,6 +234,18 @@ COPY { table_name [ ( 

 
+   
+IGNORE_ERRORS
+
+ 
+  Drops rows that contain malformed data while copying. These are rows
+  containing syntax errors in data, rows with too many or too few columns,
+  rows containing columns where the data type's input function raises an error.
+  Logs errors to system logfile and outputs the total number of errors.
+ 
+
+   
+

 DELIMITER
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e34f583ea7..e741ce3e5a 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -407,6 +407,7 @@ ProcessCopyOptions(ParseState *pstate,
    bool is_from,
    List *options)
 {
+	bool		ignore_errors_specified = false;
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
@@ -449,6 +450,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_errors") == 0)
+		{
+			if (ignore_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_errors_specified = true;
+			opts_out->ignore_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index af52faca6d..657fa44e0b 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -107,6 +107,9 @@ static char *limit_printout_length(const char *str);
 
 static void ClosePipeFromProgram(CopyFromState cstate);
 
+static bool SafeCopying(CopyFromState cstate, ExprContext *econtext,
+		TupleTableSlot *myslot);
+
 /*
  * error context callback for COPY FROM
  *
@@ -625,6 +628,173 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
 	miinfo->bufferedBytes += tuplen;
 }
 
+/*
+ * Safely reads source data, converts to a tuple and fills tuple buffer.
+ * Skips some data in the case of failed conversion if data source for
+ * a next tuple can be surely read without a danger.
+ */
+static bool
+SafeCopying(CopyFromState cstate, ExprContext *econtext, TupleTableSlot *myslot)
+{
+	SafeCopyFromState  *sfcstate = cstate->sfcstate;
+	bool valid_row = true;
+
+	/* Standard COPY if IGNORE_ERRORS is disabled */
+	if (!cstate->sfcstate)
+		/* Directly stores the values/nulls array in the slot */
+		return NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull);
+
+	if (sfcstate->replayed_tuples < sfcstate->saved_tuples)
+	{
+		Assert(sfcstate->saved_tuple

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Updated the patch:
- Optimized and simplified logic of IGNORE_ERRORS
- Changed variable names to more understandable ones
- Added an analogue of MAX_BUFFERED_BYTES for safe buffer


Regards,
Damir Belyalov
Postgres Professional

>
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index c25b52d0cb..22c992e6f6 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { table_name [ ( format_name
 FREEZE [ boolean ]
+IGNORE_ERRORS [ boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
 HEADER [ boolean | MATCH ]
@@ -233,6 +234,19 @@ COPY { table_name [ ( 

 
+   
+IGNORE_ERRORS
+
+ 
+  Drops rows that contain malformed data while copying. These are rows
+  containing syntax errors in data, rows with too many or too few columns,
+  rows containing columns where the data type's input function raises an error.
+  Outputs warnings about rows with incorrect data (the number of warnings
+  is not more than 100) and the total number of errors.
+ 
+
+   
+

 DELIMITER
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index db4c9dbc23..d04753a4c8 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -406,6 +406,7 @@ ProcessCopyOptions(ParseState *pstate,
    bool is_from,
    List *options)
 {
+	bool		ignore_errors_specified = false;
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
@@ -448,6 +449,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_errors") == 0)
+		{
+			if (ignore_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_errors_specified = true;
+			opts_out->ignore_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index a079c70152..846eac022d 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -107,6 +107,9 @@ static char *limit_printout_length(const char *str);
 
 static void ClosePipeFromProgram(CopyFromState cstate);
 
+static bool SafeCopying(CopyFromState cstate, ExprContext *econtext,
+			 TupleTableSlot *myslot);
+
 /*
  * error context callback for COPY FROM
  *
@@ -625,6 +628,175 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
 	miinfo->bufferedBytes += tuplen;
 }
 
+/*
+ * Safely reads source data, converts to a tuple and fills tuple buffer.
+ * Skips some data in the case of failed conversion if data source for
+ * a next tuple can be surely read without a danger.
+ */
+bool
+SafeCopying(CopyFromState cstate, ExprContext *econtext, TupleTableSlot *myslot)
+{
+	SafeCopyFromState  *sfcstate = cstate->sfcstate;
+	bool valid_row = true;
+
+	/* Standard COPY if IGNORE_ERRORS is disabled */
+	if (!cstate->sfcstate)
+		/* Directly stores the values/nulls array in the slot */
+		return NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull);
+
+	if (sfcstate->replayed_tuples < sfcstate->saved_tuples)
+	{
+		Assert(sfcstate->saved_tuples > 0);
+
+		/* Prepare to replay the tuple */
+		heap_deform_tuple(sfcstate->safe_buffer[sfcstate->replayed_tuples++], RelationGetDescr(cstate->rel),
+		  myslot->tts_values, myslot->tts_isnull);
+		return true;
+	}
+	else
+	{
+		/* All tuples from buffer were replayed, clean it up */
+		MemoryContextReset(sfcstate->safe_cxt);
+
+		sfcstate->saved_tuples = sfcstate->replayed_tuples = 0;
+		sfcstate->safeBufferBytes = 0;
+	}
+
+	BeginInternalSubTransaction(NULL);
+	CurrentResourceOwner = sfcstate->oldowner;
+
+	while (sfcstate->saved_tuples < SAFE_BUFFER_SIZE &&
+		   sfcstate->safeBufferBytes < MAX_SAFE_BUFFER_BYTES)
+	{
+		bool	tuple_is_valid = true;
+
+		PG_TRY();
+		{
+			MemoryContext cxt = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
+
+			valid_row = NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull);
+			tuple_is_valid = valid_row;
+
+			if (valid_row)
+sfcstate->safeBufferBytes += cstate->line_buf.len;
+
+			CurrentMemoryContext = cxt;
+		}
+		PG_CATCH();
+		{
+			MemoryContext ecxt = MemoryContextSwitchTo(sfcstate->oldcontext);
+			ErrorData 	 *errdata = CopyErrorData();
+
+			tuple_is_valid = false;
+
+			Assert(IsSubTransaction());
+
+			RollbackAndReleaseCurrentSubTransaction();
+			CurrentResourceOwner = sfcstate->oldowner;
+
+			switch (errdata->sqlerrcode)
+			{
+/* Ignore data exceptions */
+case ERRCODE_CHARACTER_NOT_IN_REPERTOIRE:
+case ERRCODE_DATA_EXCEPTION:
+case ERRCODE_ARRAY_ELEMENT_ERROR:
+case ERRCODE_DATETIME_VALUE_OUT_OF_RANGE:
+

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Updated the patch due to conflicts when applying to master.

>
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index c25b52d0cb..22c992e6f6 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { table_name [ ( format_name
 FREEZE [ boolean ]
+IGNORE_ERRORS [ boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
 HEADER [ boolean | MATCH ]
@@ -233,6 +234,19 @@ COPY { table_name [ ( 

 
+   
+IGNORE_ERRORS
+
+ 
+  Drops rows that contain malformed data while copying. These are rows
+  containing syntax errors in data, rows with too many or too few columns,
+  rows containing columns where the data type's input function raises an error.
+  Outputs warnings about rows with incorrect data (the number of warnings
+  is not more than 100) and the total number of errors.
+ 
+
+   
+

 DELIMITER
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index db4c9dbc23..d04753a4c8 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -406,6 +406,7 @@ ProcessCopyOptions(ParseState *pstate,
    bool is_from,
    List *options)
 {
+	bool		ignore_errors_specified = false;
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
@@ -448,6 +449,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_errors") == 0)
+		{
+			if (ignore_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_errors_specified = true;
+			opts_out->ignore_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index a079c70152..fa169d2cf4 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -107,6 +107,9 @@ static char *limit_printout_length(const char *str);
 
 static void ClosePipeFromProgram(CopyFromState cstate);
 
+static bool FillReplayBuffer(CopyFromState cstate, ExprContext *econtext,
+			 TupleTableSlot *myslot);
+
 /*
  * error context callback for COPY FROM
  *
@@ -625,6 +628,177 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
 	miinfo->bufferedBytes += tuplen;
 }
 
+/*
+ * Fills replay_buffer for safe copying, enables by IGNORE_ERRORS option.
+ */
+bool
+FillReplayBuffer(CopyFromState cstate, ExprContext *econtext, TupleTableSlot *myslot)
+{
+	SafeCopyFromState  *sfcstate = cstate->sfcstate;
+	bool valid_row = true;
+
+	if (!sfcstate->replay_is_active)
+	{
+		BeginInternalSubTransaction(NULL);
+		CurrentResourceOwner = sfcstate->oldowner;
+
+		while (sfcstate->saved_tuples < REPLAY_BUFFER_SIZE)
+		{
+			bool tuple_is_valid = false;
+
+			PG_TRY();
+			{
+MemoryContext cxt = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
+
+valid_row = NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull);
+
+if (valid_row)
+	tuple_is_valid = true;
+
+CurrentMemoryContext = cxt;
+			}
+			PG_CATCH();
+			{
+MemoryContext ecxt = MemoryContextSwitchTo(sfcstate->oldcontext);
+ErrorData *errdata = CopyErrorData();
+
+Assert(IsSubTransaction());
+RollbackAndReleaseCurrentSubTransaction();
+CurrentResourceOwner = sfcstate->oldowner;
+
+switch (errdata->sqlerrcode)
+{
+	/* Ignore data exceptions */
+	case ERRCODE_CHARACTER_NOT_IN_REPERTOIRE:
+	case ERRCODE_DATA_EXCEPTION:
+	case ERRCODE_ARRAY_ELEMENT_ERROR:
+	case ERRCODE_DATETIME_VALUE_OUT_OF_RANGE:
+	case ERRCODE_INTERVAL_FIELD_OVERFLOW:
+	case ERRCODE_INVALID_CHARACTER_VALUE_FOR_CAST:
+	case ERRCODE_INVALID_DATETIME_FORMAT:
+	case ERRCODE_INVALID_ESCAPE_CHARACTER:
+	case ERRCODE_INVALID_ESCAPE_SEQUENCE:
+	case ERRCODE_NONSTANDARD_USE_OF_ESCAPE_CHARACTER:
+	case ERRCODE_INVALID_PARAMETER_VALUE:
+	case ERRCODE_INVALID_TABLESAMPLE_ARGUMENT:
+	case ERRCODE_INVALID_TIME_ZONE_DISPLACEMENT_VALUE:
+	case ERRCODE_NULL_VALUE_NOT_ALLOWED:
+	case ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE:
+	case ERRCODE_SEQUENCE_GENERATOR_LIMIT_EXCEEDED:
+	case ERRCODE_STRING_DATA_LENGTH_MISMATCH:
+	case ERRCODE_STRING_DATA_RIGHT_TRUNCATION:
+	case ERRCODE_INVALID_TEXT_REPRESENTATION:
+	case ERRCODE_INVALID_BINARY_REPRESENTATION:
+	case ERRCODE_BAD_COPY_FILE_FORMAT:
+	case ERRCODE_UNTRANSLATABLE_CHARACTER:
+	case ERRCODE_DUPLICATE_JSON_OBJECT_KEY_VALUE:
+	case ERRCODE_INVALID_ARGUMENT_FOR_SQL_JSON_DATETIME_FUNCTION:
+	case ERRCODE_INVALID_JSON_TEXT:
+	case ERRCODE_INVALID_SQL_JSON_SUBSCRIPT:
+	case ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM:
+	case ERRCODE_NO_SQL_JSON_ITEM:
+	case ERRCODE_NON_NUMERIC_SQL_JSON_ITEM:
+	case ERRCODE_NON_UNIQUE_KEYS_IN_A_JSON_OBJECT:
+	case 

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

>
> Do you mean you stop dealing with errors concerned with constraints and
> triggers and we should review 0006-COPY_IGNORE_ERRORS?
>
Yes, this patch is simpler and I think it's worth adding it first.


> Hmm, I applied v6 patch and when canceled COPY by sending SIGINT(ctrl +
> C), I faced the same situation as below.
> I tested it on CentOS 8.4.
>
Thank you for pointing out this error. it really needs to be taken into
account. In the previous  0006 patch, there were 2 stages in one
subtransaction - filling the buffer and 'replay mode' (reading from the
buffer). Since only NextCopyFrom() is included in PG_TRY(), and the
ERRCODE_QUERY_CANCELED error can occur anywhere, it is impossible to catch
it and rollback the subtransaction.

I changed the 0006 patch and fixed this error and now only the 'replay
buffer filling' is made in the subtransaction.

Patch 0005 (that processed constraints) needs to be finalized, because it
requires subtransactions to rollback constraints, triggers. Therefore, it
is not possible to fix it yet. There is a decision to put for(;;) loop in
PG_TRY. It will solve the problem, but the code will be too complicated.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index c25b52d0cb..22c992e6f6 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { table_name [ ( format_name
 FREEZE [ boolean ]
+IGNORE_ERRORS [ boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
 HEADER [ boolean | MATCH ]
@@ -233,6 +234,19 @@ COPY { table_name [ ( 

 
+   
+IGNORE_ERRORS
+
+ 
+  Drops rows that contain malformed data while copying. These are rows
+  containing syntax errors in data, rows with too many or too few columns,
+  rows containing columns where the data type's input function raises an error.
+  Outputs warnings about rows with incorrect data (the number of warnings
+  is not more than 100) and the total number of errors.
+ 
+
+   
+

 DELIMITER
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 49924e476a..f41b25f26a 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -406,6 +406,7 @@ ProcessCopyOptions(ParseState *pstate,
    bool is_from,
    List *options)
 {
+	bool		ignore_errors_specified = false;
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
@@ -448,6 +449,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_errors") == 0)
+		{
+			if (ignore_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_errors_specified = true;
+			opts_out->ignore_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index e8bb168aea..caa3375758 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -106,6 +106,9 @@ static char *limit_printout_length(const char *str);
 
 static void ClosePipeFromProgram(CopyFromState cstate);
 
+static bool FillReplayBuffer(CopyFromState cstate, ExprContext *econtext,
+			 TupleTableSlot *myslot);
+
 /*
  * error context callback for COPY FROM
  *
@@ -521,6 +524,177 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
 	miinfo->bufferedBytes += tuplen;
 }
 
+/*
+ * Fills replay_buffer for safe copying, enables by IGNORE_ERRORS option.
+ */
+bool
+FillReplayBuffer(CopyFromState cstate, ExprContext *econtext, TupleTableSlot *myslot)
+{
+	SafeCopyFromState  *sfcstate = cstate->sfcstate;
+	bool valid_row = true;
+
+	if (!sfcstate->replay_is_active)
+	{
+		BeginInternalSubTransaction(NULL);
+		CurrentResourceOwner = sfcstate->oldowner;
+
+		while (sfcstate->saved_tuples < REPLAY_BUFFER_SIZE)
+		{
+			bool tuple_is_valid = false;
+
+			PG_TRY();
+			{
+MemoryContext cxt = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
+
+valid_row = NextCopyFrom(cstate, econtext, myslot->tts_values, myslot->tts_isnull);
+
+if (valid_row)
+	tuple_is_valid = true;
+
+CurrentMemoryContext = cxt;
+			}
+			PG_CATCH();
+			{
+MemoryContext ecxt = MemoryContextSwitchTo(sfcstate->oldcontext);
+ErrorData *errdata = CopyErrorData();
+
+Assert(IsSubTransaction());
+RollbackAndReleaseCurrentSubTransaction();
+CurrentResourceOwner = sfcstate->oldowner;
+
+switch (errdata->sqlerrcode)
+{
+	/* Ignore data exceptions */
+	case ERRCODE_CHARACTER_NOT_IN_REPERTOIRE:
+	case ERRCODE_DATA_EXCEPTION:
+	case ERRCODE_ARRAY_ELEMENT_ERROR:
+	case ERRCODE_DATETIME_VALUE_OUT_OF_RANGE:
+	case ERRCODE_INTERVAL_FIELD_OVERFLOW:
+	case ERRCODE_INVALID_CHARACTER_VALUE_FOR_CAST:
+	case ERRCODE_INVALID_DATETIME_FORMAT:
+	case 

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Thank you for reviewing.
In the previous patch there was an error when processing constraints. The
patch was fixed, but the code grew up and became more complicated
(0005-COPY_IGNORE_ERRORS). I also simplified the logic of
safeNextCopyFrom().
You asked why we need subtransactions, so the answer is in this patch. When
processing a row that does not satisfy constraints or INSTEAD OF triggers,
it is necessary to rollback the subtransaction and return the table to its
original state.
Cause of complexity, I had to abandon the constraints, triggers processing
in and handle only errors that occur when reading the file. Attaching
simplified patch (0006-COPY_IGNORE_ERRORS).
Checked out these patches on all COPY regress tests and it worked correctly.


> BTW in v4 patch, data are loaded into the buffer one by one, and when
> the buffer fills up, the data in the buffer are 'replayed' also one by
> one, right?
> Wouldn't this have more overhead than a normal COPY?
>
The data is loaded into the buffer one by one, but in the "replay mode"
ignore_errors works as standard COPY. Tuples add to the slot and depending
on the type of the slot (CIM_SINGLE or CIM_MULTI) tuples are replayed in
the corresponding case.
For the 0006 patch you can imagine that we divide the loop for(;;) in 2
parts. The first part is adding tuples to the buffer and the second part is
inserting tuples to the table. These parts don't intersect with each other
and are completed sequentially.
The main idea of replay_buffer is that it is needed for the CIM_SINGLE
case. You can implement the CIM_SINGLE case and see that tuples before an
error occurring don't add to the table. Logic of the 0005 patch is similar
but with some differences.

As a test, I COPYed slightly larger data with and without ignore_errors
> option.
> There might be other reasons, but I found a performance difference.

Tried to reduce performance difference with cleaning up replay_buffer with
resetting the new context for replay_buffer - replay_cxt.
```
Before:
Without ignore_errors:
COPY 1000
Time: 15538,579 ms (00:15,539)
With ignore_errors:
COPY 1000
Time: 21289,121 ms (00:21,289)

After:
Without ignore_errors:
COPY 1000
Time: 15318,922 ms (00:15,319)
With ignore_errors:
COPY 1000
Time: 19868,175 ms (00:19,868)
```

 - Put in the documentation that the warnings will not be output for more
> than 101 cases.
>
Yeah, I point it out in the doc.


> I applied v4 patch and when canceled the COPY, there was a case I found
> myself left in a transaction.
> Should this situation be prevented from occurring?
>
> ```
> =# copy test from '/tmp/1000.data' with (ignore_errors );
>
> ^CCancel request sent
> ERROR:  canceling statement due to user request
>
> =# truncate test;
> ERROR:  current transaction is aborted, commands ignored until end of
> transaction block
> ```
>
Tried to implement your error and could not. The result was the same as
COPY FROM implements.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index c25b52d0cb..c99adabcc9 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { table_name [ ( format_name
 FREEZE [ boolean ]
+IGNORE_ERRORS [ boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
 HEADER [ boolean | MATCH ]
@@ -233,6 +234,20 @@ COPY { table_name [ ( 

 
+   
+IGNORE_ERRORS
+
+ 
+  Drops rows that contain malformed data while copying. These are rows
+  containing syntax errors in data, rows with too many or too few columns,
+  rows that result in constraint violations, rows containing columns where
+  the data type's input function raises an error.
+  Outputs warnings about rows with incorrect data (the number of warnings
+  is not more than 100) and the total number of errors.
+ 
+
+   
+

 DELIMITER
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 49924e476a..f41b25f26a 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -406,6 +406,7 @@ ProcessCopyOptions(ParseState *pstate,
    bool is_from,
    List *options)
 {
+	bool		ignore_errors_specified = false;
 	bool		format_specified = false;
 	bool		freeze_specified = false;
 	bool		header_specified = false;
@@ -448,6 +449,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_errors") == 0)
+		{
+			if (ignore_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_errors_specified = true;
+			opts_out->ignore_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index e8bb168aea..6474d47d29 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -106,6 +106,12 @@ static char 

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

>
From 09befdad45a6b1ae70d6c5abc90d1c2296e56ee1 Mon Sep 17 00:00:00 2001
From: Damir Belyalov 
Date: Fri, 15 Oct 2021 11:55:18 +0300
Subject: [PATCH] COPY_IGNORE_ERRORS with GUC for replay_buffer size

---
 doc/src/sgml/config.sgml  |  17 ++
 doc/src/sgml/ref/copy.sgml|  19 ++
 src/backend/commands/copy.c   |   8 +
 src/backend/commands/copyfrom.c   | 114 +++-
 src/backend/commands/copyfromparse.c  | 169 ++
 src/backend/parser/gram.y |   8 +-
 src/backend/utils/misc/guc.c  |  11 ++
 src/backend/utils/misc/postgresql.conf.sample |   2 +
 src/bin/psql/tab-complete.c   |   3 +-
 src/include/commands/copy.h   |   6 +
 src/include/commands/copyfrom_internal.h  |  19 ++
 src/include/parser/kwlist.h   |   1 +
 src/test/regress/expected/copy2.out   | 130 ++
 src/test/regress/sql/copy2.sql| 116 
 14 files changed, 617 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 37fd80388c..69373b8d8c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1961,6 +1961,23 @@ include_dir 'conf.d'
   
  
 
+ 
+  replay_buffer_size (integer)
+  
+   replay_buffer_size configuration parameter
+  
+  
+  
+   
+Specifies the size of buffer for COPY FROM IGNORE_ERRORS option. This buffer
+is created when subtransaction begins and accumulates tuples until an error
+occurs. Then it starts replaying stored tuples. the buffer size is the size
+of the subtransaction. Therefore, on large tables, in order to avoid the
+error of the maximum number of subtransactions, it should be increased.
+   
+  
+ 
+
  
   max_stack_depth (integer)
   
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 8aae711b3b..7ff6f6dea7 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { table_name [ ( format_name
 FREEZE [ boolean ]
+IGNORE_ERRORS [ boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
 HEADER [ boolean | MATCH ]
@@ -233,6 +234,24 @@ COPY { table_name [ ( 

 
+   
+IGNORE_ERRORS
+
+ 
+  Drop rows that contain malformed data while copying. These are rows
+  containing syntax errors in data, rows with too many or too few columns,
+  rows that result in constraint violations, rows containing columns where
+  the data type's input function raises an error.
+  Outputs warnings about rows with incorrect data (the number of warnings
+  is not more than 100) and the total number of errors.
+  The option is implemented with subtransactions and a buffer created in
+  them, which accumulates tuples until an error occures.
+  The size of buffer is the size of subtransaction block.
+  It is a GUC parameter "replay_buffer_size" and equals 1000 by default.
+ 
+
+   
+

 DELIMITER
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3ac731803b..fead1aba46 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -402,6 +402,7 @@ ProcessCopyOptions(ParseState *pstate,
 {
 	bool		format_specified = false;
 	bool		freeze_specified = false;
+	bool		ignore_errors_specified = false;
 	bool		header_specified = false;
 	ListCell   *option;
 
@@ -442,6 +443,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_errors") == 0)
+		{
+			if (ignore_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_errors_specified = true;
+			opts_out->ignore_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index a976008b3d..7e997d15c6 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -73,6 +73,11 @@
 /* Trim the list of buffers back down to this number after flushing */
 #define MAX_PARTITION_BUFFERS	32
 
+/*
+ * GUC parameters
+ */
+int		replay_buffer_size;
+
 /* Stores multi-insert data related to a single relation in CopyFrom. */
 typedef struct CopyMultiInsertBuffer
 {
@@ -100,12 +105,13 @@ typedef struct CopyMultiInsertInfo
 	int			ti_options;		/* table insert options */
 } CopyMultiInsertInfo;
 
-
 /* non-export function prototypes */
 static char *limit_printout_length(const char *str);
 
 static void ClosePipeFromProgram(CopyFromState cstate);
 
+static void safeExecConstraints(CopyFromState cstate, ResultRelInfo *resultRelInfo, TupleTableSlot *myslot, EState *estate);
+
 /*
  * error cont

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

>
> > +   /* Buffer was filled, commit subtransaction and prepare
> to replay */
> > +   ReleaseCurrentSubTransaction();
> What is actually being committed by this ReleaseCurrentSubTransaction()?
> It seems to me that just safecstate->replay_buffer is fulfilled before
> this commit.
>
All tuples are collected in replay_buffer, which is created in
''oldcontext'' of CopyFrom() (not context of a subtransaction). That's why
it didn't clean up when you used RollbackAndReleaseCurrentSubTransaction().
Subtransactions are needed for better safety. There is no error when
copying from a file to the replay_buffer, but an error may occur at the
next stage - when adding a tuple to the table. Also there may be other
errors that are not obvious at first glance.

I feel it might be better to have it as a parameter rather than fixed at
> 1000.
>
Yes, I thought about it too. So I created another version with the GUC
parameter - replay_buffer_size. Attached it. But I think users won't need
to change replay_buffer_size.
Also replay_buffer does the same thing as MultiInsert buffer does and
MultiInsert buffer defined by const = 1000.

NextCopyFrom() is in copyfromparse.c while safeNextCopyFrom() is in
> copyfrom.c.
> Since safeNextCopyFrom() is analog of NextCopyFrom() as commented, would
> it be natural to put them in the same file?
>
Sure, corrected it.


> > 188 +   safecstate->errors++;
> > 189 +   if (safecstate->errors <= 100)
> > 190 +   ereport(WARNING,
> > 191 +   (errcode(errdata->sqlerrcode),
> > 192 +   errmsg("%s", errdata->context)));
>
> It would be better to state in the manual that errors exceeding 100 are
> not displayed.
> Or, it might be acceptable to output all errors that exceed 100.
>
It'll be too complicated to create a new parameter just for showing the
given number of errors. I think 100 is an optimal size.


> > +typedef struct SafeCopyFromState
> > +{
> > +#defineREPLAY_BUFFER_SIZE 1000
> > +   HeapTuple   replay_buffer[REPLAY_BUFFER_SIZE]; /* accumulates
> > tuples for replaying it after an error */
> > +   int saved_tuples;   /* # of tuples in
> > replay_buffer */
> > +   int replayed_tuples;/* # of tuples was
> > replayed from buffer */
> > +   int errors; /* total # of errors */
> > +   boolreplay_is_active;
> > +   boolbegin_subtransaction;
> > +   boolprocessed_remaining_tuples; /* for case of
> > replaying last tuples */
> > +   boolskip_row;
>
> It would be helpful to add comments about skip_row, etc.
>
Corrected it.


> WARNING:  FIND 0 ERRORS
> When there were no errors, this WARNING seems not necessary.
>
Corrected it.

Add to this patch processing other errors and constraints and tests for
them.
I had to create another function safeExecConstraints() only for processing
constraints, because ExecConstraints() is after NextCopyFrom() and is not
in PG_TRY. This thing a little bit complicated the code.
Maybe it is a good approach to create a new function SafeCopyFrom() and do
all ''safe copying'' in PG_TRY, but it will almost duplicate the CopyFrom()
code.
Or maybe create a function only for loop for(;;). But we have the same
thing with duplicating code and passing a lot of variables (which are
created at the beginning of CopyFrom()) to this function.

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

>
>
> Thank you for feedback.
I improved my patch recently and tested it on different sizes of
MAX_BUFFERED_TUPLES and REPLAY_BUFFER_SIZE.

> I loaded 1 rows which contained 1 wrong row.
> I expected I could see  rows after COPY, but just saw 999 rows.
Also I implemented your case and it worked correctly.

> BTW I may be overlooking it, but have you submit this proposal to the
next CommitFest?
Good idea. Haven't done it yet.

Regards,
Damir
Postgres Professional
From fa6b99c129eb890b25f006bb7891a247c8a431a7 Mon Sep 17 00:00:00 2001
From: Damir Belyalov 
Date: Fri, 15 Oct 2021 11:55:18 +0300
Subject: [PATCH] COPY_IGNORE_ERRORS without GUC with function
 safeNextCopyFrom() with struct SafeCopyFromState with refactoring

---
 doc/src/sgml/ref/copy.sgml   |  13 ++
 src/backend/commands/copy.c  |   8 ++
 src/backend/commands/copyfrom.c  | 162 ++-
 src/backend/parser/gram.y|   8 +-
 src/bin/psql/tab-complete.c  |   3 +-
 src/include/commands/copy.h  |   1 +
 src/include/commands/copyfrom_internal.h |  21 +++
 src/include/parser/kwlist.h  |   1 +
 src/test/regress/expected/copy2.out  | 123 +
 src/test/regress/sql/copy2.sql   | 110 +++
 10 files changed, 445 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 8aae711b3b..7d20b1649e 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -34,6 +34,7 @@ COPY { table_name [ ( format_name
 FREEZE [ boolean ]
+IGNORE_ERRORS [ boolean ]
 DELIMITER 'delimiter_character'
 NULL 'null_string'
 HEADER [ boolean | MATCH ]
@@ -233,6 +234,18 @@ COPY { table_name [ ( 

 
+   
+IGNORE_ERRORS
+
+ 
+  Drop rows that contain malformed data while copying. That is rows
+  containing syntax errors in data, rows with too many or too few columns,
+  rows that result in constraint violations, rows containing columns where
+  the data type's input function raises an error.
+ 
+
+   
+

 DELIMITER
 
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 3ac731803b..fead1aba46 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -402,6 +402,7 @@ ProcessCopyOptions(ParseState *pstate,
 {
 	bool		format_specified = false;
 	bool		freeze_specified = false;
+	bool		ignore_errors_specified = false;
 	bool		header_specified = false;
 	ListCell   *option;
 
@@ -442,6 +443,13 @@ ProcessCopyOptions(ParseState *pstate,
 			freeze_specified = true;
 			opts_out->freeze = defGetBoolean(defel);
 		}
+		else if (strcmp(defel->defname, "ignore_errors") == 0)
+		{
+			if (ignore_errors_specified)
+errorConflictingDefElem(defel, pstate);
+			ignore_errors_specified = true;
+			opts_out->ignore_errors = defGetBoolean(defel);
+		}
 		else if (strcmp(defel->defname, "delimiter") == 0)
 		{
 			if (opts_out->delim)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index a976008b3d..285c491ddd 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -106,6 +106,9 @@ static char *limit_printout_length(const char *str);
 
 static void ClosePipeFromProgram(CopyFromState cstate);
 
+static bool safeNextCopyFrom(CopyFromState cstate, ExprContext *econtext,
+	 Datum *values, bool *nulls);
+
 /*
  * error context callback for COPY FROM
  *
@@ -521,6 +524,125 @@ CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
 	miinfo->bufferedBytes += tuplen;
 }
 
+/*
+ * Analog of NextCopyFrom() but ignore rows with errors while copying.
+ */
+static bool
+safeNextCopyFrom(CopyFromState cstate, ExprContext *econtext, Datum *values, bool *nulls)
+{
+	SafeCopyFromState *safecstate = cstate->safecstate;
+	bool valid_row = true;
+
+	safecstate->skip_row = false;
+
+	PG_TRY();
+	{
+		if (!safecstate->replay_is_active)
+		{
+			if (safecstate->begin_subtransaction)
+			{
+BeginInternalSubTransaction(NULL);
+CurrentResourceOwner = safecstate->oldowner;
+
+safecstate->begin_subtransaction = false;
+			}
+
+			if (safecstate->saved_tuples < REPLAY_BUFFER_SIZE)
+			{
+valid_row = NextCopyFrom(cstate, econtext, values, nulls);
+if (valid_row)
+{
+	/* Fill replay_buffer in oldcontext*/
+	MemoryContextSwitchTo(safecstate->oldcontext);
+	safecstate->replay_buffer[safecstate->saved_tuples++] = heap_form_tuple(RelationGetDescr(cstate->rel), values, nulls);
+
+	safecstate->skip_row = true;
+}
+else if (!safecstate->processed_remaining_tuples)
+{
+	ReleaseCurrentSubTransaction();
+	CurrentResourceOwner = safecstate->oldowner;
+	if (safecstate->replayed_tuples < safecstate->saved_tuples)
+	{
+		/* Prepare to replay remaining tuples if 

Fwd: Merging statistics from children instead of re-sampling everything

[Damir Belyalov]

>
> 3) stadistinct - This is quite problematic. We only have the per-child
> estimates, and it's not clear if there's any overlap. For now I've just
> summed it up, because that's safer / similar to what we do for gather
> merge paths etc. Maybe we could improve this by estimating the overlap
> somehow (e.g. from MCV lists / histograms). But honestly, I doubt the
> estimates based on tiny sample of each child are any better. I suppose
> we could introduce a column option, determining how to combine ndistinct
> (similar to how we can override n_distinct itself).
>
> 4) MCV - It's trivial to build a new "parent" MCV list, although it may
> be too large (in which case we cut it at statistics target, and copy the
> remaining bits to the histogram)
>

I think there is one approach to solve the problem with calculating mcv and
distinct statistics.
To do this, you need to calculate the density of the sample distribution
and store it, for example, in some slot.
Then, when merging statistics, we will sum up the densities of all
partitions as functions and get a new density.
According to the new density, you can find out which values are most common
and which are distinct.

To calculate the partition densities, you can use the "Kernel density
Estimation" -
https://www.statsmodels.org/dev/examples/notebooks/generated/kernel_density
html

The approach may be very inaccurate and difficult to implement, but solves
the problem.

Regards,
Damir Belyalov
Postgres Professional

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Hi!

Improved my patch by adding block subtransactions.
The block size is determined by the REPLAY_BUFFER_SIZE parameter.
I used the idea of a buffer for accumulating tuples in it.
If we read REPLAY_BUFFER_SIZE rows without errors, the subtransaction will
be committed.
If we find an error, the subtransaction will rollback and the buffer will
be replayed containing tuples.

In the patch REPLAY_BUFFER_SIZE equals 3, but it can be changed to any
other number (for example 1000).
There is an idea to create a GUC parameter for it.
Also maybe create a GUC parameter for the number of occurring WARNINGS by
rows with errors.

For CIM_MULTI and CIM_MULTI_CONDITIONAL cases the buffer is not needed.
It is needed for the CIM_SINGLE case.

Tests:

-- CIM_MULTI case
CREATE TABLE check_ign_err (n int, m int, k int);
COPY check_ign_err FROM STDIN WITH IGNORE_ERRORS;
WARNING:  COPY check_ign_err, line 2: "2 2 2 2"
WARNING:  COPY check_ign_err, line 3: "3 3"
WARNING:  COPY check_ign_err, line 4, column n: "a"
WARNING:  COPY check_ign_err, line 5, column m: "b"
WARNING:  COPY check_ign_err, line 6, column n: ""
1 1 1
2 2 2 2
3 3
a 4 4
5 b b

7 7 7
\.
SELECT * FROM check_ign_err;
WARNING:  COPY check_ign_err, line 2: "2 2 2 2"
WARNING:  COPY check_ign_err, line 3: "3 3"
WARNING:  COPY check_ign_err, line 4, column n: "a"
WARNING:  COPY check_ign_err, line 5, column m: "b"
WARNING:  COPY check_ign_err, line 6, column n: ""
 n | m | k
---+---+---
 1 | 1 | 1
 7 | 7 | 7
(2 rows)

##


-- CIM_SINGLE case
-- BEFORE row trigger
CREATE TABLE trig_test(n int, m int);
CREATE FUNCTION fn_trig_before () RETURNS TRIGGER AS '
  BEGIN
INSERT INTO trig_test VALUES(NEW.n, NEW.m);
RETURN NEW;
  END;
' LANGUAGE plpgsql;
CREATE TRIGGER trig_before BEFORE INSERT ON check_ign_err
FOR EACH ROW EXECUTE PROCEDURE fn_trig_before();
COPY check_ign_err FROM STDIN WITH IGNORE_ERRORS;
WARNING:  COPY check_ign_err, line 2: "2 2 2 2"
WARNING:  COPY check_ign_err, line 3: "3 3"
WARNING:  COPY check_ign_err, line 4, column n: "a"
WARNING:  COPY check_ign_err, line 5, column m: "b"
WARNING:  COPY check_ign_err, line 6, column n: ""
1 1 1
2 2 2 2
3 3
a 4 4
5 b b

7 7 7
\.
SELECT * FROM check_ign_err;
 n | m | k
---+---+---
 1 | 1 | 1
 7 | 7 | 7
(2 rows)

##


-- INSTEAD OF row trigger
CREATE VIEW check_ign_err_view AS SELECT * FROM check_ign_err;
CREATE FUNCTION fn_trig_instead_of () RETURNS TRIGGER AS '
  BEGIN
INSERT INTO check_ign_err VALUES(NEW.n, NEW.m, NEW.k);
RETURN NEW;
  END;
' LANGUAGE plpgsql;
CREATE TRIGGER trig_instead_of INSTEAD OF INSERT ON check_ign_err_view
FOR EACH ROW EXECUTE PROCEDURE fn_trig_instead_of();
COPY check_ign_err_view FROM STDIN WITH IGNORE_ERRORS;
WARNING:  COPY check_ign_err, line 2: "2 2 2 2"
WARNING:  COPY check_ign_err, line 3: "3 3"
WARNING:  COPY check_ign_err, line 4, column n: "a"
WARNING:  COPY check_ign_err, line 5, column m: "b"
WARNING:  COPY check_ign_err, line 6, column n: ""
SELECT * FROM check_ign_err;
1 1 1
2 2 2 2
3 3
a 4 4
5 b b

7 7 7
\.
SELECT * FROM check_ign_err_view;
 n | m | k
---+---+---
 1 | 1 | 1
 7 | 7 | 7
(2 rows)

##

-- foreign table case in postgres_fdw extension

##

-- volatile function in WHERE clause
COPY check_ign_err FROM STDIN WITH IGNORE_ERRORS
  WHERE n = floor(random()*(1-1+1))+1; /* find values equal 1 */
WARNING:  COPY check_ign_err, line 2: "2 2 2 2"
WARNING:  COPY check_ign_err, line 3: "3 3"
WARNING:  COPY check_ign_err, line 4, column n: "a"
WARNING:  COPY check_ign_err, line 5, column m: "b"
WARNING:  COPY check_ign_err, line 6, column n: ""
SELECT * FROM check_ign_err;
1 1 1
2 2 2 2
3 3
a 4 4
5 b b

7 7 7
\.
SELECT * FROM check_ign_err;
 n | m | k
---+---+---
 1 | 1 | 1
(1 row)

##

-- CIM_MULTI_CONDITIONAL case
-- INSERT triggers for partition tables
CREATE TABLE check_ign_err (n int, m int, k int) PARTITION BY RANGE (n);
CREATE TABLE check_ign_err_part1 PARTITION OF check_ign_err
  FOR VALUES FROM (1) TO (4);
CREATE TABLE check_ign_err_part2 PARTITION OF check_ign_err
  FOR VALUES FROM (4) TO (8);
CREATE FUNCTION fn_trig_before_part () RETURNS TRIGGER AS '
  BEGIN
INSERT INTO trig_test VALUES(NEW.n, NEW.m);
RETURN NEW;
  END;
' LANGUAGE plpgsql;
CREATE TRIGGER trig_before_part BEFORE INSERT ON check_ign_err
FOR EACH ROW EXECUTE PROCEDURE fn_trig_before_part();
COPY check_ign_err FROM STDIN WITH IGNORE_ERRORS;
WARNING:  COPY check_ign_err, line 2: "2 2 2 2"
WARNING:  COPY check_ign_err, line 3: "3 3"
WARNING:  COPY check_ign_err, line 4, column n: &

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

[Damir Belyalov]

Hello.

Wrote a patch implementing COPY with ignoring errors in rows using block
subtransactions.

Syntax: COPY [table] FROM [file/stdin] WITH IGNORE_ERROS;

Examples:
CREATE TABLE check_ign_err (n int, m int, k int);
COPY check_ign_err FROM STDIN WITH IGNORE_ERRORS;
1 1 1
2 2 2 2
3 3 3
\.
WARNING:  COPY check_ign_err, line 2: "2 2 2 2"
SELECT * FROM check_ign_err;
 n | m | k
---+---+---
 1 | 1 | 1
 3 | 3 | 3
(2 rows)

##

TRUNCATE check_ign_err;
COPY check_ign_err FROM STDIN WITH IGNORE_ERRORS;
1 1 1
2 2
3 3 3
\.
WARNING:  COPY check_ign_err, line 2: "2 2"
SELECT * FROM check_ign_err;
 n | m | k
---+---+---
 1 | 1 | 1
 3 | 3 | 3
(2 rows)

##

TRUNCATE check_ign_err;
COPY check_ign_err FROM STDIN WITH IGNORE_ERRORS;
1 1 1
2 a 2
3 3 3
\.
WARNING:  COPY check_ign_err, line 2, column m: "a"
SELECT * FROM check_ign_err;
 n | m | k
---+---+---
 1 | 1 | 1
 3 | 3 | 3
(2 rows)



Regards, Damir

пт, 10 дек. 2021 г. в 21:48, Pavel Stehule :

>
>
> 2014-12-26 11:41 GMT+01:00 Pavel Stehule :
>
>>
>>
>> 2014-12-25 22:23 GMT+01:00 Alex Shulgin :
>>
>>> Trent Shipley  writes:
>>>
>>> > On Friday 2007-12-14 16:22, Tom Lane wrote:
>>> >> Neil Conway  writes:
>>> >> > By modifying COPY: COPY IGNORE ERRORS or some such would instruct
>>> COPY
>>> >> > to drop (and log) rows that contain malformed data. That is, rows
>>> with
>>> >> > too many or too few columns, rows that result in constraint
>>> violations,
>>> >> > and rows containing columns where the data type's input function
>>> raises
>>> >> > an error. The last case is the only thing that would be a bit
>>> tricky to
>>> >> > implement, I think: you could use PG_TRY() around the
>>> InputFunctionCall,
>>> >> > but I guess you'd need a subtransaction to ensure that you reset
>>> your
>>> >> > state correctly after catching an error.
>>> >>
>>> >> Yeah.  It's the subtransaction per row that's daunting --- not only
>>> the
>>> >> cycles spent for that, but the ensuing limitation to 4G rows imported
>>> >> per COPY.
>>> >
>>> > You could extend the COPY FROM syntax with a COMMIT EVERY n clause.
>>> This
>>> > would help with the 4G subtransaction limit.  The cost to the ETL
>>> process is
>>> > that a simple rollback would not be guaranteed send the process back
>>> to it's
>>> > initial state.  There are easy ways to deal with the rollback issue
>>> though.
>>> >
>>> > A {NO} RETRY {USING algorithm} clause might be useful.   If the NO
>>> RETRY
>>> > option is selected then the COPY FROM can run without subtransactions
>>> and in
>>> > excess of the 4G per transaction limit.  NO RETRY should be the
>>> default since
>>> > it preserves the legacy behavior of COPY FROM.
>>> >
>>> > You could have an EXCEPTIONS TO {filename|STDERR} clause. I would not
>>> give the
>>> > option of sending exceptions to a table since they are presumably
>>> malformed,
>>> > otherwise they would not be exceptions.  (Users should re-process
>>> exception
>>> > files if they want an if good then table a else exception to table b
>>> ...)
>>> >
>>> > EXCEPTIONS TO and NO RETRY would be mutually exclusive.
>>> >
>>> >
>>> >> If we could somehow only do a subtransaction per failure, things would
>>> >> be much better, but I don't see how.
>>>
>>> Hello,
>>>
>>> Attached is a proof of concept patch for this TODO item.  There is no
>>> docs yet, I just wanted to know if approach is sane.
>>>
>>> The added syntax is like the following:
>>>
>>>   COPY [table] FROM [file/program/stdin] EXCEPTIONS TO [file or stdout]
>>>
>>> The way it's done it is abusing Copy Both mode and from my limited
>>> testing, that seems to just work.  The error trapping itself is done
>>> using PG_TRY/PG_CATCH and can only catch formatting or before-insert
>>> trigger errors, no attempt is made to recover from a failed unique
>>> constraint, etc.
>>>
>>> Example in action:
>>>
>>> postgres=# \d test_copy2
>>>   Table "public.test_copy2"
>>>  Column |  Type   | Modifiers
>>> +---

Re: Extensibility of the PostgreSQL wire protocol

[Damir Simunic]

> On 19 Feb 2021, at 19:30, Jan Wieck  wrote:
> 
> An "extended" libpq protocol could allow the pool to give clients a unique 
> ID. The protocol handler would then maintain maps with the SQL of prepared 
> statements and what the client thinks their prepared statement name is. 

Or, the connection pooler could support a different wire protocol that has some 
form of client cookies and could let the client hold on to an opaque token to 
present back with every query and use that to route to the right backend with a 
prepared statement for that client (or match the appropriate cached p statement 
from the cache), even across client disconnections.

> Most of that would of course be possible on the pool side itself. But the 
> internal structure of pgbouncer isn't suitable for that. It is very 
> lightweight and for long SQL queries may never have the complete 'P' message 
> in memory. It would also not have direct access to security related 
> information like the search path, which would require extra round trips 
> between the pool and the backend to retrieve it.

> 
> So while not suitable to create a built in pool by itself, loadable wire 
> protocols can definitely help with connection pooling.

I think loadable wire protocols will have a positive effect on developing more 
sophisticated connection poolers.

> I also am not sure if building a connection pool into a background worker or 
> postmaster is a good idea to begin with. One of the important features of a 
> pool is to be able to suspend traffic and make the server completely idle to 
> for example be able to restart the postmaster without forcibly disconnecting 
> all clients.

Agreed. Going even further, a connection pooler supporting a protocol like quic 
(where the notion of connection is decoupled from the actual socket connection) 
could help a lot with balancing load between servers and data centers, which 
also would not be convenient for the actual Postgres to do with present 
architecture. (And here, too, a pluggable wire protocol would help with keeping 
tabs on individual backends).

--
Damir

Re: Extensibility of the PostgreSQL wire protocol

[Damir Simunic]

> On 19 Feb 2021, at 14:48, Heikki Linnakangas  wrote:
> 
> For example, there has been discussion elsewhere about integrating connection 
> pooling into the server itself. For that, you want to have a custom process 
> that listens for incoming connections, and launches backends independently of 
> the incoming connections. These hooks would not help with that.
> 

Not clear how the connection polling in the core is linked to discussing 
pluggable wire protocols. 

> Similarly, if you want to integrate a web server into the database server, 
> you probably also want some kind of connection pooling. A one-to-one 
> relationship between HTTP connections and backend processes doesn't seem nice.
> 

HTTP/2 is just a protocol, not unlike fe/be that has a one-to-one relationship 
to backend processes as it stands. It shuttles data back and forth in 
query/response exchanges, and happens to be used by web servers and web 
browsers, among other things. My mentioning of it was simply an example I can 
speak of from experience, as opposed to speculating. Could have brought up any 
other wire protocol if I had experience with it, say MQTT.

To make it clear, “a pluggable wire protocol” as discussed here is a set of 
rules that defines how data is transmitted: what the requests and responses 
are, and how is the data laid out on the wire, what to do in case of error, 
etc. Nothing to do with a web server; why would one want to integrate it in the 
database, anyway?

The intended contribution to the discussion of big picture of pluggable wire 
protocols is that there are significant use cases where the protocol choice is 
restricted on the client side, and allowing a pluggable wire protocol on the 
server side brings tangible benefits in performance and architectural 
simplification. That’s all. The rest were supporting facts that hopefully can 
also serve as a counterpoint to “pluggable wire protocol is primarily useful to 
make Postgres pretend to be Mysql."

Protocol conversion HTTP/2<—>FE/BE on the connection pooler already brings a 
lot of the mentioned benefits, and I’m satisfied with it. Beyond that I’m 
simply supporting the idea of  pluggable protocols as experience so far allows 
me to see advantages that might sound theoretical to someone who never tried 
this scenario in production.

Glad to offer a couple of examples where I see potential for performance gains 
for having such a wire protocol pluggable in the core. Let me know if you want 
me to elaborate.

> Querying multiple databases over a single connection is not possible with the 
> approach taken here. 

Indeed, querying multiple databases over a single connection is something you 
need a proxy for and a different client protocol from fe/be. No need to mix 
that with the talk about pluggable wire protocol. 

My mentioning of it was in the sense “a lot of LoB backend code is nothing more 
than a bloated protocol converter that happens to also allow connecting to 
multiple databases from a single client connection => letting the client speak 
to the database [trough a proxy in this case] removed the bloated source of 
latency but kept the advantages.”

--
Damir

Re: Extensibility of the PostgreSQL wire protocol

[Damir Simunic]

> On 11 Feb 2021, at 16:06, Tom Lane  wrote:
> 
> Maybe there is some useful thing that can be accomplished here, but we
> need to consider the bigger picture rather than believing (without proof)
> that a few hook variables will be enough to do anything.
> 
>   regards, tom lane
> 

Pluggable wire protocol is a game-changer on its own. 

The bigger picture is that a right protocol choice enables large-scale 
architectural simplifications for whole classes of production applications.

For browser-based applications (lob, saas, e-commerce), having the database 
server speak the browser protocol enables architectures without backend 
application code. This in turn leads to significant reductions of latency, 
complexity, and application development time. And it’s not just lack of backend 
code: one also profits from all the existing infrastructure like per-query 
compression/format choice, browser connection management, sse, multiple 
streams, prioritization, caching/cdns, etc.

Don’t know if you’d consider it as a proof, yet I am seeing 2x to 4x latency 
reduction in production applications from protocol conversion to http/2. My 
present solution is a simple connection pooler I built on top of Nginx 
transforming the tcp stream as it passes through.

In a recent case, letting the browser talk directly to the database allowed me 
to get rid of a ~100k-sloc .net backend and all the complexity and 
infrastructure that goes with coding/testing/deploying/maintaining it, while 
keeping all the positives: per-query compression/data conversion, querying 
multiple databases over a single connection, session cookies, etc. Deployment 
is trivial compared to what was before. Latency is down 2x-4x across the board.

Having some production experience with this approach, I can see how 
http/2-speaking Postgres would further reduce latency, processing cost, and 
time-to-interaction for applications.

A similar case can be made for IoT where one would want to plug an 
iot-optimized protocol. Again, most of the benefit is possible with a 
protocol-converting proxy, but there are additional non-trivial performance 
gains to be had if the database server speaks the right protocol.

While not the only use cases, I’d venture a guess these represent a sizable 
chunk of what Postgres is used for today, and will be used even more for, so 
the positive impact of a pluggable protocol would be significant.

--
Damir

Re: Code of Conduct plan

[Damir Colak]

Please take me off this list.


> On Sep 14, 2018, at 05:31, Chris Travers  wrote:
> 
> 
> 
> On Wed, Sep 12, 2018 at 10:53 PM Tom Lane  > wrote:
> I wrote:
> > Stephen Frost mailto:sfr...@snowman.net>> writes:
> >> We seem to be a bit past that timeline...  Do we have any update on when
> >> this will be moving forward?
> >> Or did I miss something?
> 
> > Nope, you didn't.  Folks have been on holiday which made it hard to keep
> > forward progress going, particularly with respect to selecting the initial
> > committee members.  Now that Magnus is back on shore, I hope we can
> > wrap it up quickly --- say by the end of August.
> 
> I apologize for the glacial slowness with which this has all been moving.
> The core team has now agreed to some revisions to the draft CoC based on
> the comments in this thread; see
> 
> https://wiki.postgresql.org/wiki/Code_of_Conduct 
> 
> 
> (That's the updated text, but you can use the diff tool on the page
> history tab to see the changes from the previous draft.)
> 
> I really have to object to this addition:
> "This Code is meant to cover all interaction between community members, 
> whether or not it takes place within postgresql.org  
> infrastructure, so long as there is not another Code of Conduct that takes 
> precedence (such as a conference's Code of Conduct)."
> 
> That covers things like public twitter messages over live political 
> controversies which might not be personally directed.   At least if one is 
> going to go that route, one ought to *also* include a safe harbor for 
> non-personally-directed discussions of philosophy, social issues, and 
> politics.  Otherwise, I think this is asking for trouble.  See, for example, 
> what happened with Opalgate and how this could be seen to encourage use of 
> this to silence political controversies unrelated to PostgreSQL.
> 
> I think we are about ready to announce the initial membership of the
> CoC committee, as well, but that should be a separate post.
> 
> regards, tom lane
> 
> 
> 
> -- 
> Best Wishes,
> Chris Travers
> 
> Efficito:  Hosted Accounting and ERP.  Robust and Flexible.  No vendor 
> lock-in.
> http://www.efficito.com/learn_more 

Re: Proposal: http2 wire format

[Damir Simunic]

> 
> 
> I'm rapidly losing interest. Unless this goes back toward the concrete and 
> practical I think it's going nowhere.


Your message is exactly what I was hoping for. Thanks for your guidance and 
support, really appreciate you. 

Let me now get busy and earn your continued interest and support. 


Damir
> 
> -- 
>  Craig Ringer   http://www.2ndQuadrant.com/ 
> <http://www.2ndquadrant.com/>
>  PostgreSQL Development, 24x7 Support, Training & Services

Re: Proposal: http2 wire format

[Damir Simunic]

> Currently it is implemented via different v3 messages (parse, bind, execute, 
> row description, data row, etc etc).
> 
> The claim is *any* implementation "on top of HTTP/2" would basically require 
> to implement those "parse, bind, execute, row data, etc" *messages*.

Why? Wouldn’t you be able to package that into a single request with query in 
the data frame and params as headers?

> Say you pick to use "/parse" url with SQL text in body instead of "parse 
> message". It does not make the whole thing "just HTTP/2". It just means 
> you've created "your own protocol on top of HTTP/2”.

It is new functionality, isn’t it? Of course you have to evolve protocol 
semantics for that. That’s the whole point! HTTP2 is just a nice substrate that 
comes with the way to negotiate capabilities and can separate the metadata from 
payload. Nothing revolutionary, but it lets you move forward without hurting 
existing applications. Isn’t that an upgrade from v3?

> 
> Clients would have to know the sequence of valid messages,
> clients would have to know if SQL should be present in body or in URL or in 
> form post data, etc, etc.
> 
> I believe Andres means exactly the same thing as he says
> 
> By the way: you cannot just "load balance" "parse/bind/exec" to different 
> backends, so the load balancer should be aware of meaning of those 
> "parse/bind/exec" messages. I believe that is one of the requirements Craig 
> meant by "Is practical to implement in connection pooler proxies”.

Why can’t I package this into a single request? Don’t modern web proxies deal 
with session affinity and stuff like that?

> 
> Andres>You *still* need a full blown protocol ontop of it. So no, this 
> doesn't change that
> 
> 
> Damir> Did you know that Go has HTTP2 support in the standard library? And so 
> does java, too?
> 
> Java has TCP implementation in the standard library.
> Does it help implementing v3 protocol?

It does. If Java only had IP, without TCP, would you be able to implement your 
driver? Yes, but you’d have to suffer longer.

> In the same way HTTP/2 "as a library" helps implementing v4. The problem is 
> it does not. Developer would have to somehow code the coding rules (e.g. 
> header names, body formats).
> HTTP/2 is just too low level.
> 

It’s just framing. But standard framing.

> 
> Damir>Why parse some obscure Postgres-specific binary data types when you can 
> have the database send you the data in the serialization format of your 
> client language?
> 
> From my own experience, automatic use of server-prepared statements (see 
> https://github.com/pgjdbc/pgjdbc/pull/319 
> <https://github.com/pgjdbc/pgjdbc/pull/319> ) did cut end-user response times 
> of our business application in half.
> That is clients would have to know the way to use prepared statements in 
> order to get decent performance.
> If you agree with that, then "v3 parse message", "v3 bind message", "v3 
> execute message" is not that different from "HTTP/2 POST to /parse", "HTTP/2 
> POST to /bind", "HTTP/2 POST to /execute". It is still "obscure 
> PostgreSQL-specific HTTP/2 calls”.

What of having that in one single request?

> 
> Even if you disagree (really?) you would still have to know 
> PostgreSQL-specific way to encode SQL text and "number of rows returned" and 
> "wire formats for the columns" even for a single "HTTP POST 
> /just/execute/sql" kind of API. Even that is "a full blown protocol ontop of 
> HTTP2" (c) Andres.

What does your business app do with the data?



> 
> Vladimir

Re: Proposal: http2 wire format

[Damir Simunic]

Hi Andres,

> 
> At least I do *NOT* want many protocols in core. We've a hard enough
> time to keep up with integrating patches and maintenance to not just
> willy nilly integrate multiple new features with unclear lifetimes.

Admire your effort in applying all these patches—this commitfest thing looks 
frenetic now that I’m subscribed to the mailing list. Can only guess the effort 
required on the part of a few of you to study and triage everything. Respect.

Actually, I don’t advocate multiple protocols in core. But the exercise of 
considering one will help generalize the architecture enough to make all 
protocols pluggable. 

The most interesting part for me is working out content negotiation—I think 
being able to package data in new ways will be super-interesting.

> 
> *NONE* of the interesting problems are solved by HTTP2. You *still*
> need a full blown protocol ontop of it. So no, this doesn't change that.

If you had to nominate only one of those problems, which one would you consider 
the most interesting?


Thanks for chiming in, really appreciate your time,
Damir

Re: Proposal: http2 wire format

[Damir Simunic]

> On 26 Mar 2018, at 18:09, Vladimir Sitnikov <sitnikov.vladi...@gmail.com> 
> wrote:
> 
> Damir>Postgres doesn’t know about grpc, s
> 
> I'm afraid you are missing the point.
> I would say PostgreSQL doesn't know about HTTP/2.
> It is the same as "PostgreSQL doesn't know about grpc".
> 
> Here's a quote from your pg_h2 repo:
> >What we need is to really build a request object and correctly extract
> > the full payload and parameters from the request. For example,
> >maybe we want to implement a QUERY method, similar to POST or PUT,
> > and pass the query text as the body of the request, with parameters
> > in the query string or in the headers
> 
> It basically suggests to implement own framing on top of HTTP/2.

Wouldn’t that be protocol semantics? Framing is already taken care of by the 
wire protocol.

> 
> When I say GRPC, I mean "implement PostgreSQL-specific protocol via GRPC 
> messages".
> 
> Let's take current message formats: 
> https://www.postgresql.org/docs/current/static/protocol-message-formats.html
> If one defines those message formats via GRPC, then GRPC would autogenerate 
> parsers and serializers for lots of languages "for free".
> 
> For instance
> Query (F)
>  Byte1('Q') Identifies the message as a simple query.
>  Int32 Length of message contents in bytes, including self.
>  String The query string itself.
> 
> can be defined via GPRC as
> message Query {
>   string queryText = 1;
> }
> 
> This is trivial to read, trivial to write, trivial to maintain, and it 
> automatically generates parsers/generators for lots of languages.
> 

I agree with you 100% here. But can you pull off grpc without HTTP2 framing in 
place? Would it be the only protocol supported? What if I wanted JSON or CSV 
returned, or just plain old Postgres v3 binary format, since I already have the 
parser written for it? Wouldn’t you need to first solve the problem of content 
negotiation?

HTTP2 proposal is pragmatically much smaller chunk, and it’s already hard to 
explain. Can you imagine the reaction and discussion if I came up with this?

In fact, if you ask yourself the question “how can I do something about the 
status quo of FEBE protocol that would be defensible in front of the Postgres 
community?” What would be your answer? 

> 
> Parsing of the current v3 protocol has to be reimplemented for each and every 
> language, and it would be pain to implement parsing for v4.
> Are you going to create "http/2" clients for Java, C#, Ruby, Swift, Dart, 
> etc, etc?
> 
> I am not saying that a mere redefinition of v3 messages as GRPC would do the 
> trick. I am saying that you'd better consider frameworks that would enable 
> transparent implementation of client libraries.
> 
> Damir>and will talk to any HTTP2 conforming client
> 
> I do not see where are you heading to.

Getting rid of having to write a framing parser in every client language?

> Is "curl as PostgreSQL client" one of the key objectives for you?

No, it’s just something that is available right now—the point is to demonstrate 
increased ability to get the data out, without having to write access code over 
and over, and then lug that whenever you install some data processing piece. 
Kind of the same motivation why you think grpc is it. I’m just proposing a 
layer under it that gets rid of a lot of pain.

> True clients (the ones that are used by the majority of applications) should 
> support things like "prepared statements", "data types", "cursors" (resultset 
> streaming), etc. I can hardly imagine a case when one would use "curl" and 
> operate with prepared statements.

Wouldn’t HTTP2 framing still allow prepared statements and cursors?

> I think psql is pretty good client, so I see no point in implementing HTTP/2 
> for a mere reason of using curl to fetch data from the DB.

> 
> Vladimir

Re: Proposal: http2 wire format

[Damir Simunic]

> On 26 Mar 2018, at 15:42, Alvaro Hernandez <a...@ongres.com> wrote:
> 
> 
> 
> On 26/03/18 13:11, Damir Simunic wrote:
>>> On 26 Mar 2018, at 11:13, Vladimir Sitnikov <sitnikov.vladi...@gmail.com 
>>> <mailto:sitnikov.vladi...@gmail.com>> wrote:
>>> 
>>> Damir> * What are the criteria for getting this into the core?
>>> Craig>Mine would be: 
>>> 
>>> +1
>>> 
>>> There's a relevant list as well: 
>>> https://github.com/pgjdbc/pgjdbc/blob/master/backend_protocol_v4_wanted_features.md
>>>  
>>> <https://github.com/pgjdbc/pgjdbc/blob/master/backend_protocol_v4_wanted_features.md>
>>>  
>>> 
>> 
>> This is a great addition to the list, thanks!
>> 
>> Damir
>> 
> 
> Hi Damir.
> 
> I'm interested in the idea. However, way before writing a PoC, IMVHO I'd 
> rather write a detailed document including:
> 
> - A brief summary of the main features of HTTP2 and why it might be a good 
> fit for PG (of course there's a lot of doc in the wild about HTTP/2, so just 
> a summary of the main relevant features and an analysis of how it may fit 
> Postgres).
> 
> - A more or less thorough description of how every feature in current 
> PostgreSQL protocol would be implemented on HTTP/2.
> 
> - Similar to the above, but applied to the v4 TODO feature list.
> 
> - A section for connection poolers, as  an auth, as these are very important 
> topics.
> 
> 
> Hope this helps,
> 
> Álvaro
> 

Álvaro, it does help, thanks. This discussion is to inform such a document. But 
the topic is such that having a good PoC will move the discussion further much 
faster. 

Can you help with thinking about how would HTTP2 impact connection poolers, I 
don’t know much about those?
> -- 
> 
> Alvaro Hernandez
> 
> 
> ---
> OnGres

Re: Proposal: http2 wire format

[Damir Simunic]

> On 26 Mar 2018, at 18:19, Vladimir Sitnikov  
> wrote:
> 
> Tom>But starting from the assumption that HTTP2 solves our problems seems to 
> me to be "Here's a hammer.
> 
> Agree.

Funny you agree with that—for someone having the experience of writing a driver 
and having a long list of things that you find wrong and frustrating, one would 
expect you do look at how other protocols work, or at least consider that maybe 
the right way is to change something server side.

> 
> Just a side note: if v4 is ever invented I wish client language support
> is considered.
> It does take resources to implement message framing, and data parsing (e.g. 
> int, timestamp, struct, array, ...) for each language independently.

This is a strange statement about framing. Did you know that Go has HTTP2 
support in the standard library? And so does java, too? 
https://github.com/http2/http2-spec/wiki/Implementations 


The part I hinted at in the example but did not get the message across is that 
I’m advocating the best possible client language support. The right way is to 
stop writing drivers and format the data server side. Why parse some obscure 
Postgres-specific binary data types when you can have the database send you the 
data in the serialization format of your client language? Or JSON or protobuf 
or whatever you want. What if your application has data patterns that would 
benefit from being sent over the wire in some specific columnar format? 
Wouldn’t it be cool if you could add that to the server and have all clients 
just work, without being locked in into a language because of its driver?

My point is that you go in steps. Put the foot in the door first, enable 
experimentation and then you’ll get to where you want to be.


> 
> Vladimir 

Re: Proposal: http2 wire format

[Damir Simunic]

> On 26 Mar 2018, at 11:06, Vladimir Sitnikov <sitnikov.vladi...@gmail.com> 
> wrote:
> 
> Hi,
> 
> >If anyone finds the idea of Postgres speaking http2 appealing
> 
> HTTP/2 sounds interesting.
> What do you think of https://grpc.io/ ?
> 
> Have you evaluated it?
> It does sound like a ready RPC on top of HTTP/2 with support for lots of 
> languages.
> 
> The idea of reimplementing the protocol for multiple languages from scratch 
> does not sound too appealing.

This proposal takes the stance that having HTTP2 wire protocol in place will 
enable wide experimentation  with and implementation of many new features and 
content types, but is not concerned with the specifics of those.

---
Let me illustrate with an example how it would look if we already had HTTP2 as 
proposed.

Lets’ say you have a building automation device on your network that happens to 
speak grpc, and you decided to use Postgres to store published topics in the 
database. 

Your grpc-speaking device might connect to Postgres and issue a request like 
this:

HEADERS (flags = END_HEADERS)
:method = POST
:scheme = http
:path = /CreateTopic
pg-database = Publisher
content-type = application/grpc+proto
grpc-encoding = gzip
authorization = Bearer y235.wef315yfh138vh31hv93hv8h3v

DATA (flags = END_STREAM)


(This is from grpc.io homepage; uppercase HEADERS and DATA are frame names from 
the HTTP2 specification).

Postgres would take care of TLS negotiation, unpack the frames, decompress the 
headers (:method, :path, etc are transferred compressed with a lookup table) 
and copy the payload into memory and make it  all available to the backend. If 
this was the first request, it would start the backend for you as well.

Postgres doesn’t know about grpc, so it would just conveniently return "406 Not 
Supported” to your client and close the stream (but not the connection). Still 
connected and authenticated, the device could retry the request with 
`content-type: application/json`, and if you somehow programmed a function that 
accepts json, the request would go through. (Let’s imagine we have some kind of 
mechanism to associate functions to requests and content types, maybe through 
some function attributes in the catalog). 

Say that someone else took the time and programmed a plugin that knows how to 
talk grpc. Then the server would call that plugin for you, validate and insert 
the data in the right table, and return 200 OK or 204 or whatever is 
appropriate to return according to grpc protocol semantics. 

Obviously, someone has to implement a bunch of new code on the server side to 
ungzip, to interpret the content of the protobuf message and take action. But 
that someone doesn’t need to think of getting to all the metadata like 
compression type, payload format etc. Just somehow plug into the server at the 
right level read the data and metadata from memory, and then call into SPI to 
do its thing. Similar to how application servers work today. (Or Postgres for 
that matter, though it’s just it speaks FEBE and there’s no content type 
negotiation).

The same goes for the ‘authorization’ header. Postgres does not support Bearer 
token authorization today. But maybe you’ll be able to define a function that 
knows how to deal with the token, and somehow signal to Postgres that you want 
it to call this function when it sees such a header. Or maybe someone wrote a 
plugin that does that, and you configure your server to use it. 

Then when connecting to Postgres with the above request, it would start the 
backend and call the function/plugin for you to decide whether to authorize the 
request. (As a side note, subsequent requests within the same connection would 
have this header compressed on the wire; that’s also a HTTP2 feature).

---

That’s only one possible scenario, and not the only one. In this specific 
scenario, the benefit is that Postgres will give you content negotiation built 
in, and will talk to any HTTP2 conforming client. Like you said, you don’t want 
to reimplement the protocol over and over.

But whether that content is grpc or something else, that's for a future 
discussion. 

Current focus is really on getting the framing and extensibility in the core. 
Admittedly, haven’t yet figured out how to code all the details, but I’m more 
and more clear how this will work architecturally. Now it’s about putting lots 
of elbow grease into understanding the source, coding in C, and addressing all 
the issues that make sure the new protocol is 100% supporting all existing v3 
use cases. 

Beyond v3 use cases, top of my mind are improvements like you comment on in the 
topic “Binary transfer” in your “v4 wanted features” doc (and most of the other 
stuff you mention).


Damir


> 
> Vladimir

Re: Proposal: http2 wire format

[Damir Simunic]

> On 26 Mar 2018, at 11:13, Vladimir Sitnikov <sitnikov.vladi...@gmail.com> 
> wrote:
> 
> Damir> * What are the criteria for getting this into the core?
> Craig>Mine would be: 
> 
> +1
> 
> There's a relevant list as well: 
> https://github.com/pgjdbc/pgjdbc/blob/master/backend_protocol_v4_wanted_features.md
>  
> <https://github.com/pgjdbc/pgjdbc/blob/master/backend_protocol_v4_wanted_features.md>
>  
> 

This is a great addition to the list, thanks!

Damir

Re: Proposal: http2 wire format

[Damir Simunic]

> On 26 Mar 2018, at 12:47, Craig Ringer <cr...@2ndquadrant.com> wrote:
> 
> On 26 March 2018 at 17:34, Damir Simunic <damir.simu...@wa-research.ch> wrote:
>  
> 
> > As you move forward with the PoC, consider: even if you decide not to
> > become protocol-layer experts, you'll still need to become familiar
> > with application-layer security in HTTP.
> 
> Good point. Application layer security is indeed a concern.
> 
> h2 has provisions for security by design, and a significant amount of 
> research going into this on a large scale. Adopting h2 instead of inventing 
> our own v4 gets us all this research for free.
> 
> HTTP2, please, not "h2".
> 
> It looks HTTP2 does use the term "h2" to mean "http2 over TLS", to 
> differentiate it from "h2c" which is HTTP2-over-cleartext.
> 
> IMO, you'd have to support both. Mandating TLS is going to be a non-starter 
> for sites that use loopback connections or virtual switches on VMs, VLAN 
> isolation, or other features to render traffic largely unsniffable. They 
> won't want to pay the price for crypto on all traffic. So this needs to be 
> "HTTP2 support" not "HTTP2/TLS (h2) support" anyway.

Makes sense; I’ll update all wording and function names, etc. No difference to 
the substance of this proposal. The same code path handles both h2 and h2c. TLS 
is optional, a matter of detecting the first byte of the request and taking the 
appropriate action. 

I think we can reliably and efficiently detect h2, h2c, and FEBE requests. Of 
course, the behavior needs to be configurable: which protocols to enable, and 
how to resolve the negotiation. In my mind this is self-evident.

> 
> Re Pg and security: By and large we don't invent our own security protocols. 
> We've adopted standard mechanisms like GSSAPI and SCRAM, and vendor ones like 
> SSPI. Some of the details of how they're implemented in the protocol are of 
> course protocol specific (and thus, opportunities for bugs/design mistakes), 
> of course.
> 
> But you will get _nowhere_ in making this a new default protocol if you just 
> try to treat those as outdated and uninteresting.
> 

Agreed: new default protocol must be covering 100% of existing use cases, _and_ 
add more compelling capabilities on top.

If anything I wrote made it appear contrary to that goal, it is purely because 
of my current focus on getting to a PoC. 

> In fact, part of extensibility considerations should be extensible 
> authentication.
> 
> Authentication and authorization (which any new protocol really should 
> separate) are crucial features, and there's no one-size-fits-all answer.
> 

I think that HTTP2 gets us much closer to that goal. My vision is to enable 
application-developer-defined authentication and/or authorization as well. This 
is something to research once the framing is in place.

> If you just assume, say, that everything happens over TLS with password auth 
> or x.509 client certs, you'll create a giant mess for all the sites that use 
> Kerberos or SSPI.
> 

100% agreed on everything you say, and thanks for taking the time to write this 
up. 

> 
> -- 
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services

Re: Proposal: http2 wire format

[Damir Simunic]

> On 26 Mar 2018, at 11:34, Craig Ringer <cr...@2ndquadrant.com> wrote:
> 
> On 26 March 2018 at 17:01, Damir Simunic <damir.simu...@wa-research.ch 
> <mailto:damir.simu...@wa-research.ch>> wrote:
>  
> 
> > - Doesn't break new clients connecting to old servers
> >
> 
> Old server sends “Invalid startup packet” and closes the connection; client’s 
> TLS layer reports an error. Does that count as not breaking new clients?
> 
> 
> libpq would have to do something like it does now for ssl connections, 
> falling back to non-ssl, and offering a connection option to make it try the 
> v3 protocol immediately without bothering with v4.
>  
> > - No extra round trips for new client -> old server . I don't personally 
> > care about old client -> new server so much, but should be able to offer a 
> > pg_hba.conf option to ensure v3 proto only or otherwise prevent extra round 
> > trips in this case too.
> 
> Can we talk about this more, please?
> 
> As above. A newer libpq should not perform worse on an existing server than 
> an older libpq.

Wouldn’t newer libpq continue to support v3 as long as supported servers do? 
I’m confused with “no extra round trips” part and the “pg_hba.conf option". If 
I know I’m talking to the old server, I’ll just configure the client to talk 
febe v3 and not worry.

Anyway, I’ll document all the combinations to make it easier to discuss.

>  
>  
> 
> Check.
> 
> Extensibility is the essence of h2, we’re getting this for free.
> 
> 
> Please elaborate somewhat for people not already strongly familiar with HTTP2.
> 
> BTW, please stop saying "h2" when you mean HTTP2. It's really confusing, 
> because I keep thinking you are talking about H2, the database engine 
> (http://www.h2database.com/ <http://www.h2database.com/>), which has 
> PostgreSQL protocol and syntax compatibility as well as its own wire protocol.

Haha, I din’t know that! “h2” is the protocol identifier in the ALPN; in mind, 
http2 has more of the web and http1 baggage that I’m trying to avoid here. But 
let’s stick to http2 and define it better.

>  
> > - Has a wireshark dissector
> 
> Check.
> 
> ... including understanding of the PostgreSQL bits that are payload within 
> the protocol.
> 
> Look at what the current dissector does - capture some packets.
>  
> 
> >
> > - Is practical to implement in connection pooler proxies like pgbouncer, 
> > pgpool
> 
> Something I’m planning to look into and address.
> 
> New connection poolers might become feasible, too: nginx, nghttpx, etc. (for 
> non-web related scenarios as well). Opting into h2 lets us benefit from a 
> much larger amount of time and resources being spent on improving things that 
> matter. Reverse proxies face the same architectural challenges as pg-only 
> connection poolers do.
> 
> 
> ... which is nice, but doesn't change the fact that a protocol revision that 
> completely and unfixably breaks existing tools much of the community relies 
> on won't go far.
>  
> > - Any libraries used are widespread enough that they're present in at least 
> > RHEL7 and Debian Stable. We *can't* just bundle extras in our sources, and 
> > packagers are unlikely to be at all happy packaging an extra lib or 
> > backport for us. They'll probably just disable the new protocol.
> 
> Check.
> 
> Let me see if I can make a table showing parallel availability of Postgres 
> and libnghttp versions on mainstream platforms. If there are any gaps, I’m 
> sure it is possible to lobby for inclusion of libnghttp where it matters. I 
> see Debian has it for wheezy, jessie, and sid, while pg10 is on sid and 
> buster.
> 
> 
> Good plan. But be clear that this is super experimental.
>  
> >
> > - No regressions for support of SASL / SCRAM, GSSAPI, TLS with X.509 client 
> > certs, various other auth methods.
> >
> 
> Check.
> 
> Adding new auth method keyword (“h2”) in pg_hba will give us a clean code 
> path to work with.
> 
> I think you missed the point there entirely.
> 
> HTTP2 isn't an authentication method. It's a wire protocol. It will be 
> necessary to support authentication methods including, but not limited to, 
> GSSAPI, SSPI (windows), SCRAM, etc *on any new protocol*.
> 
> If you propose a new protocol, to replace the v3 protocol, and it doesn't 
> support SSPI or SCRAM I rate your chances as about zero of getting serious 
> interest. You'll be back in extension-for-webdevs town.
>  

Great points. I need to be more clear on that. My main concern was how to 
bypass the v3 auth negotiation that is closely linked to existing methods. From 
PoC perspective, I d

Re: Proposal: http2 wire format

[Damir Simunic]

Hi,

> On 26 Mar 2018, at 06:47, Jacob Champion <pchamp...@pivotal.io> wrote:
> 
> On Sun, Mar 25, 2018 at 8:11 PM, Craig Ringer <cr...@2ndquadrant.com> wrote:
>> As others have noted, you'll want to find a way to handle this in the least
>> SSL-implementation-specific manner possible. IMO if it can't work with
>> OpenSSL, Windows's SSL implementation and OS X's SSL framework it's a
>> non-starter.
> 
> +1.
> 
>> While I'm a big fan of code reuse and using existing libraries, I understand
>> others' hesitance here. Look at what happened with ossp-uuid; that was
>> painful and it was just a contrib.
>> 
>> It's a difficult balance between NIH and maintaining a stable core.
> 
> For whatever it's worth, I think libnghttp2 is an excellent choice for
> an HTTP/2 implementation, even when taking into account the risks of
> NIH. It's a well-designed library with mature clients (Curl and Apache
> HTTP Server, among others), and it's authored by an HTTP/2 expert. (If
> you're seriously considering HTTP/2, then you seriously need to avoid
> not-invented-here syndrome. Don't roll your own unless you're
> interested in becoming HTTP/2 protocol-layer security experts in
> addition to SQL security experts.)
> 
Agreed.

> As you move forward with the PoC, consider: even if you decide not to
> become protocol-layer experts, you'll still need to become familiar
> with application-layer security in HTTP.

Good point. Application layer security is indeed a concern. 

h2 has provisions for security by design, and a significant amount of research 
going into this on a large scale. Adopting h2 instead of inventing our own v4 
gets us all this research for free.


> You'll need to decide whether
> the HTTP browser/server security model -- which is notoriously
> unintuitive for many -- works well for Postgres. In particular, you'll
> want to make sure that the new protocol doesn't put your browser-based
> users in danger (I'm thinking primarily about cross-site request
> forgeries here). Always remember that one of a web browser's core use
> cases is the execution of untrusted code…

Mentioning h2 does bring browsers in mind, but this proposal is not concerned 
with that. (quick curl sketches are shown only because curl is an already 
available h2 client). Present web-facing designs already deal with browsers and 
API clients, there will be no change to that. Existing Postgres deployment and 
security practices must remain unchanged whether we use v3 or h2. Don’t think 
anyone would want to expose Postgres to the open web without a connection 
pooler in front of it.

When you say "browser/server model,” presumably you’re having http1 in mind. h2 
does not have much in common with http1 on the wire. In fact, h2 is 
architecturally closer to febe than http1. Both h2 and febe deal with multiple 
request/response pairs over a single connection. Server initiated requests are 
covered through push_promise frames, and logical replication (being more of a 
subscription thing in my mind) is covered through stream multiplexing.

Let's keep the discussion focused on the wire protocol: the sooner we can get 
to stable h2 framing in the core, the sooner we’ll be able to experiment with 
new use cases and possibilities. Only then it will make sense to bring back 
this discussion about browsers, content negotiation, etc.


Thanks,
Damir



> --Jacob

Re: Proposal: http2 wire format

[Damir Simunic]

Hi,

> On 26 Mar 2018, at 05:11, Craig Ringer <cr...@2ndquadrant.com> wrote:
> 
> On 26 March 2018 at 06:00, Damir Simunic <damir.simu...@wa-research.ch> wrote:
>  
> > - Overhead for all clients. It may be tiny, but it needs to be
> >  measured and that cost needs to be weighed against the benefits.
> >  Maybe a cache miss in the context of a network connection is
> >  negligible, but we do need to know.
> 
> Important point. If h2 is to be seriously considered, then it must be an 
> improvement in absolutely every aspect.
> 
> The core part of this proposal is that h2 is parallel to v3. Something one 
> can opt into by compiling `--with_http2`.
> 
> IMO, a new protocol intended to supersede an old one must be a core, 
> non-optional feature. It won't reach critical mass of adoption if people 
> can't reasonably rely on it being there. There'll still be a multi-year lead 
> time as versions that support it become widespread enough to interest 
> non-libpq-based driver authors.

Agreed, it should be in core.

>  
> My PoC strategy is to touch existing code as little as possible. Yet if the 
> ProcessStartupPacket can somehow return the consumed bytes back to the TLS 
> lib for negotiation, then there’s zero cost to protocol detection for v2/v3 
> clients and only h2 clients pay the price of the extra check.
> 
> As others have noted, you'll want to find a way to handle this in the least 
> SSL-implementation-specific manner possible. IMO if it can't work with 
> OpenSSL, Windows's SSL implementation and OS X's SSL framework it's a 
> non-starter.

Understood.

Everyone that matters supports ALPN: 
https://en.wikipedia.org/wiki/Application-Layer_Protocol_Negotiation#Support

From the PoC standpoint, it’s now a straightforward chore to make sure it is 
supported for all possible build choices.

> 
> > - Dependency on a new external library. Fortunately, it's MIT
> >  licensed, so it's PostgreSQL compatible, but what happens if it
> >  becomes unmaintained? This has happened a couple of times, and it
> >  causes overhead that needs to be taken into account.
> 
> I chose nghttp because it gave me a quick start, it’s well designed, a good 
> fit for this kind of work, and fortunately indeed, the license is compatible. 
> (Also, curl links to it as well, so am pretty confident it’ll be around). 
> Very possible that over time h2 parsing code migrates into pg codebase. There 
> are so much similarities to v3 architecture, we might find a way to 
> generalize both into a single codebase. Then h2 frame parser/state machine 
> becomes only a handful of .c files.
> 
> h2 is a standard; however you decide to parse it, your code will eventually 
> converge to a stable state in the same manner that febe v3 code did. Once we 
> master the protocol, I don’t think there’ll be much need to touch the framing 
> code. IOW even if we just import what we need, it won’t be a big issue.
> 
> While I'm a big fan of code reuse and using existing libraries, I understand 
> others' hesitance here. Look at what happened with ossp-uuid; that was 
> painful and it was just a contrib.
> 
> It's a difficult balance between NIH and maintaining a stable core.

Enough important projects depend on libnghttp, I don’t think it will go away 
any time soon. And http2 is big; as more and more tools want to talk that 
protocol they’ll turn to libnghttp, so the signs of any troubles will be 
visible very very quickly.

>   
>  
> 
> * Is there merit in the idea of a completely new v4 protocol—one that freezes 
> the v3 and takes a new path?
> 
> Likely so... but it has to be pretty compelling IMO. And more importantly, 
> offer a smooth backwards- and forwards-compatible path.
>  
> 
> * What are the criteria for getting this into the core?
> 
> Mine would be: 
> 
> - No new/separate port required. Works on existing port.
> 
Check.

> - Doesn't break old clients connecting to new servers
> 
Check.

> - Doesn't break new clients connecting to old servers
> 

Old server sends “Invalid startup packet” and closes the connection; client’s 
TLS layer reports an error. Does that count as not breaking new clients? 

curl -v https://localhost:5432

...
* OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to localhost:5432
* stopped the pause stream!
* Closing connection 0
curl: (35) OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to 
localhost:5432

This applies to any TLS client (an h2-supporting libpq-fe will behave the same):
 
wget -v https://localhost:5432

Connecting to localhost|::1|:5432... connected.
Unable to establish SSL connection.


> - No extra round trips for new client -> old server . I don't personally care 
> about old client -> 

Proposal: http2 wire format

[Damir Simunic]

Hello hackers,



I’d like to propose the implementation of new wire protocol using http2 
framing. 

It appears to me that http2 solves many of the issues on the TODO list under 
“Wire Protocol Changes / v4 Protocol,“ without any obvious downsides. 

The implementation I have in mind has zero impact on existing clients. No 
changes to the format of existing v3 protocol. The new protocol works through a 
few small additions to postmaster.c to intercept TLS requests, and the rest in 
new source files, linked through PQcommMethods.

I’d like to emphasize that this proposal is empathically NOT about “let’s 
handle REST in the database” or some such. It’s about upgrading the framing, 
where http2 offers many benefits: content negotiation, concurrent bidirectional 
streams, extensible frame types, metadata/data split into headers/trailers and 
data frames, flow control, etc. It’s at least as efficient as febe v3. A lot of 
research is going into it to make it even more efficient and latency friendly. 
The mechanisms it provides for content negotiation, (and with ALPN, protocol 
negotiation), offers us a future-friendly way to evolve without the burden of 
backward compatibility compromises.

Before writing this proposal, I set out to create a proof of concept. My goal 
for the PoC is to be able to connect to the server using an existing http2 
client and get json back:

curl -k https://localhost:5432/some_func \
--http2-prior-knowledge --tlsv1.2 \
-H 'pg-database: postgres' \
-H 'pg-user: web'  \
-H ‘authorization: ….’
-H ‘accept: application/json’

{ result: [ … ] }

After spending a week getting up to speed with C, libpq internals, http2 
standard, libnghttp2 interface, etc., I’m fairly convinced that pg/http2 is 
feasible.

Sadly, my experience with C and Postgres internals is non-existent, and I am 
not yet able to finalize a live demo. The above curl request does establish the 
connection, receives the settings frame and queries the database, but I’m still 
struggling with writing code to return the http2 response. At this stage, it’s 
purely an issue of mechanically writing the code, I think I solved how it all 
works in principle.

If anyone finds the idea of Postgres speaking http2 appealing, I’d welcome 
guidance/mentoring/coding help (or just plain taking over). I a put up a repo 
with the results so far and a longer writeup: https://github.com/dsimunic/pg_h2 

All changes I made to the codebase are in a single commit, hopefully easy to 
understand what is happening. You’ll need libnghttp2 and openssl 1.0.2 or newer 
to compile.

My hope is that this post leads to a conversation and gets a few people excited 
about the idea the way I am. Maybe even some of the GSoC students would take 
the implementation further?


Damir