Horton

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

torikoshia <torikoshia@oss.nttdata.com>

View thread
From: torikoshia <torikoshia@oss.nttdata.com>
To: Alena Rybakina <lena.ribackina@yandex.ru>
Cc: Damir Belyalov <dam.bel07@gmail.com>, andres@anarazel.de, tgl@sss.pgh.pa.us, daniel@yesql.se, pgsql-hackers@postgresql.org, anisimow.d@gmail.com, HukuToc@gmail.com, Andrey Lepikhov <a.lepikhov@postgrespro.ru>
Date: 2023-05-09T14:23:11Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. doc: Fix COPY ON_ERROR option syntax synopsis.

  2. Disallow specifying ON_ERROR option without value.

  3. Rename COPY option from SAVE_ERROR_TO to ON_ERROR

  4. Fix spelling in notice

  5. Add new COPY option SAVE_ERROR_TO

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