Horton

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

Michael Paquier <michael@paquier.xyz>

View thread View on postgr.es
From: Michael Paquier <michael@paquier.xyz>
To: Sutou Kouhei <kou@clear-code.com>
Cc: sawada.mshk@gmail.com, zhjwpku@gmail.com, andrew@dunslane.net, nathandbossart@gmail.com, pgsql-hackers@postgresql.org
Date: 2024-02-05T23:48:55Z
Lists: pgsql-hackers

Attachments

On Mon, Feb 05, 2024 at 06:05:15PM +0900, Sutou Kouhei wrote:
> In <ZcCKwAeFrlOqPBuN@paquier.xyz>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 5 Feb 2024 16:14:08 +0900,
>   Michael Paquier <michael@paquier.xyz> wrote:
>> 2) I have backpedaled on the postpare callback, which did not bring
>> much in clarity IMO while being a CSV-only callback.  Note that we
>> have in copyfromparse.c more paths that are only for CSV but the past
>> versions of the patch never cared about that.  This makes the text and
>> CSV implementations much closer to each other, as a result.
> 
> Ah, sorry. I forgot to eliminate cstate->opts.csv_mode in
> CopyReadLineText(). The postpare callback is for
> optimization. If it doesn't improve performance, we don't
> need to introduce it.

No worries.

> We may want to try eliminating cstate->opts.csv_mode in
> CopyReadLineText() for performance. But we don't need to
> do this in introducing CopyFromRoutine. We can defer it.
> 
> So I don't object removing the postpare callback.

Rather related, but there has been a comment from Andres about this
kind of splits a few hours ago, so perhaps this is for the best:
https://www.postgresql.org/message-id/20240205182118.h5rkbnjgujwzuxip%40awork3.anarazel.de

I'll reply to this one in a bit.

>>                                              Let me know if you have
>> comments about all that.
> 
> Here are some comments for the patch:

Thanks.  My head was spinning after reading the diffs more than 20
times :)

> fmgr_info ->
> finfo
> optinally ->
> optionally
> CopyFromRoutine->OneRow ->
> CopyFromRoutine->CopyFromOneRow
> CopyFromTextStart ->
> CopyFromBinaryStart
> CopyFromTextEnd ->
> CopyFromBinaryEnd

Fixed all these.

> How about passing CopyFromState cstate too like other
> callbacks for consistency?

Yes, I was wondering a bit if this can be useful for the custom
formats.

> +	/*
> +	 * Copy one row to a set of `values` and `nulls` of size tupDesc->natts.
> +	 *
> +	 * 'econtext' is used to evaluate default expression for each column that
> +	 * is either not read from the file or is using the DEFAULT option of COPY
> 
> or is ->
> or

"or is" is correct here IMO.

> Wow! I didn't know that we need to update typedefs.list when
> I add a "typedef struct".

That's for the automated indentation.  This is a habit I have when it
comes to work on shaping up patches to avoid weird diffs with pgindent
and new structure names.  It's OK to forget about it :)

Attaching a v13 for now.
--
Michael

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.