Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

On 2023-05-07 05:05, Alena Rybakina wrote:
Thanks for your reviewing and comments!

> I noticed that you used _ignore_datatype_errors_specified_ variable in
> _copy.c_ , but guc has a short name _ignore_datatype_errors_. Also you
> used the short variable name in _CopyFormatOptions_ structure.

You may already understand it, but these variable names are given in 
imitation of FREEZE and BINARY cases:

   --- a/src/include/commands/copy.h
   +++ b/src/include/commands/copy.h
   @@ -42,6 +42,7 @@ typedef struct CopyFormatOptions
                                    * -1 if not specified */
       bool        binary;         /* binary format? */
       bool        freeze;         /* freeze rows on loading? */
   +   bool        ignore_datatype_errors;  /* ignore rows with datatype 
errors */

   --- a/src/backend/commands/copy.c
   +++ b/src/backend/commands/copy.c
   @@ -419,6 +419,7 @@ ProcessCopyOptions(ParseState *pstate,
       bool        format_specified = false;
       bool        freeze_specified = false;
       bool        header_specified = false;
   +   bool        ignore_datatype_errors_specified = false;


> Name used _ignore_datatype_errors_specified _is seemed very long to
> me, may be use a short version of it (_ignore_datatype_errors_) in
> _copy.c_ too?

I think it would be sane to align the names with the FREEZE and BINARY 
options.

I agree with the name is too long and we once used the name 
'ignore_errors'.
However, current implementation does not ignore all errors but just data 
type error, so I renamed it.
There may be a better name, but I haven't come up with one.

> Besides, I noticed that you used _ignored_errors_ variable in
> _CopyFromStateData_ structure and it's name is strikingly similar to
> name (_ignore_datatype_error__s_), but they have different meanings.
> Maybe it will be better to rename it as _ignored_errors_counter_?

As far as I take a quick look at on PostgreSQL source code, there're few 
variable name with "_counter". It seems to be used for function names.
Something like "ignored_errors_count" might be better.


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION