Date: Tue, 19 Nov 2024 10:31:15 +0900 (JST)
Message-Id: <20241119.103115.418773618386970482.kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: michael@paquier.xyz, pgsql-hackers@postgresql.org
Subject: Re: Make COPY format extendable: Extract COPY TO format
 implementations
From: Sutou Kouhei <kou@clear-code.com>
In-Reply-To: <CAD21AoC=DX5QQVb27C6UdpPfY-F=-PGnQ1u6rWo69DV=4EtDdw@mail.gmail.com>
References: <20241114.161948.1677325020727842666.kou@clear-code.com>
	<CAD21AoBhzGa0XyeOv5bCy5LxC+WrpufA+pfrrdZeK4XnTzF7kw@mail.gmail.com>
	<CAD21AoC=DX5QQVb27C6UdpPfY-F=-PGnQ1u6rWo69DV=4EtDdw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20241119.103115.418773618386970482.kou%40clear-code.com>
Precedence: bulk

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.

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?


Thanks,
-- 
kou