Re: POC PATCH: copy from ... exceptions to: (was Re: VLDB Features)

On Wed, Jan 10, 2024 at 4:42 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:

> Yeah, I'm still thinking it's better to implement this feature
> incrementally. Given we're closing to feature freeze, I think it's
> unlikely to get the whole feature into PG17 since there are still many
> design discussions we need in addition to what Torikoshi-san pointed
> out. The feature like "ignore errors" or "logging errors" would have
> higher possibilities. Even if we get only these parts of the whole
> "error table" feature into PG17, it will make it much easier to
implement "error tables" feature.

+1.
I'm also going to make patch for "logging errors", since this
functionality is isolated from v7 patch.

> Seems promising. I'll look at the patch.
Thanks a lot!
Sorry to attach v2 if you already reviewed v1..

On 2024-01-11 12:13, jian he wrote:
> On Tue, Jan 9, 2024 at 10:36 PM torikoshia <torikoshia(at)oss(dot)nttdata(dot)com>
> wrote:
>>
>> On Tue, Dec 19, 2023 at 10:14 AM Masahiko Sawada
>> <sawada(dot)mshk(at)gmail(dot)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.

Thanks for reviewing!

Updated the patch merging your suggestions except below points:

> + cstate->num_errors = 0;

Since cstate is already initialized in below lines, this may be
redundant.

| /* Allocate workspace and zero all fields */
| cstate = (CopyFromStateData *) palloc0(sizeof(CopyFromStateData));

> + Assert(!cstate->escontext->details_wanted);

I'm not sure this is necessary, considering we're going to add other
options like 'table' and 'log', which need details_wanted soon.

--
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation