MIME-Version: 1.0
References: <CAD21AoBKMNsO+b6wahb6847xwFci1JCfV+JykoMziVgiFxB6cw@mail.gmail.com>
 <20250329.175753.98922209929527465.kou@clear-code.com> <CAD21AoDOcYah-nREv09BB3ZoB-k+Yf1XUfJcDMoq=LLvV1v75w@mail.gmail.com>
 <20250404.173839.2270460917518093037.kou@clear-code.com>
In-Reply-To: <20250404.173839.2270460917518093037.kou@clear-code.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Wed, 23 Apr 2025 23:44:55 -0700
Message-ID: <CAD21AoAXzwPC7jjPMTcT80hnzmPa2SUJkiqdYHweEY8sZscEMA@mail.gmail.com>
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
To: Sutou Kouhei <kou@clear-code.com>
Cc: david.g.johnston@gmail.com, tgl@sss.pgh.pa.us, zhjwpku@gmail.com, 
	michael@paquier.xyz, pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAD21AoAXzwPC7jjPMTcT80hnzmPa2SUJkiqdYHweEY8sZscEMA%40mail.gmail.com>
Precedence: bulk

On Fri, Apr 4, 2025 at 1:38=E2=80=AFAM Sutou Kouhei <kou@clear-code.com> wr=
ote:
>
> Hi,
>
> In <CAD21AoDOcYah-nREv09BB3ZoB-k+Yf1XUfJcDMoq=3DLLvV1v75w@mail.gmail.com>
>   "Re: Make COPY format extendable: Extract COPY TO format implementation=
s" on Mon, 31 Mar 2025 12:35:23 -0700,
>   Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> > Most of the queries under test_copy_format/sql verifies the input
> > patterns of the FORMAT option. I find that the regression tests
> > included in that directory probably should focus on testing new
> > callback APIs and some regression tests for FORMAT option handling can
> > be moved into the normal regression test suite (e.g., in copy.sql or a
> > new file like copy_format.sql). IIUC testing for invalid input
> > patterns can be done even without creating artificial wrong handler
> > functions.
>
> Can we clarify what should we do for the next patch set
> before we create the next patch set? Are the followings
> correct?
>
> 1. Move invalid input patterns in
>    src/test/modules/test_copy_format/sql/invalid.sql to
>    src/test/regress/sql/copy.sql as much as possible.
>    * We can do only 4 patterns in 16 patterns.
>    * Other tests in
>      src/test/modules/test_copy_format/sql/*.sql depend on
>      custom COPY handler for test. So we can't move to
>      src/test/regress/sql/copy.sql.
> 2. Create
>    src/test/modules/test_copy_format/sql/test_copy_format.sql
>    and move all contents in existing *.sql to the file

Agreed.

>
> >> We can do it but I suggest that we do it as a refactoring
> >> (or cleanup) in a separated patch for easy to review.
> >
> > I think that csv_mode and binary are used mostly in
> > ProcessCopyOptions() so probably we can use local variables for that.
> > I find there are two other places where to use csv_mode:
> > NextCopyFromRawFields() and CopyToTextLikeStart(). I think we can
> > simply check the handler function's OID there, or we can define macros
> > like COPY_FORMAT_IS_TEXT/CSV/BINARY checking the OID and use them
> > there.
>
> We need this change for "ready for merge", right?

I think so.

> Can we clarify items should be resolved for "ready for
> merge"?
>
> Are the followings correct?
>
> 1. Move invalid input patterns in
>    src/test/modules/test_copy_format/sql/invalid.sql to
>    src/test/regress/sql/copy.sql as much as possible.
> 2. Create
>    src/test/modules/test_copy_format/sql/test_copy_format.sql
>    and move all contents in existing *.sql to the file.
> 3. Add comments what the tests expect to
>    src/test/modules/test_copy_format/sql/test_copy_format.sql.
> 4. Remove CopyFormatOptions::{binary,csv_mode}.

Agreed with the above items.

> 5. Squash the "Support custom format" patch and the "Define
>    handler functions for built-in formats" patch.
>    * Could you do it when you push it? Or is it required for
>      "ready for merge"?

Let's keep them for now.

> 6. Use handler OID for detecting the default built-in format
>    instead of comparing the given format as string.
> 7. Update documentation.

Agreed.

>
> There are 3 unconfirmed suggested changes for tests in:
> https://www.postgresql.org/message-id/20250330.113126.433742864258096312.=
kou%40clear-code.com
>
> Here are my opinions for them:
>
> > 1.: There is no difference between single-quoting and
> >     double-quoting here. Because the information what quote
> >     was used for the given FORMAT value isn't remained
> >     here. Should we update gram.y?
> >
> > 2.: I don't have a strong opinion for it. If nobody objects
> >     it, I'll remove them.
> >
> > 3.: I don't have a strong opinion for it. If nobody objects
> >     it, I'll remove them.
>
> Is the 1. required for "ready for merge"? If so, is there
> any suggestion? I don't have a strong opinion for it.
>
> If there are no more opinions for 2. and 3., I'll remove
> them.

Agreed.

I think we would still need some rounds of reviews but the patch is
getting in good shape.

Regards,

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