| From: | jian he <jian(dot)universality(at)gmail(dot)com> |
|---|---|
| To: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> |
| Cc: | Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: COPY ON_CONFLICT TABLE; save duplicated record to another table. |
| Date: | 2026-05-12 08:15:19 |
| Message-ID: | CACJufxE9L5vVkBxjRZRjbpsGJcSLXM1oyayr_u10TA_ZfyFvmg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Tue, May 12, 2026 at 4:40 AM Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:
>
> Hello!
>
> > One other thing I just noticed in BINARY mode. I believe we're missing a
> > check in ProcessCopyOptions() with ON_CONFLICT TABLE to show a proper
> > error message, e.g.
>
> It is possible to crash the current patch with binary mode, that check
> is definitely needed.
>
binary mode lacks the concept of a line number or the whole line string.
Since cstate->line_buf is null in binary mode, it will segfault in
```CStringGetTextDatum(cstate->line_buf.data);```
Supporting ON_CONFLICT in binary mode is not trivial.
Since ON_ERROR IGNORE also cannot be used in binary mode, not
supporting ON_CONFLICT in binary mode should be fine, IMHO.
>
> + MakeTransitionCaptureState(cstate->conflictRel->trigdesc,
> + RelationGetRelid(cstate->conflictRel),
> + CMD_INSERT);
>
> Shouldn't this update mtstate->mt_transition_capture?
>
Attached v3 fix this issue.
> + if (cstate->opts.on_conflict == ONCONFLICT_TABLE)
> ...
> + if (conflict_mstate->fireBSTriggers)
> + {
> + ExecBSInsertTriggers(conflict_mstate->ps.state,
> conflict_mstate->rootResultRelInfo);
> +
> + conflict_mstate->fireBSTriggers = false;
> + }
> +
>
> and
>
> + if (cstate->num_conflicts > 0)
> + {
> ...
> + /* Execute AFTER STATEMENT insertion triggers */
> + ExecASInsertTriggers(cstate->mtcontext->estate,
> + on_conflict_mtstate->rootResultRelInfo,
> + on_conflict_mtstate->mt_transition_capture);
>
>
> * Doesn't statements typically fire triggers unconditionally? INSERT
> ON CONFLICT DO NOTHING; fires BS+AS triggers even if it doesn't insert
> any rows.
> * Isn't firing a before statement trigger after some before/after row
> triggers were already fired (for non conflicting rows) strange?
>
Ok. I changed to
Statement-level triggers on the CONFLICT_TABLE are fired
unconditionally, regardless of whether an error occurred or not.
Each row inserted into the CONFLICT_TABLE will fire both the BEFORE
INSERT FOR EACH ROW and AFTER INSERT FOR EACH ROW triggers.
> + if (valid_col_count != 4)
> + errdetail_msg = _("The conflict_table is incomplete; exactly four
> columns are required.");
>
> if valid_col_count > 4, is it still incomplete, shouldn't the error
> message change in that case?
With v3, whether there are more or fewer columns on the
conflict_table, the error message is now the same:
+ERROR: cannot use relation "err_tbl1" for COPY on_conflict error saving
+DETAIL: The conflict_table should only have four columns
+HINT: The conflict_table must contain exactly four columns with data
types, in order: OID, TEXT, BIGINT, TEXT
Regression tests for permission checks have also been added.
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-export-ExecInsert.patch | text/x-patch | 4.7 KB |
| v3-0002-COPY-ON_CONFLICT-TABLE.patch | text/x-patch | 63.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Maksim.Melnikov | 2026-05-12 08:30:55 | Re: Incorrect checksum in control file with pg_rewind test |
| Previous Message | Alexander Pyhalov | 2026-05-12 08:09:23 | Re: Function scan FDW pushdown |