Re: Make COPY format extendable: Extract COPY TO format implementations

Hi,

In <CACJufxG=njY32g=YAF4T6rvXySN56VFbYt4ffjLTRBYQTKPAFg(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Sun, 6 Apr 2025 19:29:46 +0800,
jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> I did a brief review of v39-0001 and v39-0002.
>
> text:
> COPY_FILE
> COPY_FRONTEND
> still appear on comments in copyfrom_internal.h and copyto.c,
> Should it be removed?

Good catch!
I found them in copy{from,to}_internal.h but couldn't find
them in copyto.c. It's a typo, right?

We should update them instead of removing them. I'll update
them in the next patch set.

> +#include "commands/copyto_internal.h"
> #include "commands/progress.h"
> #include "executor/execdesc.h"
> #include "executor/executor.h"
> #include "executor/tuptable.h"
>
> "copyto_internal.h" already include:
>
> #include "executor/execdesc.h"
> #include "executor/tuptable.h"
> so you should removed
> "
> #include "executor/execdesc.h"
> #include "executor/tuptable.h"
> "
> in copyto.c.

You're right. I'll update this too in the next patch set.

> CREATE FUNCTION test_copy_format(internal)
> RETURNS copy_handler
> AS 'MODULE_PATHNAME', 'test_copy_format'
> LANGUAGE C;
> src/backend/commands/copy.c: ProcessCopyOptions
> if (strcmp(fmt, "text") == 0)
> /* default format */ ;
> else if (strcmp(fmt, "csv") == 0)
> opts_out->csv_mode = true;
> else if (strcmp(fmt, "binary") == 0)
> opts_out->binary = true;
> else
> {
> List *qualified_format;
> ....
> }
> what if our customized format name is one of "csv", "binary", "text",
> then that ELSE branch will never be reached.
> then our customized format is being shadowed?

Right. We should not use customized format handlers to keep
backward compatibility.

> https://www.postgresql.org/docs/current/error-message-reporting.html
> "The extra parentheses were required before PostgreSQL version 12, but
> are now optional."
>
> means that
> ereport(NOTICE, (errmsg("CopyFromInFunc: attribute: %s",
> format_type_be(atttypid))));
> can change to
> ereport(NOTICE, errmsg("CopyFromInFunc: attribute: %s",
> format_type_be(atttypid)));
>
> all
> ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), ....
>
> can also be simplified to
>
> ereport(ERROR, errcode(ERRCODE_INVALID_PARAMETER_VALUE), ....

Oh, I didn't notice it. Can we do it as a separated patch
because we have many codes that use this style in
copy*.c. The separated patch should update this style at
once.

Thanks,
--
kou