Feedback-ID: id4a34324:Fastmail
Date: Wed, 22 Mar 2023 10:50:00 -0700
From: Andres Freund <andres@anarazel.de>
To: torikoshia <torikoshia@oss.nttdata.com>, tgl@sss.pgh.pa.us
Cc: 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)
Message-ID: <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8e5c596e47435e3b37b7a751ebcd9569@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/20230322175000.qbdctk7bnmifh5an%40awork3.anarazel.de>
Precedence: bulk

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);

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


I don't think NextCopyFrom() is the right place to emit this warning - it
e.g. is also called from file_fdw.c, which might want to do something else
with the error. From a layering POV it seems cleaner to do this in
CopyFrom(). You already have a check for escontext.error_occurred there
anyway.


> @@ -3378,6 +3378,10 @@ copy_opt_item:
>  				{
>  					$$ = makeDefElem("freeze", (Node *) makeBoolean(true), @1);
>  				}
> +			| IGNORE_DATATYPE_ERRORS
> +				{
> +					$$ = makeDefElem("ignore_datatype_errors", (Node *)makeBoolean(true), @1);
> +				}
>  			| DELIMITER opt_as Sconst
>  				{
>  					$$ = makeDefElem("delimiter", (Node *) makeString($3), @1);


I think we shouldn't add a new keyword for this, but only support this via
/* new COPY option syntax */
copy_generic_opt_list:
			copy_generic_opt_elem

Further increasing the size of the grammar with random keywords when we have
more generic ways to represent them seems unnecessary.


> +-- tests for IGNORE_DATATYPE_ERRORS option
> +CREATE TABLE check_ign_err (n int, m int[], k int);
> +COPY check_ign_err FROM STDIN WITH IGNORE_DATATYPE_ERRORS;
> +1	{1}	1
> +a	{2}	2
> +3	{3}	3333333333
> +4	{a, 4}	4
> +
> +5	{5}	5
> +\.
> +SELECT * FROM check_ign_err;
> +

I suggest adding a few more tests:

- COPY with a datatype error that can't be handled as a soft error
- test documenting that COPY FORMAT BINARY is incompatible with IGNORE_DATATYPE_ERRORS
- a soft error showing the error context - although that will require some
  care to avoid the function name + line in the output

Greetings,

Andres Freund