Re: COPY ON_CONFLICT TABLE; save duplicated record to another table. - Mailing list pgsql-hackers

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