pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: torikoshia <torikoshia@oss.nttdata.com>
To: jian he <jian.universality@gmail.com>
Cc: zhjwpku@gmail.com
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Add new COPY option REJECT_LIMIT
Date: Thu, 3 Oct 2024 23:43:25 +0900
Message-ID: <8830518a-28ac-43a2-8a11-1676d9a3cdf8@oss.nttdata.com> (raw)
In-Reply-To: <d9636900e5a1b7117f5f42daf9be8cab@oss.nttdata.com>
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>



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






view thread (26+ messages)  latest in thread

Message-ID: <8830518a-28ac-43a2-8a11-1676d9a3cdf8@oss.nttdata.com>
Permalink:  ../8830518a-28ac-43a2-8a11-1676d9a3cdf8@oss.nttdata.com/
Also on:    postgresql.org/message-id/8830518a-28ac-43a2-8a11-1676d9a3cdf8@oss.nttdata.com

 · 

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: pgsql-hackers@postgresql.org
  Cc: masao.fujii@oss.nttdata.com, torikoshia@oss.nttdata.com, jian.universality@gmail.com, zhjwpku@gmail.com, pgsql-hackers@lists.postgresql.org
  Subject: Re: Add new COPY option REJECT_LIMIT
  In-Reply-To: <8830518a-28ac-43a2-8a11-1676d9a3cdf8@oss.nttdata.com>

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

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