Re: on_error table, saving error info to a table

On Fri, May 15, 2026 at 1:14 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> SET debug_discard_caches = 1;
> CREATE TABLE src_tbl (a int, b int);
> CREATE TABLE err_tbl OF copy_error_saving;
> CREATE INDEX src_idx ON src_tbl(a);
> COPY src_tbl FROM STDIN (FORMAT csv, ON_ERROR table, ERROR_TABLE err_tbl);
> 1,2
> xx,3
> 3,4
> \.
> -- ERROR: relation with OID 0 does not exist
>
Yech. We need do this in EndCopyFrom
+    if (cstate->error_rel)
+        table_close(cstate->error_rel, NoLock);

>
> +               /* Handle queued AFTER triggers */
> +               AfterTriggerEndQuery(cstate->mtcontext->estate);
>
> Is the order of this correct? See the following snippet that crashes the server:
>
> CREATE TABLE target_tbl (id int, val int);
> CREATE TABLE err_tbl OF copy_error_saving;
>
> CREATE OR REPLACE FUNCTION err_stmt_trans_fn() RETURNS trigger AS $$
> BEGIN
> END;
> $$ LANGUAGE plpgsql;
>
> CREATE TRIGGER err_stmt_trans
> AFTER INSERT ON err_tbl
> REFERENCING NEW TABLE AS new_rows
> FOR EACH STATEMENT EXECUTE FUNCTION err_stmt_trans_fn();
>
> \echo === COPY ===
> COPY target_tbl FROM stdin WITH (on_error 'table', error_table 'err_tbl');
> 1       100
> bad     200
> 3       notanumber
> 4       400
> \.
>

We need do AfterTriggerEndQuery(cstate->mtcontext->estate);
first then
AfterTriggerEndQuery(estate);

ExecForPortionOfLeftovers() handles this in the same way.

``static AfterTriggersData afterTriggers;``
and ExecASInsertTriggers, AfterTriggerEndQuery will change the value
of afterTriggers constantly,
therefore, the position of these functions is important!

Therefore in copyfrom.c the order should be:
ExecBSInsertTriggers(estate, resultRelInfo);
ExecBSInsertTriggers(cstate->mtcontext->mtstate->ps.state,
                      cstate->mtcontext->mtstate->rootResultRelInfo);
ExecASInsertTriggers(cstate->mtcontext->estate,
            on_error_mtstate->rootResultRelInfo,
            on_error_mtstate->mt_transition_capture);
ExecASInsertTriggers(estate, target_resultRelInfo, cstate->transition_capture);

> +
> +                               cstate->num_errors = cstate->num_errors + estate->es_processed;
>
> Counting seems to miss if a before trigger returns null:
>
> \set ON_ERROR_STOP 0
> CREATE TABLE t2 (a int, b int, c int);
> CREATE TABLE err_tbl2 OF copy_error_saving;
> CREATE FUNCTION drop_all() RETURNS TRIGGER LANGUAGE plpgsql AS $$
> BEGIN
>   RETURN NULL;
> END;
> $$;
> CREATE TRIGGER drop_all_t BEFORE INSERT ON err_tbl2 FOR EACH ROW
>   EXECUTE FUNCTION drop_all();
> COPY t2 FROM STDIN WITH (FORMAT csv, ON_ERROR table, ERROR_TABLE err_tbl2);
> 1,2,a
> 3,4,b
> 5,6,c
> 7,8,d
> 9,10,e
> \.
>
> SELECT count(*) AS n FROM t2;
> SELECT count(*) AS n FROM err_tbl2;
>
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.

>
> +               typoid = GetSysCacheOid2(TYPENAMENSP, Anum_pg_type_oid,
> +                                                                PointerGetDatum("copy_error_saving"),
> +                                                                ObjectIdGetDatum(PG_CATALOG_NAMESPACE));
> ...
> +                       if (reloftype != typoid)
> +                               ereport(ERROR,
> ...
> +                   errhint("The COPY error saving table must be a
> typed table based on type \"%s\".",
> +                                                               format_type_be_qualified(typoid)));
>
> Isn't an if (!OidIsValid(typoid)) check missing between the two?

Sure, I have added a
+        if (!OidIsValid(typoid))
+            elog(ERROR, "cache lookup failed for catalog type %s",
"copy_error_saving");



--
jian
https://www.enterprisedb.com/