Message-ID: <81844000-703c-408a-9296-14f5e176098d@oss.nttdata.com>
Date: Fri, 20 Sep 2024 11:27:42 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add on_error and log_verbosity options to file_fdw
Content-Language: en-US
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: Michael Paquier <michael@paquier.xyz>, sawada.mshk@gmail.com,
 Pgsql Hackers <pgsql-hackers@postgresql.org>
References: <ab59dad10490ea3734cf022b16c24cfd@oss.nttdata.com>
 <841721f3ca9a69574a72eb141c83933c@oss.nttdata.com>
 <CAD21AoBij_2cy5T7ihk9ArRQzDcXVHoK13vzy34zAUAMLgrKkQ@mail.gmail.com>
 <Zp7x8Y-SpbkZtpE-@paquier.xyz>
 <c22f047faf8aea56acecb26449b3f454@oss.nttdata.com>
 <79a336ccebc70401625743686c41b81a@oss.nttdata.com>
 <b0d44cd9-a7f7-469c-9ecf-b0115b86ca70@oss.nttdata.com>
 <5739a8ee4320a31ca04752fe3972aa73@oss.nttdata.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <5739a8ee4320a31ca04752fe3972aa73@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/81844000-703c-408a-9296-14f5e176098d%40oss.nttdata.com>
Precedence: bulk


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