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

On Tue, Feb 25, 2025 at 3:52 PM Sutou Kouhei <kou(at)clear-code(dot)com> wrote:
>
>

Thank you for reviewing the patches. I've addressed comments except
for the following comment:

> > --- a/src/backend/commands/copyfromparse.c
> > +++ b/src/backend/commands/copyfromparse.c
>
> > @@ -1087,7 +1132,7 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
>
> > static bool
> > -CopyReadLine(CopyFromState cstate)
> > +CopyReadLine(CopyFromState cstate, bool is_csv)
>
> > @@ -1163,7 +1208,7 @@ CopyReadLine(CopyFromState cstate)
>
> > static bool
> > -CopyReadLineText(CopyFromState cstate)
> > +CopyReadLineText(CopyFromState cstate, bool is_csv)
>
> We may want to add a comment why we don't use "inline" nor
> "pg_attribute_always_inline" here:
>
> https://www.postgresql.org/message-id/CAD21AoBNfKDbJnu-zONNpG820ZXYC0fuTSLrJ-UdRqU4qp2wog%40mail.gmail.com
>
> > Yes, I'm not sure it's really necessary to make it inline since the
> > benchmark results don't show much difference. Probably this is because
> > the function has 'is_csv' in some 'if' branches but the compiler
> > cannot optimize out the whole 'if' branches as most 'if' branches
> > check 'is_csv' and other variables.
>
> Or we can add "inline" not "pg_attribute_always_inline" here
> as a hint for compiler.

I think we should not add inline unless we see a performance
improvement. Also, I find that it would be independent with this
refactoring so we can add it later if needed.

I've attached updated patches.

Regards,

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