MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Thu, 21 Nov 2024 15:43:22 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Yugo NAGATA <nagata@sraoss.co.jp>, Pgsql Hackers
 <pgsql-hackers@postgresql.org>
Subject: Re: Add reject_limit option to file_fdw
In-Reply-To: <336dd0aa-f91b-4953-b0a0-b87215c571d2@oss.nttdata.com>
References: <bab68a9fc502b12693f0755b6f35f327@oss.nttdata.com>
 <fd14c3a0-a036-4bce-b5f8-4af83259eae1@oss.nttdata.com>
 <54747389a562688eab7077b144daff6a@oss.nttdata.com>
 <2b3ad6d4-e05f-4188-8cfe-1fd9d949a9c9@oss.nttdata.com>
 <192ede1e4c2b34b853e5ac46f8dee50b@oss.nttdata.com>
 <d7a03664-bef5-49b0-998f-3ea5356b53c9@oss.nttdata.com>
 <f02ec7b6c348b9fbeeb69657733da9ba@oss.nttdata.com>
 <20241112145114.ba288defc684699bf8646942@sraoss.co.jp>
 <2f03bf95556c4c41d6b1659e74b346c7@oss.nttdata.com>
 <20241113231706.09e5b5ea9640289312835be8@sraoss.co.jp>
 <a7d70e35229e2075b371cd1e81222244@oss.nttdata.com>
 <336dd0aa-f91b-4953-b0a0-b87215c571d2@oss.nttdata.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <ed549b14a025b7f4b7451c035205d762@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ed549b14a025b7f4b7451c035205d762%40oss.nttdata.com>
Precedence: bulk

On 2024-11-21 00:43, Fujii Masao wrote:
> On 2024/11/19 21:40, torikoshia wrote:
>>> These messages may be unexpected for some users because the 
>>> documentation of
>>> fild_fdw does not explicitly describe that file_fdw uses COPY 
>>> internally.
>>> (I can find several wordings like "as COPY", though.)
>>> However, since the current file_fdw already has such messages, I came 
>>> to think
>>> making the messages on "reject_limit" consistent with COPY is 
>>> reasonable.
>>> I mean, the previous version of the message seems fine.
>> 
>> Agreed. Thanks for your investigation.
> 
> I've pushed the 0001 patch. Thanks!

Thanks a lot!

>>> As another comment, do we need to add validator test for on_error and
>>> reject_limit as similar to other options?
>> 
>> That might be better.
>> Added some queries which had wrong options to cause errors.
>> 
>> I was unsure whether to add it to "validator test" section or 
>> "on_error, log_verbosity and reject_limit tests" section, but I chose 
>> "validator test" as I believe it makes things clearer.
>> 
>> Added 0002 patch since some of the tests are not related to 
>> reject_limit but just on_error.
> 
> How about adding validator tests for log_verbosity and reject_limit=0,
> similar to the tests in the copy2.sql regression test? I've added those
> two tests, and the latest version of the patch is attached.

Thanks for the update! Agreed.
I think the validator tests related to the on_error option you added are 
sufficient.

As for the other file_fdw options, I'm a bit concerned whether it would 
be better to add validator tests for cases like when 'encoding' is 
'unsupported', but that should be discussed in another thread.

> Regards,

-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.