MIME-Version: 1.0
References: <CACJufxH_OJpVra=0c4ow8fbxHj7heMcVaTNEPa5vAurSeNA-6Q@mail.gmail.com>
In-Reply-To: <CACJufxH_OJpVra=0c4ow8fbxHj7heMcVaTNEPa5vAurSeNA-6Q@mail.gmail.com>
From: Nishant Sharma <nishant.sharma@enterprisedb.com>
Date: Mon, 15 Jul 2024 11:12:40 +0530
Message-ID: <CADrsxdYb3HZKBnsAUV=H2F_54MUPoqW-VTBbOqFTiNrb+LDN7g@mail.gmail.com>
Subject: Re: on_error table, saving error info to a table
To: jian he <jian.universality@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="00000000000027d0bb061d42b344"
Archived-At: <https://www.postgresql.org/message-id/CADrsxdYb3HZKBnsAUV%3DH2F_54MUPoqW-VTBbOqFTiNrb%2BLDN7g%40mail.gmail.com>
Precedence: bulk

--00000000000027d0bb061d42b344
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Thanks for the patch!

I was not able to understand why we need a special error table for COPY?
Can't we just log it in a new log error file for COPY in a proper format?
Like
using table approach, PG would be unnecessarily be utilising its resources
like
extra CPU to format the data in pages to store in its table format, writing
and
keeping the table in its store (which may or may not be required), the user
would be required to make sure it creates the error table with proper
columns
to be used in COPY, etc..


Meanwhile, please find some quick review comments:-

1) Patch needs re-base.

2) If the columns of the error table are fixed, then why not create it
internally using
some function or something instead of making the user create the table
correctly
with all the columns?

3) I think, error messages can be improved, which looks big to me.

4) I think no need of below pstrdup, As CStringGetTextDatum creates a text
copy for
the same:-
err_code =3D
pstrdup(unpack_sql_state(cstate->escontext->error_data->sqlerrcode));

t_values[9] =3D CStringGetTextDatum(err_code);

5) Should 'on_error_rel' as not NULL be checked along with below, as I can
see it is
passed as NULL from two locations?
+               if (cstate->opts.on_error =3D=3D COPY_ON_ERROR_TABLE)
+               {

6) Below declarations can be shifted to the if block, where they are used.
Instead of
keeping them as global in function?
+   HeapTuple   on_error_tup;
+   TupleDesc   on_error_tupDesc;

+               if (cstate->opts.on_error =3D=3D COPY_ON_ERROR_TABLE)
+               {

7) Below comment going beyond 80 char width:-
* if on_error is specified with 'table', then on_error_rel is the error
saving table

8) Need space after 'false'
err_detail ? false: true;

9) Below call can fit in a single line. No need to keep the 2nd and 3rd
parameter in
nextlines.
+                   on_error_tup =3D heap_form_tuple(on_error_tupDesc,
+                                                   t_values,
+                                                   t_isnull);

10) Variable declarations Tab Spacing issue at multiple places.

11) Comments in the patch are not matched to PG comment style.


Kindly note I have not tested the patch properly yet. Only checked it with
a positive test
case. As I will wait for a conclusion on my opinion of the proposed patch.


Regards,
Nishant Sharma.
EnterpriseDB, Pune.


On Sat, Feb 3, 2024 at 11:52=E2=80=AFAM jian he <jian.universality@gmail.co=
m> wrote:

> Hi.
> I previously did some work in COPY FROM save error information to a table=
.
> still based on this suggestion:
> https://www.postgresql.org/message-id/752672.1699474336%40sss.pgh.pa.us
> Now I refactored it.
>
> the syntax:
> ON_ERROR 'table', TABLE 'error_saving_tbl'
>
> if ON_ERROR is not specified with 'table', TABLE is specified, then error=
.
> if ON_ERROR is specified with 'table', TABLE is not specified or
> error_saving_tbl does not exist, then error.
>
> In BeginCopyFrom, we check the data definition of error_saving_table,
> we also check if the user has INSERT privilege to error_saving_table
> (all the columns).
> We also did a preliminary check of the lock condition of
> error_saving_table.
>
> if it does not meet these conditions, we quickly error out.
> error_saving_table will be the same schema as the copy from table.
>
> Because "table" is a keyword, I have to add the following changes to
> gram.y.
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -3420,6 +3420,10 @@ copy_opt_item:
>   {
>   $$ =3D makeDefElem("null", (Node *) makeString($3), @1);
>   }
> + | TABLE opt_as Sconst
> + {
> + $$ =3D makeDefElem("table", (Node *) makeString($3), @1);
> + }
>
> since "table" is already a keyword, so there is no influence on the
> parsing speed?
>
> demo:
>
> create table err_tbl(
>     userid oid, -- the user oid while copy generated this entry
>     copy_tbl oid, --copy table
>     filename text,
>     lineno  int8,
>     line    text,
>     colname text,
>     raw_field_value text,
>     err_message text,
>     err_detail text,
>     errorcode text
> );
> create table t_copy_tbl(a int, b int, c int);
> COPY t_copy_tbl FROM STDIN WITH (delimiter ',', on_error 'table',
> table err_tbl);
> 1,2,a
> \.
>
> table err_tbl \gx
> -[ RECORD 1 ]---+-------------------------------------------
> userid          | 10
> copy_tbl        | 17920
> filename        | STDIN
> lineno          | 1
> line            | 1,2,a
> colname         | c
> raw_field_value | a
> err_message     | invalid input syntax for type integer: "a"
> err_detail      |
> errorcode       | 22P02
>

--00000000000027d0bb061d42b344
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Thanks for the patch!<div><br></div><div>I was not able to=
 understand why we need a special error table for COPY?</div><div>Can&#39;t=
 we just log it in a new log error file for COPY in a proper format? Like</=
div><div>using table approach, PG would be unnecessarily be utilising its r=
esources like</div><div>extra CPU to format the data in pages to store in i=
ts table format, writing and</div><div>keeping the table in its store (whic=
h may or may not be required), the user</div><div>would be required to make=
 sure it creates the error table with proper columns</div><div>to be used i=
n COPY, etc..<br></div><div><br></div><div><br></div><div>Meanwhile, please=
 find some quick review comments:-<br><br>1) Patch needs re-base.<br><br>2)=
 If the columns of the error table are fixed, then why not create it intern=
ally using</div><div>some function or something instead of making the user =
create the table correctly</div><div>with all the columns?<br><br>3) I thin=
k, error messages can be improved, which looks big to me.<br><br>4) I think=
 no need of below pstrdup, As CStringGetTextDatum creates a text copy for</=
div><div>the same:-<br>err_code =3D pstrdup(unpack_sql_state(cstate-&gt;esc=
ontext-&gt;error_data-&gt;sqlerrcode));<br><br>t_values[9] =3D CStringGetTe=
xtDatum(err_code);<br><br>5) Should &#39;on_error_rel&#39; as not NULL be c=
hecked along with below, as I can see it is</div><div>passed as NULL from t=
wo locations?<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (cst=
ate-&gt;opts.on_error =3D=3D COPY_ON_ERROR_TABLE)<br>+ =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {<br><br>6) Below declarations can be shifted =
to the if block, where they are used. Instead of</div><div>keeping them as =
global in function?<br>+ =C2=A0 HeapTuple =C2=A0 on_error_tup;<br>+ =C2=A0 =
TupleDesc =C2=A0 on_error_tupDesc;<br><br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 if (cstate-&gt;opts.on_error =3D=3D COPY_ON_ERROR_TABLE)<=
br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {<br><br>7) Below com=
ment going beyond 80 char width:-<br>* if on_error is specified with &#39;t=
able&#39;, then on_error_rel is the error saving table<br><br>8) Need space=
 after &#39;false&#39;<br>err_detail ? false: true;<br><br>9) Below call ca=
n fit in a single line. No need to keep the 2nd and 3rd parameter in</div><=
div>nextlines.<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 on_error_tup =3D heap_form_tuple(on_error_tupDesc,<br>+ =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 t_values,<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 t_isnull);<br><=
br>10) Variable declarations Tab Spacing issue at multiple places.<br><br>1=
1) Comments in the patch are not matched to PG comment style.</div><div><br=
><br>Kindly note I have not tested the patch properly yet. Only checked it =
with a positive test</div><div>case. As I will wait for a conclusion on my =
opinion of the proposed patch.</div><div><br></div><div><br></div><div>Rega=
rds,</div><div>Nishant Sharma.</div><div>EnterpriseDB, Pune.</div><div><br>=
</div></div><br><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_=
attr">On Sat, Feb 3, 2024 at 11:52=E2=80=AFAM jian he &lt;<a href=3D"mailto=
:jian.universality@gmail.com">jian.universality@gmail.com</a>&gt; wrote:<br=
></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;=
border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi.<br>
I previously did some work in COPY FROM save error information to a table.<=
br>
still based on this suggestion:<br>
<a href=3D"https://www.postgresql.org/message-id/752672.1699474336%40sss.pg=
h.pa.us" rel=3D"noreferrer" target=3D"_blank">https://www.postgresql.org/me=
ssage-id/752672.1699474336%40sss.pgh.pa.us</a><br>
Now I refactored it.<br>
<br>
the syntax:<br>
ON_ERROR &#39;table&#39;, TABLE &#39;error_saving_tbl&#39;<br>
<br>
if ON_ERROR is not specified with &#39;table&#39;, TABLE is specified, then=
 error.<br>
if ON_ERROR is specified with &#39;table&#39;, TABLE is not specified or<br=
>
error_saving_tbl does not exist, then error.<br>
<br>
In BeginCopyFrom, we check the data definition of error_saving_table,<br>
we also check if the user has INSERT privilege to error_saving_table<br>
(all the columns).<br>
We also did a preliminary check of the lock condition of error_saving_table=
.<br>
<br>
if it does not meet these conditions, we quickly error out.<br>
error_saving_table will be the same schema as the copy from table.<br>
<br>
Because &quot;table&quot; is a keyword, I have to add the following changes=
 to gram.y.<br>
--- a/src/backend/parser/gram.y<br>
+++ b/src/backend/parser/gram.y<br>
@@ -3420,6 +3420,10 @@ copy_opt_item:<br>
=C2=A0 {<br>
=C2=A0 $$ =3D makeDefElem(&quot;null&quot;, (Node *) makeString($3), @1);<b=
r>
=C2=A0 }<br>
+ | TABLE opt_as Sconst<br>
+ {<br>
+ $$ =3D makeDefElem(&quot;table&quot;, (Node *) makeString($3), @1);<br>
+ }<br>
<br>
since &quot;table&quot; is already a keyword, so there is no influence on t=
he<br>
parsing speed?<br>
<br>
demo:<br>
<br>
create table err_tbl(<br>
=C2=A0 =C2=A0 userid oid, -- the user oid while copy generated this entry<b=
r>
=C2=A0 =C2=A0 copy_tbl oid, --copy table<br>
=C2=A0 =C2=A0 filename text,<br>
=C2=A0 =C2=A0 lineno=C2=A0 int8,<br>
=C2=A0 =C2=A0 line=C2=A0 =C2=A0 text,<br>
=C2=A0 =C2=A0 colname text,<br>
=C2=A0 =C2=A0 raw_field_value text,<br>
=C2=A0 =C2=A0 err_message text,<br>
=C2=A0 =C2=A0 err_detail text,<br>
=C2=A0 =C2=A0 errorcode text<br>
);<br>
create table t_copy_tbl(a int, b int, c int);<br>
COPY t_copy_tbl FROM STDIN WITH (delimiter &#39;,&#39;, on_error &#39;table=
&#39;,<br>
table err_tbl);<br>
1,2,a<br>
\.<br>
<br>
table err_tbl \gx<br>
-[ RECORD 1 ]---+-------------------------------------------<br>
userid=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 10<br>
copy_tbl=C2=A0 =C2=A0 =C2=A0 =C2=A0 | 17920<br>
filename=C2=A0 =C2=A0 =C2=A0 =C2=A0 | STDIN<br>
lineno=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 1<br>
line=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 1,2,a<br>
colname=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| c<br>
raw_field_value | a<br>
err_message=C2=A0 =C2=A0 =C2=A0| invalid input syntax for type integer: &qu=
ot;a&quot;<br>
err_detail=C2=A0 =C2=A0 =C2=A0 |<br>
errorcode=C2=A0 =C2=A0 =C2=A0 =C2=A0| 22P02<br>
</blockquote></div>

--00000000000027d0bb061d42b344--