Horton

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

Sutou Kouhei <kou@clear-code.com>

View thread View on postgr.es
From: Sutou Kouhei <kou@clear-code.com>
To: sawada.mshk@gmail.com
Cc: michael@paquier.xyz, zhjwpku@gmail.com, pgsql-hackers@postgresql.org
Date: 2025-02-21T02:48:12Z
Lists: pgsql-hackers 
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

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.