public inbox for [email protected]  

help / color / mirror / Atom feed

From: Nishant Sharma <[email protected]>
To: jian he <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Kirill Reshke <[email protected]>
Subject: Re: on_error table, saving error info to a table
Date: Mon, 16 Dec 2024 17:20:24 +0530
Message-ID: <CADrsxdbDqF7XUMSVfJ5z6_BpJwQPOb+4a2XuGf3y_zCEOHm1YA@mail.gmail.com> (raw)
In-Reply-To: <CACJufxEYJ+NvATCk0Q96vztm65jg6epZjOeUV4mS8BkEn=dTwg@mail.gmail.com>
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>
	<CALdSSPgG5kyO-ZHj97BJ2trCde9=pFG9R7dXGZQxJd2Q6_W4qw@mail.gmail.com>
	<CADrsxda5V4PO++zJOcF3PPzdZP_vHqHFnmXAd=ptPFbrtvOeGA@mail.gmail.com>
	<CACJufxEYJ+NvATCk0Q96vztm65jg6epZjOeUV4mS8BkEn=dTwg@mail.gmail.com>

On Fri, Dec 13, 2024 at 1:57 PM jian he <[email protected]> wrote:

> On Wed, Dec 11, 2024 at 7:41 PM Nishant Sharma
> <[email protected]> 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 != 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)
>
> 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 table
> Oid,
> I think it somehow counts as repeating name lookups, see relevant
> linke [1], [2].
>
> [1] https://postgr.es/m/[email protected]
> [2]
> https://postgr.es/m/CA+TgmobHYix=Nn8D4RUHa6fhUVPR88KGAMq1pBfnGfOfEjRixA@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 = systable_getnext(ascan))) -->
while (HeapTupleIsValid(atup = systable_getnext(ascan)) && on_error_tbl_ok)

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.

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: on_error table, saving error info to a table
  In-Reply-To: <CADrsxdbDqF7XUMSVfJ5z6_BpJwQPOb+4a2XuGf3y_zCEOHm1YA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox