Re: Extend COPY FROM with HEADER <integer> to skip multiple lines

On Mon, Jun 30, 2025 at 4:24 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
>
> > > 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.

Thank you for the review. The change looks good to me, too.  A new
patch is attached.

> Regarding the documentation, how about explicitly stating that when MATCH is specified, only
> the first line is skipped? While this may seem obvious, it’s worth clarifying, as the semantics
> of the HEADER option have become a bit more complex with this change.

Agreed.  I have updated the documentation as follows:

+      lines are discarded.  If the option is set to <literal>MATCH</literal>,
+      the number and names of the columns in the header line must exactly
+      match those of the table and, in order, after which the header line is
+      discarded;  otherwise an error is raised.  The <literal>MATCH</literal>


-- 
Best regards,
Shinya Kato
NTT OSS Center