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

On Mon, Nov 18, 2024 at 5:31 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
> Hi,
>
> In <CAD21AoC=DX5QQVb27C6UdpPfY-F=-PGnQ1u6rWo69DV=4EtDdw(at)mail(dot)gmail(dot)com>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 18 Nov 2024 17:02:41 -0800,
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.

Regards,

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