On 2024/08/08 16:36, torikoshia wrote:
> Attached patches.
> 0001 adds new option 'silent' to log_verbosity and 0002 adds on_error and log_verbosity options to file_fdw.
Thanks for the patches!
Here are the review comments for 0001 patch.
+ <literal>silent</literal> excludes verbose messages.
This should clarify that in silent mode, not only verbose messages but also
default ones are suppressed?
+ cstate->opts.log_verbosity != COPY_LOG_VERBOSITY_SILENT)
I think using "cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT" instead
might improve readability.
- 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?
Here are the review comments for 0002 patch.
+ pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
+ ++skipped);
The skipped tuple count isn't accurate because fileIterateForeignScan() resets
"skipped" to 0.
+ 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?
+ for(;;)
+ {
Using "goto" here might improve readability instead of using a "for" loop.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
