Message-ID: <8830518a-28ac-43a2-8a11-1676d9a3cdf8@oss.nttdata.com>
Date: Thu, 3 Oct 2024 23:43:25 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add new COPY option REJECT_LIMIT
Content-Language: en-US
To: torikoshia <torikoshia@oss.nttdata.com>,
 jian he <jian.universality@gmail.com>
Cc: zhjwpku@gmail.com, pgsql-hackers@lists.postgresql.org
References: <63f99327aa6b404cc951217fa3e61fe4@oss.nttdata.com>
 <fab7018f-6e00-4be4-bd1e-4a013096f6c9@oss.nttdata.com>
 <de0551d0f9d9e9072e324e51ed5c426d@oss.nttdata.com>
 <6e3dcdc6-8e18-4716-a23b-3bee316aeaa7@oss.nttdata.com>
 <979c6d306343f3d665d14eab2cd1b7bc@oss.nttdata.com>
 <920a29a36032befbeb355cbdfbbe0f63@oss.nttdata.com>
 <a873643c-e7d1-456c-b16b-6162ab698036@oss.nttdata.com>
 <54bea7dd-94b1-41f1-a4e9-c5c53cc82ce7@oss.nttdata.com>
 <f9feda0ecd4e2e09620a6f9ca8f4aac1@oss.nttdata.com>
 <644a094c-2c0c-4b5f-b151-3677c7de4ee7@oss.nttdata.com>
 <5f807fcf3a36df7ba41464ab40b5c37d@oss.nttdata.com>
 <056a5728cf733685cff7d9b5a8fbb0ad@oss.nttdata.com>
 <a0343e25-644e-4435-8399-f2e74c04ca23@oss.nttdata.com>
 <dd542f3ce7e15cde7e53824041f2a2f4@oss.nttdata.com>
 <CACJufxEh5e_5BOmP7RAHcuV=e32FpzBMxJ8-W-OFXm-zkdY4yw@mail.gmail.com>
 <d9636900e5a1b7117f5f42daf9be8cab@oss.nttdata.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <d9636900e5a1b7117f5f42daf9be8cab@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/8830518a-28ac-43a2-8a11-1676d9a3cdf8%40oss.nttdata.com>
Precedence: bulk


On 2024/09/30 21:08, torikoshia wrote:
> Since defGetInt64() checks not only whether the input value exceeds the range of bigint but also input value is specified, attached v6 patch checks them by directly calling defGetInt64().

Thanks for updating the patch! But the patch no longer applied cleanly
to the master branch. Could you rebase it?


+	if (opts_out->reject_limit && !opts_out->on_error)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+		/*- translator: first and second %s are the names of COPY
+		 * option, e.g. ON_ERROR, third is the value of the COPY option,
+		 * e.g. IGNORE */
+				 errmsg("COPY %s requires %s to be set to %s",
+						 "REJECT_LIMIT", "ON_ERROR", "IGNORE")));

This check might be better moved to other place, for example at
the bottom of ProcessCopyOptions(). I noticed a comment in the code:
"Check for incompatible options (must do these two before inserting defaults)."
You added your condition after this comment and before inserting the defaults,
but it's not related to that process. So, moving this check to other place
seems more appropriate.

While reviewing, I also noticed that the check for
"opts_out->binary && opts_out->on_error != COPY_ON_ERROR_STOP"
is similarly placed before setting the defaults, which might not
be correct. This check should probably be moved as well.
Additionally, the comment mentioning "must do these two" should be
updated to "must do these three." These changes should be handled
in a separate patch.


+			if (cstate->opts.reject_limit &&

Wouldn't it be clearer to use cstate->opts.reject_limit > 0 for better readability?


+				cstate->num_errors > cstate->opts.reject_limit)
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+						 errmsg("skipped more than REJECT_LIMIT rows: \"%lld\",",
+								(long long) cstate->opts.reject_limit)));

To make the error message clearer, similar to the NOTICE about
skipped rows, how about rewording it to:
"skipped more than REJECT_LIMIT (%lld) rows due to data type incompatibility"?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION