public inbox for [email protected]  
help / color / mirror / Atom feed
From: torikoshia <[email protected]>
To: Alena Rybakina <[email protected]>
Cc: Damir Belyalov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Andrey Lepikhov <[email protected]>
Subject: Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
Date: Tue, 09 May 2023 23:23:11 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<CALH1LgvQ5eRMi3zO7RSmod4ucru=9ekASpReahR7zDcCabANZw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CALH1LguAEsoTYJTCsXNB-7z2Hu9UGEpsXA4kj0FOTmoP=6Wp3Q@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CALH1LguOY-VvJBWvjpLR2W3t0Z2p=LGSK5tzLrKwtzZ2UsqwGg@mail.gmail.com>
	<[email protected]>
	<CALH1LgseNT7mxpGzQsQwOJz_UwYBo6L-4vR9tmJqb-WNPkt6Bg@mail.gmail.com>
	<[email protected]>

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






view thread (4+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox