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

On Thu, Feb 27, 2025 at 7:57 PM Sutou Kouhei <kou@clear-code.com> wrote:
>
> Hi,
>
> In <CAD21AoDABLkUTTOwWa1he6gbc=nM46COMu-BvWjc_i6USnNbHw@mail.gmail.com>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 27 Feb 2025 15:24:26 -0800,
>   Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> > Pushed the 0001 patch.
>
> Thanks!
>
> > Regarding the 0002 patch, I realized we stopped exposing
> > NextCopyFromRawFields() function:
> >
> >  --- a/src/include/commands/copy.h
> > +++ b/src/include/commands/copy.h
> > @@ -107,8 +107,6 @@ extern CopyFromState BeginCopyFrom(ParseState
> > *pstate, Relation rel, Node *where
> >  extern void EndCopyFrom(CopyFromState cstate);
> >  extern bool NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
> >                          Datum *values, bool *nulls);
> > -extern bool NextCopyFromRawFields(CopyFromState cstate,
> > -                                 char ***fields, int *nfields);
> >
> > I think that this change is not relevant with the refactoring and
> > probably we should keep it exposed as extension might be using it.
> > Considering that we added pg_attribute_always_inline to the function,
> > does it work even if we omit pg_attribute_always_inline to its
> > function declaration in the copy.h file?
>
> Unfortunately, no. The inline + constant argument
> optimization requires "static".
>
> How about the following?
>
> static pg_attribute_always_inline bool
> NextCopyFromRawFieldsInternal(CopyFromState cstate, char ***fields, int *nfields, bool is_csv)
> {
>     ...
> }
>
> bool
> NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
> {
>     return NextCopyFromRawFieldsInternal(cstate, fields, nfields, cstate->opts.csv_mode);
> }
>

Thank you for the confirmation.

I initially thought it would be acceptable to stop
NextCopyFromRawFields exposed since NextCopyFrom() could serve as an
alternative. For example, the NextCopyFromRawFields() function was
originally exposed in commit 8ddc05fb01ee2c primarily to support
extension modules like file_fdw but file_fdw wasn't utilizing this
API. I pushed the patch without the above change. Unfortunately, this
commit subsequently broke file_text_array_fdw[1] and made BF animal
crake unhappy[2].

Upon examining file_text_array_fdw more closely, I realized that
NextCopyFrom() may not be a suitable replacement for
NextCopyFromRawFields() in certain scenarios. Specifically,
NextCopyFrom() assumes that the caller has prior knowledge of the
source data's column count, making it inadequate for cases where
extensions like file_text_array_fdw need to construct an array of
source data with an unknown number of columns. In such situations,
NextCopyFromRawFields() proves to be more practical. Given these
considerations, I'm now leaning towards implementing the proposed
change. Thoughts?

Regards,

[1] https://github.com/adunstan/file_text_array_fdw
[2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2025-02-28%2018%3A47%3A02

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