Date: Mon, 29 Jan 2024 18:45:23 +0900 (JST)
Message-Id: <20240129.184523.703743711014180020.kou@clear-code.com>
To: zhjwpku@gmail.com
Cc: sawada.mshk@gmail.com, andrew@dunslane.net, michael@paquier.xyz,
 nathandbossart@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: 
 <CAEG8a3Jnmbjw82OiSvRK3v9XN2zSshsB8ew1mZCQDAkKq6r9YQ@mail.gmail.com>
References: 
 <CAEG8a3+_oK1p7T9iew29CbXJfK7LYSgh-6gT_=SbGuJZA65Q_w@mail.gmail.com>
	<CAD21AoDjrMWf4bmqGGt8py3xEs98JPUH0ePBD2LV5D4Ts5JpYQ@mail.gmail.com>
	<CAEG8a3Jnmbjw82OiSvRK3v9XN2zSshsB8ew1mZCQDAkKq6r9YQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Mon_Jan_29_18_45_23_2024_013)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20240129.184523.703743711014180020.kou%40clear-code.com>
Precedence: bulk

----Next_Part(Mon_Jan_29_18_45_23_2024_013)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi,

In <CAEG8a3Jnmbjw82OiSvRK3v9XN2zSshsB8ew1mZCQDAkKq6r9YQ@mail.gmail.com>
  "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 29 Jan 2024 11:37:07 +0800,
  Junwang Zhao <zhjwpku@gmail.com> wrote:

>> > > Does it make sense to pass only non-builtin options to the custom
>> > > format callback after parsing and evaluating the builtin options? That
>> > > is, we parse and evaluate only the builtin options and populate
>> > > opts_out first, then pass each rest option to the custom format
>> > > handler callback. The callback can refer to the builtin option values.
>>
>> What I imagined is that while parsing the all specified options, we
>> evaluate builtin options and we add non-builtin options to another
>> list. Then when parsing a non-builtin option, we check if this option
>> already exists in the list. If there is, we raise the "option %s not
>> recognized" error.". Once we complete checking all options, we pass
>> each option in the list to the callback.

I implemented this idea and the following ideas:

1. Add init callback for initialization
2. Change GetFormat() to FillCopyXXXResponse()
   because JSON format always use 1 column
3. FROM only: Eliminate more cstate->opts.csv_mode branches
   (This is for performance.)

See the attached v9 patch set for details. Changes since v7:

0001:

* Move CopyToProcessOption() calls to the end of
  ProcessCopyOptions() for easy to option validation
* Add CopyToState::CopyToInit() and call it in
  ProcessCopyOptionFormatTo()
* Change CopyToState::CopyToGetFormat() to
  CopyToState::CopyToFillCopyOutResponse() and use it in
  SendCopyBegin()

0002:

* Move CopyFromProcessOption() calls to the end of
  ProcessCopyOptions() for easy to option validation
* Add CopyFromState::CopyFromInit() and call it in
  ProcessCopyOptionFormatFrom()
* Change CopyFromState::CopyFromGetFormat() to
  CopyFromState::CopyFromFillCopyOutResponse() and use it in
  ReceiveCopyBegin()
* Rename NextCopyFromRawFields() to
  NextCopyFromRawFieldsInternal() and pass the read
  attributes callback explicitly to eliminate more
  cstate->opts.csv_mode branches


Thanks,
-- 
kou

----Next_Part(Mon_Jan_29_18_45_23_2024_013)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="v9-0001-Extract-COPY-TO-format-implementations.patch"