MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Mon, 26 Feb 2024 21:17:36 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>,
 jian.universality@gmail.com, Pgsql Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add new error_action COPY ON_ERROR "log"
In-Reply-To: <5f5c3e316d7f24e31d8983f32be0f2bc@oss.nttdata.com>
References: <9e40f7a7864c0456ab9b8d3ff85b59d0@oss.nttdata.com>
 <CAKFQuwa8ZEmh0LAbdUY-oO3G+iN7nW8GDFNMNLw4Tq+s798scQ@mail.gmail.com>
 <a6e223a41d864f685e55f6a79cd3d8d9@oss.nttdata.com>
 <CALj2ACUk700cYhx1ATRQyRw-fBM+aRo6auRAitKGff7XNmYfqQ@mail.gmail.com>
 <3bad6fd5570c5b63e0b68f79e20253f2@oss.nttdata.com>
 <CALj2ACUK0LaC5CeR9aFb+Gc+oiuSsWeUGmUDEPWd4d2j-+uXOA@mail.gmail.com>
 <0e76f13b74f6729958321aba4a32a2cf@oss.nttdata.com>
 <CALj2ACVMBrzO5ic+XawNqkUEf3_9diUCEE-=yso_3RoEH7D5Uw@mail.gmail.com>
 <5f5c3e316d7f24e31d8983f32be0f2bc@oss.nttdata.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <018e2a33e42be809d479db10c20fcdaf@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/018e2a33e42be809d479db10c20fcdaf%40oss.nttdata.com>
Precedence: bulk

On 2024-02-20 17:22, torikoshia wrote:
> On 2024-02-17 15:00, Bharath Rupireddy wrote:
>> On Fri, Feb 16, 2024 at 8:17 PM torikoshia 
>> <torikoshia@oss.nttdata.com> wrote:
>>> 
>>> I may be wrong since I seldom do data loading tasks, but I greed with
>>> you.
>>> 
>>> I also a little concerned about the case where there are many 
>>> malformed
>>> data and it causes lots of messages, but the information is usually
>>> valuable and if users don't need it, they can suppress it by changing
>>> client_min_messages.
>>> 
>>> Currently both summary of failures and individual information is 
>>> logged
>>> in NOTICE level.
>>> If we should assume that there are cases where only summary 
>>> information
>>> is required, it'd be useful to set lower log level, i.e. LOG to the
>>> individual information.
>> 
>> How about we emit the summary at INFO level and individual information
>> at NOTICE level? With this, the summary is given a different priority
>> than the individual info. With SET client_min_messages = WARNING; one
>> can still get the summary but not the individual info. Also, to get
>> all of these into server log, one can SET log_min_messages = INFO; or
>> SET log_min_messages = NOTICE;.
>> 
>> Thoughts?
> 
> It looks good to me.

Here are comments on the v2 patch.

+               if (cstate->opts.on_error != COPY_ON_ERROR_STOP)
+               {
+                   ereport(NOTICE,

I think cstate->opts.on_error is not COPY_ON_ERROR_STOP here, since if 
it is COPY_ON_ERROR_STOP, InputFunctionCallSafe() should already have 
errored out.

Should it be something like "Assert(cstate->opts.on_error != 
COPY_ON_ERROR_STOP)"?


Should below manual also be updated?

> A NOTICE message containing the ignored row count is emitted at the end 
> of the COPY FROM if at least one row was discarded.

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation