Re: COPY ON_CONFLICT TABLE; save duplicated record to another table.

Hi Jian

On 25/04/2026 06:12, jian he wrote:
> Comments are welcome!

Thanks for the patch!

A few comments:

== double defGet in function name ==

defGetdefGetCopyOnConflictChoice should probably be
defGetCopyOnConflictChoice

== identical error message in ProcessCopyOptions() ==

The same error message is raised for conflict_tbl_specified and
!conflict_tbl_specified

ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY %s requires %s option", "CONFLICT_TABLE", "ON_CONFLICT"));

== unused enum in CopyOnErrorChoice ==

The enum COPY_ON_ERROR_TABLE is introduced, but is never used anywhere.

== unconditional ExecOpenIndices() ==

ExecOpenIndices(resultRelInfo, true);

I'm not very familiar with this part of the code, but it looks like that
this change would affect other COPY FROM operations. If I'm mistaken, a
comment would add some value here. Or perhaps something like:

ExecOpenIndices(resultRelInfo, cstate->opts.on_conflict != ONCONFLICT_NONE);

== ON_CONFLICT TABLE not rejected in COPY TO ==

CONFLICT_TABLE is silently ignored, even if the table does not exist:

postgres=# COPY t TO '/dev/null' (ON_CONFLICT TABLE, CONFLICT_TABLE
table_does_not_exist);
COPY 1

I guess adding a is_from to defGetdefGetCopyOnConflictChoice() is the
way to go.

== redundant condition in CopyFrom() ==

The second condition seems unnecessary, as the previous if already tests
for cstate->opts.on_conflict == ONCONFLICT_NONE:

else if (resultRelInfo->ri_NumIndices > 0 &&
cstate->opts.on_conflict != ONCONFLICT_NONE)

== typos ==
regular realtion > regular relation
vertification > verification
resouces > resources
unqiue > unique

== unnecessary pnstrdup (?) ==

newvalues[i] = CStringGetTextDatum(pnstrdup(cstate->line_buf.data,
cstate->line_buf.len));

Is the duplication really necessary? Wouldn't it suffice to use
cstring_to_text_with_len() instead? Something like:

newvalues[i] =
PointerGetDatum(cstring_to_text_with_len(cstate->line_buf.data,
cstate->line_buf.len));

or even

newvalues[i] = CStringGetTextDatum(cstate->line_buf.data)

I'll check the documentation after we get more feedback on the syntax.

Thanks!
Best, Jim