Re: on_error table, saving error info to a table

On Fri, Dec 13, 2024 at 1:57 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> On Wed, Dec 11, 2024 at 7:41 PM Nishant Sharma
> <nishant(dot)sharma(at)enterprisedb(dot)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

Thanks for the v4 patch!

Review comment on v4:-

1) The new switch logic does not look correct to me. It will pass for
a failing scenario. I think you can use v3's logic instead with below
changes:-

a)
while (HeapTupleIsValid(atup = systable_getnext(ascan))) -->
while (HeapTupleIsValid(atup = systable_getnext(ascan)) && on_error_tbl_ok)

b)
attcnt++; --> just before the "switch (attForm->attnum)".

Thats it.

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.

Regards,
Nishant.