Re: on_error table, saving error info to a table - Mailing list pgsql-hackers
| From | Nishant Sharma |
|---|---|
| Subject | Re: on_error table, saving error info to a table |
| Date | |
| Msg-id | CADrsxda5V4PO++zJOcF3PPzdZP_vHqHFnmXAd=ptPFbrtvOeGA@mail.gmail.com Whole thread Raw |
| In response to | Re: on_error table, saving error info to a table (Kirill Reshke <reshkekirill@gmail.com>) |
| List | pgsql-hackers |
Thanks for the v3 patch!
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
+ 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
+ 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)
+ if (maxattnum != 10)
+ on_error_tbl_ok = false;
4)
>
> 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.
> 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.
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.
Regards,
Nishant Sharma (EDB).
On Tue, Dec 3, 2024 at 3:58 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
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
pgsql-hackers by date: