MIME-Version: 1.0
References: <20241114.161948.1677325020727842666.kou@clear-code.com>
 <CAD21AoBhzGa0XyeOv5bCy5LxC+WrpufA+pfrrdZeK4XnTzF7kw@mail.gmail.com>
 <CAD21AoC=DX5QQVb27C6UdpPfY-F=-PGnQ1u6rWo69DV=4EtDdw@mail.gmail.com> <20241119.103115.418773618386970482.kou@clear-code.com>
In-Reply-To: <20241119.103115.418773618386970482.kou@clear-code.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 18 Nov 2024 20:44:25 -0800
Message-ID: <CAD21AoByMxtzg9TExQCm2jm1oCN1DwFtNj7Cf_ewSyAwV1c7iQ@mail.gmail.com>
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
To: Sutou Kouhei <kou@clear-code.com>
Cc: michael@paquier.xyz, pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAD21AoByMxtzg9TExQCm2jm1oCN1DwFtNj7Cf_ewSyAwV1c7iQ%40mail.gmail.com>
Precedence: bulk

On Mon, Nov 18, 2024 at 5:31=E2=80=AFPM Sutou Kouhei <kou@clear-code.com> w=
rote:
>
> Hi,
>
> In <CAD21AoC=3DDX5QQVb27C6UdpPfY-F=3D-PGnQ1u6rWo69DV=3D4EtDdw@mail.gmail.=
com>
>   "Re: Make COPY format extendable: Extract COPY TO format implementation=
s" 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 *nfiel=
ds)
> > +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,

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