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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Sutou Kouhei <kou(at)clear-code(dot)com>
Cc: sawada(dot)mshk(at)gmail(dot)com, zhjwpku(at)gmail(dot)com, andrew(at)dunslane(dot)net, nathandbossart(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
Date: 2024-02-05 23:48:55
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Feb 05, 2024 at 06:05:15PM +0900, Sutou Kouhei wrote:
> In <ZcCKwAeFrlOqPBuN(at)paquier(dot)xyz>
> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Mon, 5 Feb 2024 16:14:08 +0900,
> Michael Paquier <michael(at)paquier(dot)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:

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

> + /*
> + * 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.

Attachment Content-Type Size
v13-0001-Extract-COPY-FROM-TO-format-implementations.patch text/x-diff 36.3 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message David Zhang 2024-02-05 23:51:03 Proposal for implementing OCSP Stapling in PostgreSQL
Previous Message Justin Pryzby 2024-02-05 22:21:46 Re: pgsql: Add EXPLAIN (MEMORY) to report planner memory consumption