On Fri, Feb 28, 2025 at 2:16 PM Sutou Kouhei <k...@clear-code.com> wrote:
>
> Hi,
>
> Thanks for following up the patch!
>
> In <cad21aoba414q76lthy65njfwbjoxxn1bdffsd_nbht2wpus...@mail.gmail.com>
> "Re: pgsql: Refactor COPY FROM to use format callback functions." on Fri,
> 28 Feb 2025 12:56:19 -0800,
> Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> > Right. I've attached the updated patch.
>
> In general, this approach will work but could you keep
> the same signature for backward compatibility?
>
> > --- a/src/backend/commands/copyfromparse.c
> > +++ b/src/backend/commands/copyfromparse.c
>
> > +bool
> > +NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields,
> > bool is_csv)
> > +{
> > + return NextCopyFromRawFieldsInternal(cstate, fields, nfields, is_csv);
> > +}
>
> NextCopyFromRawFields() uses
> NextCopyFromRawFields(CopyFromState cstate, char ***fields,
> int *nfields) (no "bool is_csv") before we remove it. So
> could you use the no "bool is_csv" signature for backward
> compatibility?
>
> bool
> NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
> {
> return NextCopyFromRawFieldsInternal(cstate, fields, nfields,
> cstate->opts.csv_mode);
> }
I like this idea.
>
>
> > @@ -738,6 +742,15 @@ CopyReadBinaryData(CopyFromState cstate, char *dest,
> > int nbytes)
> > /*
> > * Read raw fields in the next line for COPY FROM in text or csv mode.
> > * Return false if no more lines.
> > + */
> > +bool
> > +NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields,
> > bool is_csv)
> > +{
> > + return NextCopyFromRawFieldsInternal(cstate, fields, nfields, is_csv);
> > +}
> > +
> > +/*
> > + * Workhorse for NextCopyFromRawFields().
>
> It may be better that we don't split docs for
> NextCopyFromRawFields() and
> NextCopyFromRawFieldsInternal(). How about referring the
> NextCopyFromRawFieldsInternal() doc from the
> NextCopyFromRawFields() doc something like the following?
>
> /*
> * See NextCopyFromRawFieldsInternal() for details.
> */
> bool
> NextCopyFromRawFields(...)
> {
> ...
> }
>
> /*
> * Workhorse for NextCopyFromRawFields().
> *
> * Read raw fields ...
> *
> * ...
> */
> static pg_attribute_always_inline bool
> NextCopyFromRawFieldsInternal()
> {
> ...
> }
>
Agreed.
I've updated the patch. I'm going to push it, barring any objections
and further comments.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
v2-0001-Re-export-NextCopyFromRawFields-to-copy.h.patch
Description: Binary data
