Re: Add on_error and log_verbosity options to file_fdw

On 2024-09-11 23:50, Fujii Masao wrote:

Thanks for your comments!

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

Agreed.

> + cstate->opts.log_verbosity !=
> COPY_LOG_VERBOSITY_SILENT)
>
> I think using "cstate->opts.log_verbosity >=
> COPY_LOG_VERBOSITY_DEFAULT" instead
> might improve readability.

Agreed.
I've also modified a similar code in fileEndForeignScan() in 0002 patch.

> - 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.

Should we assign 0 to COPY_LOG_VERBOSITY_SILENT and change
opts_out->log_verbosity after the initialization?

> 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.

Ugh. Fixed to use cstate->num_errors.
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.
I'm going to discuss it in another thread.

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

```
+ /* Report that this tuple was skipped by the ON_ERROR clause
*/
+ pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
+ ++skipped);
+
+ /* Repeat NextCopyFrom() until no soft error occurs */
+ continue;
```

> + 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":

```
start_of_getting_tuple:

if (!NextCopyFrom(cstate, econtext,
slot->tts_values, slot->tts_isnull))
goto end_of_fileread;

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)

/*
* Just make ErrorSaveContext ready for the next
NextCopyFrom.
* Since we don't set details_wanted and error_data is not
to
* be filled, just resetting error_occurred is enough.
*/
cstate->escontext->error_occurred = false;

/* Report that this tuple was skipped by the ON_ERROR clause */
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
cstate->num_errors);
goto start_of_fileread;
}
ExecStoreVirtualTuple(slot);

end_of_getting_tuple:
```

Attached v4 patches reflected these comments.

--
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation