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
