On Thu, Oct 24, 2024 at 2:30 PM Joel Jacobson <joel@compiler.org> wrote:
>
> On Thu, Oct 24, 2024, at 03:54, Masahiko Sawada wrote:
> > I have one question:
> >
> > From the 0001 patch's commit message:
> >
> > No behavioral changes are intended; this is a pure refactoring to improve code
> > clarity and maintainability.
> >
> > Does the reorganization of the option validation done by this patch
> > also help make the 0002 patch simple or small?
>
> Thanks for the review. No, not much, except the changes necessary to
> ProcessCopyOptions for raw, without also refactoring it, makes it
> more complicated.
>
> > If not much, while it
> > makes sense to me that introducing the CopyFormat enum is required by
> > the 0002 patch, I think we can discuss the reorganization part
> > separately. And I'd suggest the patch organization would be:
> >
> > 0001: introduce CopyFormat and replace csv_mode and binary fields with it.
> > 0002: add new 'raw' format.
> > 0003: reorganize option validations.
> >
/* Check force_quote */
- if (!opts_out->csv_mode && (opts_out->force_quote ||
opts_out->force_quote_all))
+ if (opts_out->format != COPY_FORMAT_CSV && (opts_out->force_quote ||
+ opts_out->force_quote_all))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
/*- translator: %s is the name of a COPY option, e.g. ON_ERROR */
maybe this has a code indentation issue.
since "if" and "opts_out" in the same column position.
It came to my mind,
change errmsg occurrence of "BINARY mode", "CSV mode" to "binary
format", "csv format" respectively.
I think "format" would be more accurate.
but the message seems invasive,
so i guess we need to use "mode".
overall v13-0001-Introduce-CopyFormat-and-replace-csv_mode-and-binary.patch
looks good to me.
