On Tue, Jan 9, 2024 at 10:36 PM torikoshia <torikos...@oss.nttdata.com> wrote: > > On Tue, Dec 19, 2023 at 10:14 AM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > If we want only such a feature we need to implement it together (the > > patch could be split, though). But if some parts of the feature are > > useful for users as well, I'd recommend implementing it incrementally. > > That way, the patches can get small and it would be easy for reviewers > > and committers to review/commit them. > > Jian, how do you think this comment? > > Looking back at the discussion so far, it seems that not everyone thinks > saving table information is the best idea[1] and some people think just > skipping error data is useful.[2] > > Since there are issues to be considered from the design such as > physical/logical replication treatment, putting error information to > table is likely to take time for consensus building and development. > > Wouldn't it be better to follow the following advice and develop the > functionality incrementally? > > On Fri, Dec 15, 2023 at 4:49 AM Masahiko Sawada > <sawada(dot)mshk(at)gmail(dot)com> wrote: > > So I'm thinking we may be able to implement this > > feature incrementally. The first step would be something like an > > option to ignore all errors or an option to specify the maximum number > > of errors to tolerate before raising an ERROR. The second step would > > be to support logging destinations such as server logs and tables. > > > Attached a patch for this "first step" with reference to v7 patch, which > logged errors and simpler than latest one. > - This patch adds new option SAVE_ERROR_TO, but currently only supports > 'none', which means just skips error data. It is expected to support > 'log' and 'table'. > - This patch Skips just soft errors and don't handle other errors such > as missing column data.
Hi.
I made the following change based on your patch
(v1-0001-Add-new-COPY-option-SAVE_ERROR_TO.patch)
* when specified SAVE_ERROR_TO, move the initialization of
ErrorSaveContext to the function BeginCopyFrom.
I think that's the right place to initialize struct CopyFromState field.
* I think your patch when there are N rows have malformed data, then it
will initialize N ErrorSaveContext.
In the struct CopyFromStateData, I changed it to ErrorSaveContext *escontext.
So if an error occurred, you can just set the escontext accordingly.
* doc: mention "If this option is omitted, <command>COPY</command>
stops operation at the first error."
* Since we only support 'none' for now, 'none' means we don't want
ErrorSaveContext metadata,
so we should set cstate->escontext->details_wanted to false.
> BTW I have question and comment about v15 patch:
>
> > + {
> > + /*
> > + *
> > + * InputFunctionCall is more faster than
> > InputFunctionCallSafe.
> > + *
> > + */
>
> Have you measured this?
> When I tested it in an older patch, there were no big difference[3].
Thanks for pointing it out, I probably was over thinking.
> > - SAVEPOINT SCALAR SCHEMA SCHEMAS SCROLL SEARCH SECOND_P SECURITY
> SELECT
> > + SAVEPOINT SAVE_ERROR SCALAR SCHEMA SCHEMAS SCROLL SEARCH SECOND_P
> SECURITY SELECT
>
> There was a comment that we shouldn't add new keyword for this[4].
>
Thanks for pointing it out.
v1-0001-minor-refactor.no-cfbot
Description: Binary data
