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

On Mon, Nov 18, 2024 at 8:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Nov 18, 2024 at 5:31 PM Sutou Kouhei <kou@clear-code.com> wrote:
> >
> > Hi,
> >
> > In <CAD21AoC=DX5QQVb27C6UdpPfY-F=-PGnQ1u6rWo69DV=4EtDdw@mail.gmail.com>
> >   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 18 Nov 2024 17:02:41 -0800,
> >   Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > > I have a question about v22. We use pg_attribute_always_inline for
> > > some functions to avoid function call overheads. Applying it to
> > > CopyToTextLikeOneRow() and CopyFromTextLikeOneRow() are legitimate as
> > > we've discussed. But there are more function where the patch applied
> > > it to:
> > >
> > > -bool
> > > -NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
> > > +static pg_attribute_always_inline bool
> > > +NextCopyFromRawFields(CopyFromState cstate, char ***fields, int
> > > *nfields, bool is_csv)
> > >
> > > -static bool
> > > -CopyReadLineText(CopyFromState cstate)
> > > +static pg_attribute_always_inline bool
> > > +CopyReadLineText(CopyFromState cstate, bool is_csv)
> > >
> > > +static pg_attribute_always_inline void
> > > +CopyToTextLikeSendEndOfRow(CopyToState cstate)
> > >
> > > I think it's out of scope of this patch even if these changes are
> > > legitimate. Is there any reason for these changes?
> >
> > Yes for NextCopyFromRawFields() and CopyReadLineText().
> > No for CopyToTextLikeSendEndOfRow().
> >
> > NextCopyFromRawFields() and CopyReadLineText() have "bool
> > is_csv". So I think that we should use
> > pg_attribute_always_inline (or inline) like
> > CopyToTextLikeOneRow() and CopyFromTextLikeOneRow(). I think
> > that it's not out of scope of this patch because it's a part
> > of CopyToTextLikeOneRow() and CopyFromTextLikeOneRow()
> > optimization.
> >
> > Note: The optimization is based on "bool is_csv" parameter
> > and constant "true"/"false" argument function call. If we
> > can inline this function call, all "if (is_csv)" checks in
> > the function are removed.
>
> Understood, thank you for pointing this out.
>
> >
> > pg_attribute_always_inline (or inline) for
> > CopyToTextLikeSendEndOfRow() is out of scope of this
> > patch. You're right.
> >
> > I think that inlining CopyToTextLikeSendEndOfRow() is better
> > because it's called per row. But it's not related to the
> > optimization.
> >
> >
> > Should I create a new patch set without
> > pg_attribute_always_inline/inline for
> > CopyToTextLikeSendEndOfRow()? Or could you remove it when
> > you push?
>
> Since I'm reviewing the patch and the patch organization I'll include it.
>

I've extracted the changes to refactor COPY TO/FROM to use the format
callback routines from v23 patch set, which seems to be a better patch
split to me. Also, I've reviewed these changes and made some changes
on top of them. The attached patches are:

0001: make COPY TO use CopyToRoutine.
0002: minor changes to 0001 patch. will be fixed up.
0003: make COPY FROM use CopyFromRoutine.
0004: minor changes to 0003 patch. will be fixed up.

I've confirmed that v24 has a similar performance improvement to v23.
Please check these extractions and minor change suggestions.

Regards,

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