Re: on_error table, saving error info to a table

On Tue, 3 Dec 2024 at 09:29, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Tue, Nov 5, 2024 at 6:30 PM Nishant Sharma
> <nishant(dot)sharma(at)enterprisedb(dot)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