Hi,
In <CAD21AoAni3cKToPfdShTsc0NmaJOtbJuUb=skyz3Udj7HZY7dA@mail.gmail.com>
"Re: Make COPY format extendable: Extract COPY TO format implementations" on Thu, 20 Feb 2025 15:28:26 -0800,
Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Looking at the 0001 patch again, I have a question: we have
> CopyToTextLikeOneRow() for both CSV and text format:
>
> +/* Implementation of the per-row callback for text format */
> +static void
> +CopyToTextOneRow(CopyToState cstate, TupleTableSlot *slot)
> +{
> + CopyToTextLikeOneRow(cstate, slot, false);
> +}
> +
> +/* Implementation of the per-row callback for CSV format */
> +static void
> +CopyToCSVOneRow(CopyToState cstate, TupleTableSlot *slot)
> +{
> + CopyToTextLikeOneRow(cstate, slot, true);
> +}
>
> These two functions pass different is_csv value to that function,
> which is used as follows:
>
> + if (is_csv)
> + CopyAttributeOutCSV(cstate, string,
> +
> cstate->opts.force_quote_flags[attnum - 1]);
> + else
> + CopyAttributeOutText(cstate, string);
>
> However, we can know whether the format is CSV or text by checking
> cstate->opts.csv_mode instead of passing is_csv. That way, we can
> directly call CopyToTextLikeOneRow() but not via CopyToCSVOneRow() or
> CopyToTextOneRow(). It would not help performance since we already
> inline CopyToTextLikeOneRow(), but it looks simpler.
This means the following, right?
1. We remove CopyToTextOneRow() and CopyToCSVOneRow()
2. We remove "bool is_csv" parameter from CopyToTextLikeOneRow()
and use cstate->opts.csv_mode in CopyToTextLikeOneRow()
instead of is_csv
3. We use CopyToTextLikeOneRow() for
CopyToRoutineText::CopyToOneRow and
CopyToRoutineCSV::CopyToOneRow
If we use this approach, we can't remove the following
branch in compile time:
+ if (is_csv)
+ CopyAttributeOutCSV(cstate, string,
+ cstate->opts.force_quote_flags[attnum - 1]);
+ else
+ CopyAttributeOutText(cstate, string);
We can remove the branch in compile time with the current
approach (constant argument + inline).
It may have a negative performance impact because the "if"
is used many times with large data. (That's why we choose
the constant argument + inline approach in this thread.)
Thanks,
--
kou
