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

Hi,

In <CAD21AoA3KMddnjxY1hxth3f4f1wo8a8i2icgK6GEKqXNR_e6jA(at)mail(dot)gmail(dot)com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 31 Jan 2025 16:34:52 -0800,
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> Again, what I'd like to avoid is that we end up adding everything
> (including new fields we add in the future) related to copy operation
> to copyapi.h. For example, with v28 that moves both CopyFromState and
> CopyToState to copyapi.h, file_fdw.c includes unrelated CopyToState
> struct via copyfrom_internal.h -> copyapi.h. In addition to that, both
> copyfrom.c and copyfrom_internal.h did the same, which made me think
> copyfrom_internal.h mostly no longer plays its role. I'm very welcome
> to other ideas too if they could achieve the same goal.

For the propose, copyapi.h should not include
copy{to,from}_internal.h. If we do it, copyto.c includes
CopyFromState and copyfrom*.c include CopyToState.

What do you think about the following change? Note that
extensions must include copy{to,from}_internal.h explicitly
in addition to copyapi.h.

-----
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 10f80ef3654..a2dc2d04407 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -23,6 +23,8 @@
#include "access/xact.h"
#include "catalog/pg_authid.h"
#include "commands/copyapi.h"
+#include "commands/copyto_internal.h"
+#include "commands/copyfrom_internal.h"
#include "commands/defrem.h"
#include "executor/executor.h"
#include "mb/pg_wchar.h"
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 3f6b0031d94..7bcf1c6544b 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -29,6 +29,7 @@
#include "access/xact.h"
#include "catalog/namespace.h"
#include "commands/copyapi.h"
+#include "commands/copyfrom_internal.h"
#include "commands/progress.h"
#include "commands/trigger.h"
#include "executor/execPartition.h"
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index b016f43a711..7296745d6d2 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -63,6 +63,7 @@
#include <sys/stat.h>

#include "commands/copyapi.h"
+#include "commands/copyfrom_internal.h"
#include "commands/progress.h"
#include "executor/executor.h"
#include "libpq/libpq.h"
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index da281f32950..a69771ea6da 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -20,6 +20,7 @@

#include "access/tableam.h"
#include "commands/copyapi.h"
+#include "commands/copyto_internal.h"
#include "commands/progress.h"
#include "executor/execdesc.h"
#include "executor/executor.h"
diff --git a/src/include/commands/copyapi.h b/src/include/commands/copyapi.h
index 389f887b2c1..dfab62372a7 100644
--- a/src/include/commands/copyapi.h
+++ b/src/include/commands/copyapi.h
@@ -14,8 +14,7 @@
#ifndef COPYAPI_H
#define COPYAPI_H

-#include "commands/copyto_internal.h"
-#include "commands/copyfrom_internal.h"
+#include "commands/copy.h"

/*
* API structure for a COPY TO format implementation. Note this must be
diff --git a/src/test/modules/test_copy_format/test_copy_format.c b/src/test/modules/test_copy_format/test_copy_format.c
index d72d5c33c1b..c05d65557a9 100644
--- a/src/test/modules/test_copy_format/test_copy_format.c
+++ b/src/test/modules/test_copy_format/test_copy_format.c
@@ -14,6 +14,8 @@
#include "postgres.h"

#include "commands/copyapi.h"
+#include "commands/copyfrom_internal.h"
+#include "commands/copyto_internal.h"
#include "commands/defrem.h"

PG_MODULE_MAGIC;
-----

>> If we use the approach, we can't show error position when a
>> custom COPY format handler function returns invalid routine
>> because DefElem for the "format" option isn't available in
>> BeginCopyTo(). Is it acceptable? If it's acceptable, let's
>> use the approach.
>
> I think we can live with it. All errors happening while processing the
> copy options don't necessarily show the error position.

OK. I attach the v31 patch set that uses this
approach. Mainly, 0003 and 0006 were changed. The v31 patch
set also includes the above
copyapi.h/copy{to,from}_internal.h related changes.

If we have a feature that returns a function name from Oid,
we can improve error messages by including function name
(format name) when a custom format handler function returns
not Copy{To,From}Routine...

Thanks,
--
kou