Thread: on_error table, saving error info to a table

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:
  {
  $$ = makeDefElem("null", (Node *) makeString($3), @1);
  }
+ | TABLE opt_as Sconst
+ {
+ $$ = 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

On Tue, 3 Dec 2024 at 09:29, jian he <jian.universality@gmail.com> wrote:
>
> On Tue, Nov 5, 2024 at 6:30 PM Nishant Sharma
> <nishant.sharma@enterprisedb.com> wrote:
> >
> > Thanks for the v2 patch!
> >
> > I see v1 review comments got addressed in v2 along with some
> > further improvements.
> >
> > 1) v2 Patch again needs re-base.
> >
> > 2) I think we need not worry whether table name is unique or not,
> > table name can be provided by user and we can check if it does
> > not exists then simply we can create it with appropriate columns,
> > if it exists we use it to check if its correct on_error table and
> > proceed.
>
> "simply we can create it with appropriate columns,"
> that would be more work.
> so i stick to if there is a table can use to
> error saving then use it, otherwise error out.
>
>
> >
> > 3) Using #define in between the code? I don't see that style in
> > copyfromparse.c file. I do see such style in other src file. So, not
> > sure if committer would allow it or not.
> > #define ERROR_TBL_COLUMNS   10
> >
> let's wait and see.
>
> > 4) Below appears redundant to me, it was not the case in v1 patch
> > set, where it had only one return and one increment of error as new
> > added code was at the end of the block:-
> > +                   cstate->num_errors++;
> > +                   return true;
> > +               }
> >                 cstate->num_errors++;
> >
> changed per your advice.
>
> > I was not able to test the v2 due to conflicts in v2, I did attempt to
> > resolve but I saw many failures in make world.
> >
> I get rid of all the SPI code.
>
> Instead, now I iterate through AttributeRelationId to check if the
> error saving table is ok or not,
> using DirectFunctionCall3 to do the privilege check.
> removed gram.y change, turns out it is not necessary.
> and other kinds of refactoring.
>
> please check attached.


Hi!

1)
> + switch (attForm->attnum)
> + {
> + case 1:
> + (.....)
> + case 2:

case 1,2,3 ... Is too random. Other parts of core tend to use `#define
Anum_<relname>_<columname> <num>`. Can we follow this style?

2)
>+ /*
> + * similar to commit a9cf48a
> + * (https://postgr.es/m/7bcfc39d4176faf85ab317d0c26786953646a411.camel@cybertec.at)
> + * in COPY FROM keep error saving table locks until the transaction end.
> + */

I can rarely see other comments referencing commits, and even few
referencing a mail archive thread.
Can we just write proper comment explaining the reasons?


===== overall

Patch design is a little dubious for me. We give users some really
incomprehensible API. To use on_error *relation* feature user must
create tables with proper schema.
Maybe a better design will be to auto-create on_error table if this
table does not exist.


Thoughts?

--
Best regards,
Kirill Reshke

On Tue, Nov 5, 2024 at 6:30 PM Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:

Thanks for the v2 patch!

I see v1 review comments got addressed in v2 along with some
further improvements.

1) v2 Patch again needs re-base.

2) I think we need not worry whether table name is unique or not,
table name can be provided by user and we can check if it does
not exists then simply we can create it with appropriate columns,
if it exists we use it to check if its correct on_error table and
proceed.

"simply we can create it with appropriate columns,"
that would be more work.
so i stick to if there is a table can use to
error saving then use it, otherwise error out.

3) Using #define in between the code? I don't see that style in
copyfromparse.c file. I do see such style in other src file. So, not
sure if committer would allow it or not.
#define ERROR_TBL_COLUMNS   10

let's wait and see.

4) Below appears redundant to me, it was not the case in v1 patch
set, where it had only one return and one increment of error as new
added code was at the end of the block:-
+                   cstate->num_errors++;
+                   return true;
+               }                cstate->num_errors++;

changed per your advice.

I was not able to test the v2 due to conflicts in v2, I did attempt to
resolve but I saw many failures in make world.

I get rid of all the SPI code.

Instead, now I iterate through AttributeRelationId to check if the
error saving table is ok or not,
using DirectFunctionCall3 to do the privilege check.
removed gram.y change, turns out it is not necessary.
and other kinds of refactoring.

please check attached.

I don't have a comment on the patch in general, but wouldn't it be better to use a typed table? Then all you would need to check is the reloftype to make sure it's the right type, instead of checking all the column names and types. Creating the table would be simpler, too. Just

CREATE TABLE my_copy_errors OF TYPE pg_copy_error;

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, 16 Dec 2024 at 16:50, Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:
> Also, I think Andrew's suggestion can resolve the concern me and Krill
> had on forcing users to create tables with correct column names and
> numbers. Also, will make error table checking simpler. No need for the
> above kind of checks.

+1 on that.

-- 
Best regards,
Kirill Reshke