On Fri, Feb 28, 2025 at 2:16 PM Sutou Kouhei <[email protected]> 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 <[email protected]> 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
