MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Fri, 24 Mar 2023 18:27:08 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Andres Freund <andres@anarazel.de>
Cc: tgl@sss.pgh.pa.us, Daniel Gustafsson <daniel@yesql.se>, Damir Belyalov
 <dam.bel07@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Danil Anisimow <anisimow.d@gmail.com>, Nikita Malakhov <HukuToc@gmail.com>,
 a.lepikhov@postgrespro.ru
Subject: Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)
In-Reply-To: <20230322175000.qbdctk7bnmifh5an@awork3.anarazel.de>
References: <483E1B59-4A80-450B-8685-7CC12762FD5A@anarazel.de>
 <2592557.1675663234@sss.pgh.pa.us>
 <39143e83571d59dfc04c75707fa0ea5a@oss.nttdata.com>
 <CALH1LgvQ5eRMi3zO7RSmod4ucru=9ekASpReahR7zDcCabANZw@mail.gmail.com>
 <0DE0602F-CC12-40ED-B259-3AB91FB02C3B@yesql.se>
 <8ad8492ff9fae3481d87c7aab4e0aed0@oss.nttdata.com>
 <CALH1LguAEsoTYJTCsXNB-7z2Hu9UGEpsXA4kj0FOTmoP=6Wp3Q@mail.gmail.com>
 <F2BEB24D-A6AB-4BB1-AC75-9A060E5338E0@yesql.se>
 <9a94e684d6528dff74de5959e97032f3@oss.nttdata.com>
 <8e5c596e47435e3b37b7a751ebcd9569@oss.nttdata.com>
 <20230322175000.qbdctk7bnmifh5an@awork3.anarazel.de>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <16e09747fcfbb21c30e4c1009c416aa4@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/16e09747fcfbb21c30e4c1009c416aa4%40oss.nttdata.com>
Precedence: bulk

On 2023-03-23 02:50, Andres Freund wrote:
> Hi,
> 
> Tom, see below - I wonder if should provide one more piece of 
> infrastructure
> around the saved error stuff...
> 
> 
> Have you measured whether this has negative performance effects when 
> *NOT*
> using the new option?
> 
> 
> As-is this does not work with FORMAT BINARY - and converting the binary 
> input
> functions to support soft errors won't happen for 16. So I think you 
> need to
> raise an error if BINARY and IGNORE_DATATYPE_ERRORS are specified.
> 
> 
> On 2023-03-22 22:34:20 +0900, torikoshia wrote:
>> @@ -985,9 +986,28 @@ CopyFrom(CopyFromState cstate)
>> 
>>  		ExecClearTuple(myslot);
>> 
>> +		if (cstate->opts.ignore_datatype_errors)
>> +		{
>> +			escontext.details_wanted = true;
>> +			cstate->escontext = escontext;
>> +		}
> 
> I think it might be worth pulling this out of the loop. That does mean 
> you'd
> have to reset escontext.error_occurred after an error, but that doesn't 
> seem
> too bad, you need to do other cleanup anyway.
> 
> 
>> @@ -956,10 +957,20 @@ NextCopyFrom(CopyFromState cstate, ExprContext 
>> *econtext,
>>  				values[m] = ExecEvalExpr(defexprs[m], econtext, &nulls[m]);
>>  			}
>>  			else
>> -				values[m] = InputFunctionCall(&in_functions[m],
>> -											  string,
>> -											  typioparams[m],
>> -											  att->atttypmod);
>> +				/* If IGNORE_DATATYPE_ERRORS is enabled skip rows with datatype 
>> errors */
>> +				if (!InputFunctionCallSafe(&in_functions[m],
>> +										   string,
>> +										   typioparams[m],
>> +										   att->atttypmod,
>> +										   (Node *) &cstate->escontext,
>> +										   &values[m]))
>> +				{
>> +					cstate->ignored_errors++;
>> +
>> +					ereport(WARNING,
>> +							errmsg("%s", cstate->escontext.error_data->message));
> 
> That isn't right - you loose all the details of the message. As is 
> you'd also
> leak the error context.
> 
> I think the best bet for now is to do something like
>     /* adjust elevel so we don't jump out */
>     cstate->escontext.error_data->elevel = WARNING;
>     /* despite the name, this won't raise an error if elevel < ERROR */
>     ThrowErrorData(cstate->escontext.error_data);

Thanks for your reviewing!
I'll try to fix it this way for the time being.

> I wonder if we ought to provide a wrapper for this? It could e.g. know 
> to
> mention the original elevel and such?

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION