public inbox for [email protected]  

help / color / mirror / Atom feed

From: Fujii Masao <[email protected]>
To: torikoshia <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: [email protected]
Cc: Pgsql Hackers <[email protected]>
Subject: Re: Add on_error and log_verbosity options to file_fdw
Date: Fri, 20 Sep 2024 11:27:42 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<CAD21AoBij_2cy5T7ihk9ArRQzDcXVHoK13vzy34zAUAMLgrKkQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>



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

view thread (13+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add on_error and log_verbosity options to file_fdw
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox