Message-ID: <1b24fcc9-0f18-40bf-b532-ab4cbe4b9e32@oss.nttdata.com>
Date: Wed, 25 Sep 2024 00:46:24 +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>
Cc: Michael Paquier <michael@paquier.xyz>, sawada.mshk@gmail.com,
 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>
 <b0d44cd9-a7f7-469c-9ecf-b0115b86ca70@oss.nttdata.com>
 <5739a8ee4320a31ca04752fe3972aa73@oss.nttdata.com>
 <81844000-703c-408a-9296-14f5e176098d@oss.nttdata.com>
 <156b2c1bfe27441aa5e59a52d0af5142@oss.nttdata.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <156b2c1bfe27441aa5e59a52d0af5142@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/1b24fcc9-0f18-40bf-b532-ab4cbe4b9e32%40oss.nttdata.com>
Precedence: bulk


On 2024/09/24 20:08, torikoshia wrote:
> Thanks for the explanation and suggestion.
> Since there is almost the same code in copyfrom.c, attached 0003 patch for refactoring both.

Thanks for updating the patches!

Regarding 0002.patch, I think it’s better to include the refactored code
from the start rather than adding redundant code intentionally.
How about leaving just the refactor in copyfrom.c to 0003.patch?
If that works, as a refactoring, you could also replace "skipped" with
"cstate->num_errors" in that patch, as you suggested earlier.

While reviewing again, I noticed that running ANALYZE on a file_fdw
foreign table also calls NextCopyFrom(), but it doesn’t seem to
skip erroneous rows when on_error is set to "ignore." This could lead
to inaccurate statistics. Shouldn’t ANALYZE on file_fdw foreign tables
with on_error=ignore also skip erroneous rows?

>> The tab-completion needs to be updated to support the "silent" option?
> 
> Yes, updated 0002 patch.

Thanks! Also, this should be part of 0001.patch since "silent" is
introduced there, right?

Regards,

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