Re: Add on_error and log_verbosity options to file_fdw

On 2024-09-20 11:27, Fujii Masao wrote:

Thanks for your comments!

> On 2024/09/19 23:16, torikoshia wrote:
>>> -       COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional
>>> messages, default */
>>> -       COPY_LOG_VERBOSITY_VERBOSE, /* logs additional messages */
>>> +       COPY_LOG_VERBOSITY_SILENT = -1, /* logs none */
>>> +       COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional
>>> messages, default */
>>> +       COPY_LOG_VERBOSITY_VERBOSE,     /* logs additional messages
>>> */
>>>
>>> Why do we need to assign specific numbers like -1 or 0 in this enum
>>> definition?
>>
>> CopyFormatOptions is initialized by palloc0() at the beginning of
>> ProcessCopyOptions().
>> The reason to assign specific numbers here is to assign
>> COPY_LOG_VERBOSITY_DEFAULT to 0 as default value and sort elements of
>> enum according to the amount of logging.
>
> Understood.
>
>
>> BTW CopyFrom() also uses local variable skipped. It isn't reset like
>> file_fdw, but using local variable seems not necessary since we can
>> use cstate->num_errors here as well.
>
> Sounds reasonable to me.
>
>
>>> +               if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
>>> +                       cstate->escontext->error_occurred)
>>> +               {
>>> +                       /*
>>> +                        * Soft error occurred, skip this tuple and
>>> deal with error
>>> +                        * information according to ON_ERROR.
>>> +                        */
>>> +                       if (cstate->opts.on_error ==
>>> COPY_ON_ERROR_IGNORE)
>>>
>>> If COPY_ON_ERROR_IGNORE indicates tuple skipping, shouldn’t we not
>>> only reset
>>> error_occurred but also call "pgstat_progress_update_param" and
>>> continue within
>>> this block?
>>
>> I may misunderstand your comment, but I thought it to behave as you
>> expect in the below codes:
>
> The "on_error == COPY_ON_ERROR_IGNORE" condition isn't needed since
> "on_error != COPY_ON_ERROR_STOP" is already checked, and on_error
> accepts
> only two values "ignore" and "stop." I assume you added it with
> a future option in mind, like "set_to_null" (as discussed in another
> thread).
> However, I’m not sure how much this helps such future changes.
> So, how about simplifying the code by replacing "on_error !=
> COPY_ON_ERROR_STOP"
> with "on_error == COPY_ON_ERROR_IGNORE" at the top and removing
> the "on_error == COPY_ON_ERROR_IGNORE" check in the middle?
> It could improve readability.

Thanks for the explanation and suggestion.
Since there is almost the same code in copyfrom.c, attached 0003 patch
for refactoring both.

>>> +       for(;;)
>>> +       {
>>> Using "goto" here might improve readability instead of using a "for"
>>> loop.
>>
>> Hmm, AFAIU we need to return a slot here before the end of file is
>> reached.
>>
>> ```
>> --src/backend/executor/execMain.c [ExecutePlan()]
>>            /*
>>             * if the tuple is null, then we assume there is nothing
>> more to
>>             * process so we just end the loop...
>>             */
>>            if (TupIsNull(slot))
>>                break;
>> ```
>>
>> When ignoring errors, we have to keep calling NextCopyFrom() until we
>> find a non error tuple or EOF and to do so calling NextCopyFrom() in
>> for loop seems straight forward.
>>
>> Replacing the "for" loop using "goto" as follows is possible, but
>> seems not so readable because of the upward "goto":
>
> Understood.
>
>
>> Attached v4 patches reflected these comments.
>
> Thanks for updating the patches!
>
> The tab-completion needs to be updated to support the "silent" option?

Yes, updated 0002 patch.

--
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation