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