Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps  (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1soOfy-00Aw2o-Be
	for pgsql-hackers@arkaria.postgresql.org; Wed, 11 Sep 2024 14:50:35 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.94.2)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1soOfx-001m1L-Jh
	for pgsql-hackers@arkaria.postgresql.org; Wed, 11 Sep 2024 14:50:33 +0000
Received: from makus.postgresql.org ([2001:4800:3e1:1::229])
	by malur.postgresql.org with esmtps  (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <masao.fujii@oss.nttdata.com>)
	id 1soOfx-001m10-8V
	for pgsql-hackers@lists.postgresql.org; Wed, 11 Sep 2024 14:50:33 +0000
Received: from oss.nttdata.com ([49.212.34.109])
	by makus.postgresql.org with esmtps  (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <masao.fujii@oss.nttdata.com>)
	id 1soOft-000eOW-Qj
	for pgsql-hackers@postgresql.org; Wed, 11 Sep 2024 14:50:32 +0000
Received: from [172.16.104.125] (122x216x53x202.ap122.ftth.ucom.ne.jp [122.216.53.202])
	by oss.nttdata.com (Postfix) with ESMTPSA id C313661271;
	Wed, 11 Sep 2024 23:50:25 +0900 (JST)
X-Virus-Status: Clean
X-Virus-Scanned: clamav-milter 0.103.11 at oss.nttdata.com
Message-ID: <b0d44cd9-a7f7-469c-9ecf-b0115b86ca70@oss.nttdata.com>
Date: Wed, 11 Sep 2024 23:50:25 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add on_error and log_verbosity options to file_fdw
Content-Language: en-US
To: torikoshia <torikoshia@oss.nttdata.com>,
 Michael Paquier <michael@paquier.xyz>, sawada.mshk@gmail.com
Cc: Pgsql Hackers <pgsql-hackers@postgresql.org>
References: <ab59dad10490ea3734cf022b16c24cfd@oss.nttdata.com>
 <841721f3ca9a69574a72eb141c83933c@oss.nttdata.com>
 <CAD21AoBij_2cy5T7ihk9ArRQzDcXVHoK13vzy34zAUAMLgrKkQ@mail.gmail.com>
 <Zp7x8Y-SpbkZtpE-@paquier.xyz>
 <c22f047faf8aea56acecb26449b3f454@oss.nttdata.com>
 <79a336ccebc70401625743686c41b81a@oss.nttdata.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <79a336ccebc70401625743686c41b81a@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: <https://www.postgresql.org/message-id/b0d44cd9-a7f7-469c-9ecf-b0115b86ca70%40oss.nttdata.com>
Precedence: bulk



On 2024/08/08 16:36, torikoshia wrote:
> Attached patches.
> 0001 adds new option 'silent' to log_verbosity and 0002 adds on_error and log_verbosity options to file_fdw.

Thanks for the patches!

Here are the review comments for 0001 patch.

+      <literal>silent</literal> excludes verbose messages.

This should clarify that in silent mode, not only verbose messages but also
default ones are suppressed?

+		cstate->opts.log_verbosity != COPY_LOG_VERBOSITY_SILENT)

I think using "cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT" instead
might improve readability.

-	COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages, default */
-	COPY_LOG_VERBOSITY_VERBOSE, /* logs additional messages */
+	COPY_LOG_VERBOSITY_SILENT = -1,	/* logs none */
+	COPY_LOG_VERBOSITY_DEFAULT = 0,	/* logs no additional messages, default */
+	COPY_LOG_VERBOSITY_VERBOSE,	/* logs additional messages */

Why do we need to assign specific numbers like -1 or 0 in this enum definition?


Here are the review comments for 0002 patch.

+			pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
+										 ++skipped);

The skipped tuple count isn't accurate because fileIterateForeignScan() resets
"skipped" to 0.

+		if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
+			cstate->escontext->error_occurred)
+		{
+			/*
+			 * Soft error occurred, skip this tuple and deal with error
+			 * information according to ON_ERROR.
+			 */
+			if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)

If COPY_ON_ERROR_IGNORE indicates tuple skipping, shouldn’t we not only reset
error_occurred but also call "pgstat_progress_update_param" and continue within
this block?

+	for(;;)
+	{

Using "goto" here might improve readability instead of using a "for" loop.

Regards,

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