Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tLL5m-00ECkY-3m for pgsql-hackers@arkaria.postgresql.org; Wed, 11 Dec 2024 11:41:22 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1tLL5j-00FgUQ-6U for pgsql-hackers@arkaria.postgresql.org; Wed, 11 Dec 2024 11:41:20 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tLL5i-00FgUI-Qz for pgsql-hackers@lists.postgresql.org; Wed, 11 Dec 2024 11:41:20 +0000 Received: from mail-vk1-xa36.google.com ([2607:f8b0:4864:20::a36]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1tLL5f-002FkH-Rs for pgsql-hackers@lists.postgresql.org; Wed, 11 Dec 2024 11:41:18 +0000 Received: by mail-vk1-xa36.google.com with SMTP id 71dfb90a1353d-51882c83065so1368229e0c.0 for ; Wed, 11 Dec 2024 03:41:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb.com; s=google; t=1733917274; x=1734522074; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=6Vn5icGfn5277lD/hNfQEruCB35O3O+cw6NkoRm3vzU=; b=MosDYpN2nyJe0drKplLdMDJChCjk6BZYd1yrG2Wnh0igrn0BZD571gjMpzjOandPmT vJe6Muf/oFDoFz2e3RIj3Y0qRwNR76LtnDCYwlwTpe9Y6WH5mqjr86pWYETwMZ5krt49 +H5qQLZ+zm2VVtt1EcrbcJHStVjULLZ6PJneAv51/Difqb8i5TJG1tM+NC+ipENHXOn0 EgY5X4nbjRPjssC0jpE8T6OzDAMXX+1IqXf4umDh4cABcfj77Meh5STGH66yGjdbubhS FH4Ngzwo31pcm3WUUzBOnU+Q7W6P6qURQ8mfNYzs0cAT99Vn6rmA0cHMGoAhHpMi3Xj2 Io7A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733917274; x=1734522074; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=6Vn5icGfn5277lD/hNfQEruCB35O3O+cw6NkoRm3vzU=; b=uThnTejyocPl/sTjF0BVXs67k34oPMWrtUCXTS9HnbYHsev4ypQRZGIFlMh/61uoYw c4vQ20VFLRU3KQ/J7z+rrVryJ2wEnP0uvAMTBh92TYyqbMficXWAOCKePMqnX2hjtXEt ZzPzSw/FpKgL4/s5nMiG/NYfaIM9RE8nqSn04yl8f95hsNvd3NvJ2kPaqYaVp1LHuJMR JgDI4Bfwm+zgFOLhCvy5jW5DJc4xBqkjjbHnj8SEvKEzmjD4wAlkEOnxM8M2ZLwbdhpz 6Mv4sMpH4NTm+sdPTZihwZxbcTyZyqrNDIBfu/96TaD0xeYDU9vv1MSSfjo71vOwcd2U +AUw== X-Gm-Message-State: AOJu0Yw6xtu5Ss/wIOsVPdH+I1sIRTbDSif14i21SPAO4WZ49ZlcwGjB FVViQRLDzVHRpyKo3aEa0akrhVJax2MFA+QkTFEX4SfOa3xbthU6gTvOItMnRf3N94rdcSpcq17 Q2XHHIrR/XiIHp7A7uzlLYkr8SdjK4LmEW/FZ X-Gm-Gg: ASbGnct9SC88rvrHfIKPcE5I1YNIrCvBXeThnO0YTqtiS26MCfqqJqyP1RlBRPY5QOn LGzU7FpWPSYeHXtZQbuVjn8Hp2jAiRKl0poqpugBDD5l1zUS9i10be6lrFIMFIrA01SrUNA== X-Google-Smtp-Source: AGHT+IHlEmyIxuaIG8+ICo6AhAfDFTMmCNjSiO6Q+Ma9j3NGK/nIVxv841XLqa0fg26jfpcLYwRNiPPn9Kcr5usLPX4= X-Received: by 2002:a05:6122:3285:b0:515:3bfb:d422 with SMTP id 71dfb90a1353d-518a3cf331amr2117121e0c.12.1733917273878; Wed, 11 Dec 2024 03:41:13 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Nishant Sharma Date: Wed, 11 Dec 2024 17:11:02 +0530 Message-ID: Subject: Re: on_error table, saving error info to a table To: jian he Cc: PostgreSQL Hackers , Kirill Reshke Content-Type: multipart/alternative; boundary="0000000000002e07e80628fd138c" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000002e07e80628fd138c Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 !=3D 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 >=3D 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 <=3D attForm->attnum) + maxattnum =3D attForm->attnum; 3) #define would be better, also as mentioned by Kirill switch condition with proper #define would be better. + if (maxattnum !=3D 10) + on_error_tbl_ok =3D 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=E2=80=AFPM Kirill Reshke wrote: > On Tue, 3 Dec 2024 at 09:29, jian he wrote: > > > > On Tue, Nov 5, 2024 at 6:30=E2=80=AFPM Nishant Sharma > > 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 t= o > > > 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__ `. Can we follow this style? > > 2) > >+ /* > > + * similar to commit a9cf48a > > + * ( > https://postgr.es/m/7bcfc39d4176faf85ab317d0c26786953646a411.camel@cybert= ec.at > ) > > + * in COPY FROM keep error saving table locks until the transaction en= d. > > + */ > > 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? > > -- > Best regards, > Kirill Reshke > --0000000000002e07e80628fd138c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks for the v3 patch!

Please f= ind review comments on v3:-

1) I think no need to change the below i= f condition, we can keep
it the way it was before i.e with
&q= uot;cstate->opts.on_error !=3D COPY_ON_ERROR_STOP" and we
add a new error ereport the way v3 has. Because for
cstate->o= pts.on_error as COPY_ON_ERROR_STOP cases we
can avoid two if cond= itions inside upper if.

+=C2=A0 =C2=A0 if (cstate->num_errors > 0 && =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cstate->opts.log_verbosity >=3D CO= PY_LOG_VERBOSITY_DEFAULT)

2) No need for the below "if" ch= eck for maxattnum. We can simply
increment it with "++maxatt= num" and later check if we have exactly
10 attributes for th= e error table. Because even if we drop any
attribute and maxattnu= m is 10 in pg_attribute for that rel, we should
still error out. = Maybe we can rename it to "totalatts"?

+ =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (maxattnum &= lt;=3D attForm->attnum)
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 maxattnum = =3D attForm->attnum;

3) #define would be better, also as mentione= d by Kirill switch
condition with proper #define would be better.=

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (maxattnum != =3D 10)
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 on_error_tbl_ok =3D false;

4)
>=C2=A0=C2= =A0
> that would be more work.
> so i stick to if there is a ta= ble can use to
> error saving then use it, otherwise error out.
=
>
YES. but that would lead to a better design with an error tabl= e.
Also, I think Krill mentions the same. That is to auto create,= if it
does not exist.


<= div>Regards,
Nishant Sharma (EDB).

On = Tue, Dec 3, 2024 at 3:58=E2=80=AFPM Kirill Reshke <reshkekirill@gmail.com> wrote:
On Tue, 3 Dec 2024 at 09:29, jia= n he <j= ian.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=C2=A0 =C2=A010
> >
> 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 n= ew
> > added code was at the end of the block:-
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0cstate->num_errors++;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0return true;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0csta= te->num_errors++;
> >
> changed per your advice.
>
> > I was not able to test the v2 due to conflicts in v2, I did attem= pt 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 sty= le?

2)
>+ /*
> + * similar to commit a9cf48a
> + * (https://post= gr.es/m/7bcfc39d4176faf85ab317d0c26786953646a411.camel@cybertec.at)
> + * in COPY FROM keep error saving table locks until the transaction e= nd.
> + */

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?

--
Best regards,
Kirill Reshke
--0000000000002e07e80628fd138c--