Horton

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

Masahiko Sawada <sawada.mshk@gmail.com>

View thread
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Alexander Korotkov <aekorotkov@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-15T06:43:46Z
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

On Mon, Jan 15, 2024 at 8:21 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> 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.

Thank you for updating the patch. I have one minor point:

+       if (cstate->opts.save_error_to != COPY_SAVE_ERROR_TO_UNSPECIFIED &&
+               cstate->num_errors > 0)
+               ereport(NOTICE,
+                               errmsg("%zd rows were skipped due to
data type incompatibility",
+                                          cstate->num_errors));
+

We can use errmsg_plural() instead.

I have a question about the option values; do you think we need to
have another value of SAVE_ERROR_TO option to explicitly specify the
current default behavior, i.e. not accept any error? With the v4
patch, the user needs to omit SAVE_ERROR_TO option to accept errors
during COPY FROM. If we change the default behavior in the future,
many users will be affected and probably end up changing their
applications to keep the current default behavior.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com