public inbox for [email protected]  

help / color / mirror / Atom feed

From: Fujii Masao <[email protected]>
To: torikoshia <[email protected]>
To: Michael Paquier <[email protected]>
To: [email protected]
Cc: Pgsql Hackers <[email protected]>
Subject: Re: Add on_error and log_verbosity options to file_fdw
Date: Wed, 11 Sep 2024 23:50:25 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<CAD21AoBij_2cy5T7ihk9ArRQzDcXVHoK13vzy34zAUAMLgrKkQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>



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

view thread (13+ 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]
  Subject: Re: Add on_error and log_verbosity options to file_fdw
  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