Re: Add new error_action COPY ON_ERROR "log"

On Tue, Mar 26, 2024 at 7:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> > Please see the attached v9 patch set.
>
> Thank you for updating the patch! The patch mostly looks good to me.
> Here are some minor comments:

Thanks for looking into this.

> ---
>  /* non-export function prototypes */
> -static char *limit_printout_length(const char *str);
> -
> static void ClosePipeFromProgram(CopyFromState cstate);
>
> Now that we have only one function we should replace "prototypes" with
> "prototype".

Well no. We might add a few more (never know). A quick look around the
GUCs under /* GUCs */ tells me that plural form there is being used
even just one GUC is defined (xlogprefetcher.c for instance).

> ---
> +                                                ereport(NOTICE,
> +
> errmsg("data type incompatibility at line %llu for column %s: \"%s\"",
> +
>      (unsigned long long) cstate->cur_lineno,
> +
>      cstate->cur_attname,
> +
>      attval));
>
> I guess it would be better to make the log message clearer to convey
> what we did for the malformed row. For example, how about something
> like "skipping row due to data type incompatibility at line %llu for
> column %s: \"s\""?

The summary message which gets printed at the end says that "NOTICE:
6 rows were skipped due to data type incompatibility". Isn't this
enough? If someone is using ON_ERROR 'ignore', it's quite natural that
such rows get skipped softly and the summary message can help them,
no?

> ---
>  extern void CopyFromErrorCallback(void *arg);
> +extern char *limit_printout_length(const char *str);
>
> I don't disagree with exposing the limit_printout_length() function
> but I think it's better to rename it for consistency with other
> exposed COPY command functions. Only this function is snake-case. How
> about CopyLimitPrintoutLength() or similar?

WFM. Although its implementation is not related to COPY code, COPY is
the sole user of it right now, so I'm fine with it. Done that.

> FWIW I'm going to merge two patches before the push.

Done that.

Please see the attached v10 patch.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com