Re: Extend COPY FROM with HEADER to skip multiple lines

On Sat, 28 Jun 2025 00:33:45 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

> 
> 
> On 2025/06/27 13:09, Yugo Nagata wrote:
> > On Fri, 27 Jun 2025 12:22:17 +0900
> > Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > 
> >>> However, your patch is clear, and it seems we don't need to worry about this concern.
> >>> Your patch looks good to me.
> >>
> >> Thanks for reviewing the patch! I've marked it as ready for committer.
> > 
> > I have a few minor comment on the patch.
> > 
> > +                if (ival < 0)
> > +                    ereport(ERROR,
> > +                            (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > +                             errmsg("%s requires a Boolean value, a non-negative "
> > +                                    "integer, or the string \"match\"",
> > +                                    def->defname)));
> > 
> >       ereport(ERROR,
> >               (errcode(ERRCODE_SYNTAX_ERROR),
> > -             errmsg("%s requires a Boolean value or \"match\"",
> > +             errmsg("%s requires a Boolean value, a non-negative integer, "
> > +                    "or the string \"match\"",
> >                       def->defname)));
> > 
> > These two pieces of code raise the same error, but with different error codes:
> > one returns ERRCODE_INVALID_PARAMETER_VALUE, while the other returns ERRCODE_SYNTAX_ERROR.
> > 
> > I believe the former is more appropriate, although the existing code uses the
> > latter.　In any case, the error codes should be consistent.
> 
> I'm not sure there's an actual rule like "the error code must match
> if the error message is the same." But if using the same message with
> a different error code is confusing, I'm fine with changing
> the earlier message. For example:
> 
>                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> -                             errmsg("%s requires a Boolean value, a non-negative "
> -                                    "integer, or the string \"match\"",
> +                             errmsg("a negative integer value cannot be "
> +                                    "specified for %s",
>                                       def->defname)));

Fair point. There might not be any explicit rule.
However, I feel it somewhat confusing.

After looking into the code, it seems that ERRCODE_SYNTAX_ERROR tends to be used
for invalid keywords or parameter types, while ERRCODE_INVALID_PARAMETER_VALUE
is typically used for values that are out of the acceptable range.

So, the proposed change seems reasonable to me.

Regards,
Yugo Nagata

-- 
Yugo Nagata <nagata@sraoss.co.jp>