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 1tN9cc-008v7v-E2 for pgsql-hackers@arkaria.postgresql.org; Mon, 16 Dec 2024 11:50:46 +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 1tN9cZ-005xib-F8 for pgsql-hackers@arkaria.postgresql.org; Mon, 16 Dec 2024 11:50:44 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tN9cZ-005xiI-0R for pgsql-hackers@lists.postgresql.org; Mon, 16 Dec 2024 11:50:44 +0000 Received: from mail-vs1-xe30.google.com ([2607:f8b0:4864:20::e30]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1tN9cS-0036xF-Pk for pgsql-hackers@lists.postgresql.org; Mon, 16 Dec 2024 11:50:42 +0000 Received: by mail-vs1-xe30.google.com with SMTP id ada2fe7eead31-4affd0fb6adso1010431137.1 for ; Mon, 16 Dec 2024 03:50:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb.com; s=google; t=1734349836; x=1734954636; 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=cIk7OnwQlrPKBwAxgu7S8M3lVziFtm+6s1ZYW+PIAys=; b=j9coWrUxbqyoAx7p6bd2eQQg90PQlaiD+1xvQMOz59BA8WlGJ5bOpAfCZPP76IkiiZ 9aAVi8vO68qjiL4OCk9XzeqQ6zTCMNNC1BccG47KGPXtYiUELEotaVX2dCK8h28QoKPU muoLzpPwoMUag8dae9/Jv8n1PLbcE2bVuVuqLr4sK+2lyrHAslttF5LJru4RP0axij/Z IOmxj+UEtPYKFCwtqR4XtNUOlI27Di+eFNx2gOT/K6jhC8eKW2EcKapzAPwOJmejUkA7 KeHKknERIkUgc7dUlVnGPUnmdeyZlvzjOcDK1eUpRrmgAVVpPdg2ZEpTGBjHXKy+336e diUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734349836; x=1734954636; 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=cIk7OnwQlrPKBwAxgu7S8M3lVziFtm+6s1ZYW+PIAys=; b=J6kvjerWw3GbushZO3EGimKrxcVlXGb4YUOkIWQzzW/UHEO5Gsimo9OBAHqMIw9+yI kFyQQiGrafHMYSB5KuUt2ktt4tyBgasmFpltXnX2FdQhWZNGLyEDrEXLZzs733+fIoIW 4ER5Rnkb/6WCYk5Auetg5W+20Qi/i5ZRDAqpyMp7qr7F1+Rec4BipIRW6BvNGy4olLo9 Z1PyGvWzuv74XRJT2lTnNBt6UXWqOgbXyOZdJqlTH+Z69+uMEaL/dlY/vITX6seALFeg 7OpSFWbQ+MuEI1teL9e+MsiCjUZmKSpd7qyrnMrwmrp8/p8mJQBIcNRTHXOj8OXaAvH/ 8OSg== X-Gm-Message-State: AOJu0Yw2Id3CPZg3UEv7T+uHUrISCjikzl0BIOLUZqO1/yiG2ZqxdT1Q Ox4ljvgiS2sY8KT3eHBnkIxvyMktq24UrU7SpPakCLtpVRoHfYcKdpksHJ49uFCqw6TFjYvcr2G CZ1aISf0C9vbzAq71Pso5jmNWuWkl3JYkbvjW X-Gm-Gg: ASbGncunaoTsHEcGAryUQpWrJUmr6Sf9Mauq+/EFIo4Cy6shL0x/RJCRz6XnNWffU+y Ffjwkb6lMvLskZDp1rK5Dhlhw0BNBe6Z4kzjTYK+k4W5Hgwx6gkN1NSmnWPHnRDn9iUkrGh03 X-Google-Smtp-Source: AGHT+IEAiFWgKn1+0zz/zSoLhDL78zEykP24QqE3cHRTZ1BowvhnADGi23b2Vu18ufDh1WkuYLZeRKVggDIjR/xZk2k= X-Received: by 2002:a05:6102:510e:b0:4b1:3409:93dd with SMTP id ada2fe7eead31-4b25dae9aeamr10909318137.18.1734349835676; Mon, 16 Dec 2024 03:50:35 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Nishant Sharma Date: Mon, 16 Dec 2024 17:20:24 +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="000000000000df498f062961c944" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000df498f062961c944 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Dec 13, 2024 at 1:57=E2=80=AFPM jian he wrote: > On Wed, Dec 11, 2024 at 7:41=E2=80=AFPM Nishant Sharma > wrote: > > > > 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) > > hi. Thanks for the review. > The attached v4 patch addressed these two issues. > > > > 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. > > > I decided not to auto-create the table. > main reason not to do it: > 1. utility COPY command with another SPI utility CREATE TABLE command may > work. > but there is no precedent. > > 2. if we auto-create the on_error table with BeginCopyFrom. > then later we have to use get_relname_relid to get the newly created tabl= e > Oid, > I think it somehow counts as repeating name lookups, see relevant > linke [1], [2]. > > [1] https://postgr.es/m/20240808171351.a9.nmisch@google.com > [2] > https://postgr.es/m/CA+TgmobHYix=3DNn8D4RUHa6fhUVPR88KGAMq1pBfnGfOfEjRixA= @mail.gmail.com Thanks for the v4 patch! Review comment on v4:- 1) The new switch logic does not look correct to me. It will pass for a failing scenario. I think you can use v3's logic instead with below changes:- a) while (HeapTupleIsValid(atup =3D systable_getnext(ascan))) --> while (HeapTupleIsValid(atup =3D systable_getnext(ascan)) && on_error_tbl_o= k) b) attcnt++; --> just before the "switch (attForm->attnum)". Thats it. Also, I think Andrew's suggestion can resolve the concern me and Krill had on forcing users to create tables with correct column names and numbers. Also, will make error table checking simpler. No need for the above kind of checks. Regards, Nishant. --000000000000df498f062961c944 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Fri, Dec 13, 2024 at 1:57=E2=80=AFPM j= ian he <jian.universality= @gmail.com> wrote:
On Wed, Dec 11, 2024= at 7:41=E2=80=AFPM Nishant Sharma
<ni= shant.sharma@enterprisedb.com> wrote:
>
> 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.
>
> +=C2=A0 =C2=A0 if (cstate->num_errors > 0 &&
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 cstate->opts.log_verbosity >= =3D COPY_LOG_VERBOSITY_DEFAULT)

> 2) No need for the below "if" check for maxattnum. We can si= mply
> increment it with "++maxattnum" and later check if we have e= xactly
> 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<= br> > 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 =C2=A0if (maxattnum <=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 =C2=A0maxattnum =3D attForm->attnum;=
>
> 3) #define would be better, also as mentioned by Kirill switch
> condition with proper #define would be better.
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (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 =C2=A0on_error_tbl_ok =3D false;
>
> 4)

hi. Thanks for the review.
The attached v4 patch addressed these two issues.

> > 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.
>
I decided not to auto-create the table.
main reason not to do it:
1. utility COPY command with another SPI utility CREATE TABLE command may w= ork.
but there is no precedent.

2. if we auto-create the on_error table with BeginCopyFrom.
then later we have to use get_relname_relid to get the newly created table = Oid,
I think it somehow counts as repeating name lookups, see relevant
linke [1], [2].

[1] https://postgr.es/m/20240808171351.a9.nmi= sch@google.com
[2] https://= postgr.es/m/CA+TgmobHYix=3DNn8D4RUHa6fhUVPR88KGAMq1pBfnGfOfEjRixA@mail.gmai= l.com

Thanks for the v4 patch!

Review comment on v4:-

1) The new switch logic does= not look correct to me. It will pass for=C2=A0
a failing scenari= o. I think you can use v3's logic instead with below
changes:= -

a)
while (HeapTupleIsValid(atup =3D systable_getnext(ascan))) -= ->
while (HeapTupleIsValid(atup =3D systable_getnext(ascan)) &&am= p; on_error_tbl_ok)

b)
attcnt++; --> just before the "swi= tch (attForm->attnum)".

Thats it.

Also, I think Andrew's suggestion can resolve the conc= ern me and Krill
had on forcing users to create tables with corre= ct column names and
numbers. Also, will make error table checking= simpler. No need for the
above kind of checks.


Regards,
Nishant.
=C2=A0
--000000000000df498f062961c944--