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

On Wed, Sep 10, 2025 at 10:46 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoBb3t7EcsjYT4w68p9OfMNwWTYsbSVaSRY6DRhi7sNRFg(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 10 Sep 2025 00:36:38 -0700,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > How about another idea like we move format-specific data to another
> > struct that embeds CopyFrom/ToStateData at the first field and have
> > CopyFrom/ToStart callback return memory with the size of that
> > struct?It resolves the concerns about adding an extra indirection
> > layer and extensions doesn't need to allocate memory for unnecessary
> > fields (used only for built-in formats). While extensions can access
> > the internal fields I think we can live with that given that there are
> > some similar precedents such as table AM's scan descriptions.
>
> The another idea looks like the following, right?
>
> struct CopyToStateBuiltInData
> {
> struct CopyToStateData parent;
>
> /* Members for built-in formats */
> ...;
> }
>
> typedef CopyToState *(*CopyToStart) (void);
>
> CopyToState
> BeginCopyTo(..., CopyToStart copy_to_start)
> {
> ...;
>
> /* Allocate workspace and zero all fields */
> cstate = copy_to_start();
> ...;
> }

Right.

> This idea will almost work. But we can't know which
> CopyToStart should be used before we parse "FORMAT" option
> of COPY.
>
> If we can iterate options twice in BeginCopy{To,From}(), we
> can know it. For example:
>
> BeginCopyTo(...)
> {
> ...;
>
> CopyToStart copy_to_start = NULL;
> foreach(option, options)
> {
> DefElem *defel = lfirst_node(DefElem, option);
>
> if (strcmp(defel->defname, "format") == 0)
> {
> char *fmt = defGetString(defel);
> if (strcmp(fmt, "text") == 0 ||
> strcmp(fmt, "csv") == 0 ||
> strcmp(fmt, "binary") == 0) {
> /* Use the builtin cstate */
> } else {
> copy_to_start = /* Detect CopyToStart for custom format */;
> }
> }
> }
> if (copy_to_start)
> cstate = copy_to_start();
> else
> cstate = (CopyToStateData *) palloc0(sizeof(CopyToStateBuiltInData));
> ...;
> }
>
> (It may be better that we add
> Copy{To,From}Routine::Copy{To,From}Allocate() instead of
> CopyToStart callback.)

I think we can use a local variable of CopyFormatOptions and memcpy it
to the opts of the returned cstate.

Regards,

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