Re: on_error table, saving error info to a table

On Wed, Dec 11, 2024 at 7:41 PM Nishant Sharma
<nishant.sharma@enterprisedb.com> wrote:
>
> Thanks for the v3 patch!
>
> Please find review comments on v3:-
>
> 1) I think no need to change the below if condition, we can keep
> it the way it was before i.e with
> "cstate->opts.on_error != COPY_ON_ERROR_STOP" and we
> add a new error ereport the way v3 has. Because for
> cstate->opts.on_error as COPY_ON_ERROR_STOP cases we
> can avoid two if conditions inside upper if.
>
> +    if (cstate->num_errors > 0 &&
>          cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT)

> 2) No need for the below "if" check for maxattnum. We can simply
> increment it with "++maxattnum" and later check if we have exactly
> 10 attributes for the error table. Because even if we drop any
> attribute and maxattnum is 10 in pg_attribute for that rel, we should
> still error out. Maybe we can rename it to "totalatts"?
>
> +                       if (maxattnum <= attForm->attnum)
> +                               maxattnum = attForm->attnum;
>
> 3) #define would be better, also as mentioned by Kirill switch
> condition with proper #define would be better.
>
> +               if (maxattnum != 10)
> +                       on_error_tbl_ok = false;
>
> 4)

hi. Thanks for the review.
The attached v4 patch addressed these two issues.

> > 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.
> >
> YES. but that would lead to a better design with an error table.
> Also, I think Krill mentions the same. That is to auto create, if it
> does not exist.
>
I decided not to auto-create the table.
main reason not to do it:
1. utility COPY command with another SPI utility CREATE TABLE command may work.
but there is no precedent.

2. if we auto-create the on_error table with BeginCopyFrom.
then later we have to use get_relname_relid to get the newly created table Oid,
I think it somehow counts as repeating name lookups, see relevant
linke [1], [2].

[1] https://postgr.es/m/20240808171351.a9.nmisch@google.com
[2] https://postgr.es/m/CA+TgmobHYix=Nn8D4RUHa6fhUVPR88KGAMq1pBfnGfOfEjRixA@mail.gmail.com