Date: Thu, 11 Sep 2025 14:46:27 +0900 (JST)
Message-Id: <20250911.144627.1367548062212866518.kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: michael@paquier.xyz, david.g.johnston@gmail.com, tgl@sss.pgh.pa.us,
 zhjwpku@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: Make COPY format extendable: Extract COPY TO format
 implementations
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: 
 <CAD21AoBb3t7EcsjYT4w68p9OfMNwWTYsbSVaSRY6DRhi7sNRFg@mail.gmail.com>
References: 
 <CAD21AoCidyfKcpf9-f2Np8kWgkM09c4TjnS1h1hcO_-CCbjeqw@mail.gmail.com>
	<20250910.114058.944005985178219874.kou@clear-code.com>
	<CAD21AoBb3t7EcsjYT4w68p9OfMNwWTYsbSVaSRY6DRhi7sNRFg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20250911.144627.1367548062212866518.kou%40clear-code.com>
Precedence: bulk

Hi,

In <CAD21AoBb3t7EcsjYT4w68p9OfMNwWTYsbSVaSRY6DRhi7sNRFg@mail.gmail.com>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Wed, 10 Sep 2025 00:36:38 -0700,
  Masahiko Sawada <sawada.mshk@gmail.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();
  ...;
}

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 that this is acceptable because this must be a light
process. This must not have negative performance impact.


Thanks,
-- 
kou