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)
>
> 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.
Regards,
Nishant Sharma (EDB).
On Tue, Dec 3, 2024 at 3:58 PM Kirill Reshke <reshkekir...@gmail.com> wrote:
> On Tue, 3 Dec 2024 at 09:29, jian he <jian.universal...@gmail.com> wrote:
> >
> > On Tue, Nov 5, 2024 at 6:30 PM Nishant Sharma
> > <nishant.sha...@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.ca...@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
>
