MIME-Version: 1.0
References: <CACJufxH_OJpVra=0c4ow8fbxHj7heMcVaTNEPa5vAurSeNA-6Q@mail.gmail.com>
 <CADrsxdYb3HZKBnsAUV=H2F_54MUPoqW-VTBbOqFTiNrb+LDN7g@mail.gmail.com>
 <CACJufxGMOVr53-=oSMvogbX0BN5R73AD4sj0r1M3P6-x5XNwjg@mail.gmail.com>
 <CADrsxdYsxbgmWYBWtjd9h8WaUw9TWzjKsREX7=Br5Z12ozrRsg@mail.gmail.com> <CACJufxGcX2fKxQqjXJdnXLSVSyowrGiQ5i0960UbiKytu0gm5A@mail.gmail.com>
In-Reply-To: <CACJufxGcX2fKxQqjXJdnXLSVSyowrGiQ5i0960UbiKytu0gm5A@mail.gmail.com>
From: Kirill Reshke <reshkekirill@gmail.com>
Date: Tue, 3 Dec 2024 15:28:40 +0500
Message-ID: <CALdSSPgG5kyO-ZHj97BJ2trCde9=pFG9R7dXGZQxJd2Q6_W4qw@mail.gmail.com>
Subject: Re: on_error table, saving error info to a table
To: jian he <jian.universality@gmail.com>
Cc: Nishant Sharma <nishant.sharma@enterprisedb.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CALdSSPgG5kyO-ZHj97BJ2trCde9%3DpFG9R7dXGZQxJd2Q6_W4qw%40mail.gmail.com>
Precedence: bulk

On Tue, 3 Dec 2024 at 09:29, jian he <jian.universality@gmail.com> wrote:
>
> On Tue, Nov 5, 2024 at 6:30=E2=80=AFPM 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@c=
ybertec.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?


=3D=3D=3D=3D=3D 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?

--=20
Best regards,
Kirill Reshke