Re: on_error table, saving error info to a table

> I am not sure.
> Should we produce the NOTICE message below for the above test case?
> +NOTICE: 5 rows were saved to table "err_tbl2" due to data type incompatibility
> but because of the trigger, err_tbl2 has zero rows.

I am also not sure about the exact details, however silently dropping
rows from both the target and error table seems wrong to me, so I
would definitely add some output about this happening.

In my example I used a trigger that always returns null, but it is
also possible that we deal with something that only returns null
sometimes. And then we get an output like:

NOTICE: 1 row was saved to table "err_tbl" due to data type incompatibility
COPY 2

With an input or 4 or 5 rows.

+ else if (cstate->opts.on_error == COPY_ON_ERROR_TABLE)
+ ereport(NOTICE,
+ errmsg("saving error information to table \"%s\" row due to
data type incompatibility at line %" PRIu64 " for column \"%s\":
\"%s\"",
+ RelationGetRelationName(cstate->error_rel),
+ cstate->cur_lineno,
+ cstate->cur_attname,
+ attval));

Also this notice is emitted even if the before trigger returns null.
The wording suggest that it is in progress ("saving", not "saved"),
but then it can be still confusing to see a notice about this and then
no result.

I would also add documentation about how triggers work on the error
table, for example (silently part is current behavior - if that
changes, the documentation should too):

Statement-level triggers (BEFORE/AFTER INSERT FOR EACH STATEMENT)
on `error_saving_table` are fired on every COPY FROM that specifies
this table, regardless of whether any errors occurred. Row-level
triggers fire once per error row. A BEFORE INSERT FOR EACH ROW
trigger that returns NULL silently drops the audit row.

+ /*
+ * Copy other important information into the EState, this aligned with
+ * ExecutorStart
+ */
+ estate->es_snapshot = RegisterSnapshot(GetActiveSnapshot());
+ estate->es_crosscheck_snapshot = RegisterSnapshot(InvalidSnapshot);
+

Maybe I'm missing something, but is this required for the patch?

+ /* Must have INSERT privilege on the table */
+ aclresult = pg_class_aclcheck(RelationGetRelid(rel),
+ GetUserId(),
+ ACL_INSER
...
+
+ ExecCheckPermissions(pstate->p_rtable, list_make1(perminfo), true);

Isn't the first redundant with ExecCheckPermissions?

+ if (opts_out->reject_limit)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot set option %s when %s is specified as \"%s\"",
"REJECT_LIMIT", "ON_ERROR", "TABLE"));

Isn't this check repeated later with a different error message? "COPY
REJECT_LIMIT requires ON_ERROR to be set to IGNORE"

+ /* Prepare to build the result tuple */
+ TupleTableSlot *myslot = ExecGetReturningSlot(estate,
+ mtstate->resultRelInfo);

Is reusing the returning slot for this the proper way to do it? I'm
not saying that it's wrong, but I'm unsure about this.

+ /* TODO: Support cstate->error_rel when it is a partitioned table */

Is this todo relevant? Isn't this code unreachable with partitioned tables?

There are also a few typos:

realtion => relation
resouces => resources
vertified => verified