Horton

Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: jian he <jian.universality@gmail.com>, torikoshia <torikoshia@oss.nttdata.com>, vignesh C <vignesh21@gmail.com>, Alena Rybakina <lena.ribackina@yandex.ru>, Damir Belyalov <dam.bel07@gmail.com>, zhihuifan1213@163.com, Tom Lane <tgl@sss.pgh.pa.us>, Daniel Gustafsson <daniel@yesql.se>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Andres Freund <andres@anarazel.de>, anisimow.d@gmail.com, HukuToc@gmail.com, pgsql-hackers@lists.postgresql.org, Andrei Lepikhov <a.lepikhov@postgrespro.ru>
Date: 2024-01-14T23:21:07Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. doc: Fix COPY ON_ERROR option syntax synopsis.

  2. Disallow specifying ON_ERROR option without value.

  3. Rename COPY option from SAVE_ERROR_TO to ON_ERROR

  4. Fix spelling in notice

  5. Add new COPY option SAVE_ERROR_TO

Attachments

On Sun, Jan 14, 2024 at 10:35 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Thank you for updating the patch. Here are two comments:
>
> ---
> +   if (cstate->opts.save_error_to != COPY_SAVE_ERROR_TO_UNSPECIFIED &&
> +       cstate->num_errors > 0)
> +       ereport(WARNING,
> +               errmsg("%zd rows were skipped due to data type incompatibility",
> +                      cstate->num_errors));
> +
>     /* Done, clean up */
>     error_context_stack = errcallback.previous;
>
> If a malformed input is not the last data, the context message seems odd:
>
> postgres(1:1769258)=# create table test (a int);
> CREATE TABLE
> postgres(1:1769258)=# copy test from stdin (save_error_to none);
> Enter data to be copied followed by a newline.
> End with a backslash and a period on a line by itself, or an EOF signal.
> >> a
> >> 1
> >>
> 2024-01-15 05:05:53.980 JST [1769258] WARNING:  1 rows were skipped
> due to data type incompatibility
> 2024-01-15 05:05:53.980 JST [1769258] CONTEXT:  COPY test, line 3: ""
> COPY 1
>
> I think it's better to report the WARNING after resetting the
> error_context_stack. Or is a WARNING really appropriate here? The
> v15-0001-Make-COPY-FROM-more-error-tolerant.patch[1] uses NOTICE but
> the v1-0001-Add-new-COPY-option-SAVE_ERROR_TO.patch[2] changes it to
> WARNING without explanation.

Thank you for noticing this.  I think NOTICE is more appropriate here.
There is nothing to "worry" about: the user asked to ignore the errors
and we did.  And yes, it doesn't make sense to use the last line as
the context.  Fixed.

> ---
> +-- test missing data: should fail
> +COPY check_ign_err FROM STDIN WITH (save_error_to none);
> +1  {1}
> +\.
>
> We might want to cover the extra data cases too.

Agreed, the relevant test is added.

------
Regards,
Alexander Korotkov