| From: | jian he <jian(dot)universality(at)gmail(dot)com> |
|---|---|
| To: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: on_error table, saving error info to a table |
| Date: | 2026-05-25 08:13:44 |
| Message-ID: | CACJufxFd1pbip3SXcwTshWN21fCWOknKQHZ5c8tVYfPGWPQ-7g@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Fri, May 15, 2026 at 7:32 PM Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:
>
> > 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.
>
With v13, I have added:
+ <para>
+ Triggers on <replaceable
class="parameter">error_saving_table</replaceable>
+ will be fired. Therefore, the <literal>NOTICE</literal> message regarding
+ rows inserted into <replaceable
class="parameter">error_saving_table</replaceable>
+ may differ from what is actually being inserted.
+ </para>
> + /* 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.
Searching the codebase for ExecGetReturningSlot or ri_ReturningSlot shows that
this is its only call site for CopyFromState->error_rel
(Note that in ExecInsert, resultRelInfo->ri_projectReturning is NULL
for the CopyFromState->error_rel).
In ExecInsert, we also have these comments:
---------
* Using ExecGetReturningSlot() to store the tuple for the
* recheck isn't that pretty, but we can't trivially use
* the input slot, because it might not be of a compatible
* type. As there's no conflicting usage of
* ExecGetReturningSlot() in the DO NOTHING case...
---------
Therefore I think it's safe to reuse ResultRelInfo->ri_ReturningSlot.
Regarding the trigger behavior, with v13:
Statement-level and row-level triggers will fire for every error
record insertion, which behaves very similarly to
ExecForPortionOfLeftovers.
I have attached version 13, which should resolve the rest of the
issues you raised.
| Attachment | Content-Type | Size |
|---|---|---|
| v13-0002-COPY-FROM-on_error-table-error_table-errtbl.patch | text/x-patch | 56.8 KB |
| v13-0001-export-ExecInsert.patch | text/x-patch | 4.8 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Michael Paquier | 2026-05-25 08:21:49 | Re: NULL pointer dereference in syslogger with load_libraries() and -DEXEC_BACKEND at startup |
| Previous Message | Shinya Kato | 2026-05-25 08:09:47 | Re: Use pg_current_xact_id() instead of deprecated txid_current() |