Hi, In <cad21aobnfkdbjnu-zonnpg820zxyc0futslrj-udrqu4qp2...@mail.gmail.com> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Fri, 22 Nov 2024 13:01:06 -0800, Masahiko Sawada <sawada.m...@gmail.com> wrote:
>> @@ -1237,7 +1219,7 @@ CopyReadLine(CopyFromState cstate, bool is_csv) >> /* >> * CopyReadLineText - inner loop of CopyReadLine for text mode >> */ >> -static pg_attribute_always_inline bool >> +static bool >> CopyReadLineText(CopyFromState cstate, bool is_csv) >> >> Is this an intentional change? >> CopyReadLineText() has "bool in_csv". > > 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. I see. If explicit "inline" isn't related to performance, we don't need explicit "inline". > I've attached the v25 patches that squashed the minor changes I made > in v24 and incorporated all comments I got so far. I think these two > patches are in good shape. Could you rebase remaining patches on top > of them so that we can see the big picture of this feature? OK. I'll work on it. > Regarding exposing the structs such as CopyToStateData, v22-0004 patch > moves most of all copy-related structs to copyapi.h from copyto.c, > copyfrom_internal.h, and copy.h, which seems odd to me. I think we can > expose CopyToStateData (and related structs) in a new file > copyto_internal.h and keep other structs in the original header files. Custom COPY format extensions need to use CopyToStateData/CopyFromStateData. For example, CopyToStateData::rel is used to retrieve table schema. If we move CopyToStateData to copyto_internal.h not copyapi.h, custom COPY format extensions need to include copyto_internal.h. I feel that it's strange that extensions need to use internal headers. What is your real concern? If you don't want to export CopyToStateData/CopyFromStateData entirely, we can provide accessors only for some members of them. FYI: We discussed this in the past. For example: https://www.postgresql.org/message-id/flat/20240115.152350.1128880926282754664.kou%40clear-code.com#1b523fb95e8fb46702f5568ae19e3649 Thanks, -- kou
