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

On Thu, Jan 23, 2025 at 1:12 AM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> I noticed that the last patch set (v27) can't be applied to
> the current master. I've rebased on the current master and
> created v28 patch set. No code change.

Thank you for updating the patch!

While 0001 and 0002 look good to me overall, we still need to polish
subsequent patches. Here are review comments:

---
I still find that it would not be a good idea to move all copy-related
struct definitions to copyapi.h because we need to include copyapi.h
file into a .c file even if the file is not related to the custom copy
format routines. I think that copyapi.h should have only the
definitions of CopyToRoutine and CopyFromRoutine as well as some
functions related to the custom copy format. Here is an idea:

- CopyToState and CopyFromState are defined in copyto_internal.h (new
file) and copyfrom_internal.h, respectively.
- These two files #include's copy.h and other necessary header files.
- copyapi.h has only CopyToRoutine and CopyFromRoutine and #include's
both copyfrom_internal.h and copyto_internal.h.
- copyto.c, copyfrom.c and copyfromparse.c #include copyapi.h

Some advantages of this idea:

- we can keep both CopyToState and CopyFromState private in _internal.h files.
- custom format extension can include copyapi.h to provide a custom
copy format routine and to access the copy state data.
- copy-related .c files won't need to include copyapi.h if they don't
use custom copy format routines.

---
The 0008 patch introduces CopyFromStateRead(). While it would be a
good start, I think we can consider sorting out low-level
communication functions more. For example, CopyReadBinaryData() uses
the internal 64kB buffer but some custom copy format extensions might
want to use a larger buffer in its own implementation, which would
require exposing CopyGetData() etc. Given that we might expose more
functions to provide more ways for extensions, we might want to rename
CopyFromStateRead().

---
While we get the format routines for custom formats in
ProcessCopyOptionFormat(), we do that for built-in formats in
BeginCopyTo(), which seems odd to me. I think we can have
CopyToGetRoutine() responsible for getting CopyToRoutine for built-in
formats as well as custom format. The same is true for
CopyFromRoutine.

---
Copy[To|From]Routine for built-in formats are missing to set the node type.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com