Re: Add on_error and log_verbosity options to file_fdw

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.

>> +       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?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION