On Tue, Feb 25, 2025 at 3:52 PM Sutou Kouhei <k...@clear-code.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
v34-0002-Refactor-COPY-FROM-to-use-format-callback-functi.patch
Description: Binary data
v34-0001-Refactor-COPY-TO-to-use-format-callback-function.patch
Description: Binary data
