Horton

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

Masahiko Sawada <sawada.mshk@gmail.com>

View thread View on postgr.es
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Sutou Kouhei <kou@clear-code.com>
Cc: michael@paquier.xyz, pgsql-hackers@postgresql.org
Date: 2024-11-19T04:44:25Z
Lists: pgsql-hackers 
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.

Regards,

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

Commits

  1. Refactor Copy{From|To}GetRoutine() to use pass-by-reference argument.

  2. Refactor COPY FROM to use format callback functions.

  3. Refactor COPY TO to use format callback functions.

  4. Another try to fix BF failure introduced in commit ddd5f4f54a.

  5. Revert "Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM"

  6. Improve COPY TO performance when server and client encodings match

  7. Simplify signature of CopyAttributeOutCSV() in copyto.c

  8. Revert "Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO"

  9. Refactor CopyAttributeOut{CSV,Text}() to use a callback in COPY TO

  10. Refactor CopyReadAttributes{CSV,Text}() to use a callback in COPY FROM

  11. Add progress reporting of skipped tuples during COPY FROM.

  12. pgbench: Add \syncpipeline

  13. meson: Make gzip and tar optional

  14. Export the external file reader used in COPY FROM as APIs.