Re: on_error table, saving error info to a table

On Mon, Jul 15, 2024 at 1:42 PM Nishant Sharma
<nishant(dot)sharma(at)enterprisedb(dot)com> wrote:
>
> 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?

I'll think about it more.
previously, i tried to use SPI to create tables, but at that time, i
thought that's kind of excessive.
you need to create the table, check whether the table name is unique,
check the privilege.
now we quickly error out if the error saving table definition does not
meet. I guess that's less work to do.

> 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 = pstrdup(unpack_sql_state(cstate->escontext->error_data->sqlerrcode));
>
> t_values[9] = 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 == 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 == 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 = 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.
>
almost all these issues have been addressed.
The error messages are still not so good. I need to polish it.