public inbox for [email protected]
help / color / mirror / Atom feedFrom: Junwang Zhao <[email protected]>
To: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Sutou Kouhei <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
Date: Sat, 9 Dec 2023 16:39:11 +0800
Message-ID: <CAEG8a3LpvSjgYxr_x9z_Ddz+PsdBAaMRpTh23En6oi_S2udRtw@mail.gmail.com> (raw)
In-Reply-To: <TY3PR01MB9889C9234CD220A3A7075F0DF589A@TY3PR01MB9889.jpnprd01.prod.outlook.com>
References: <CAEG8a3Jf7kPV3ez5OHu-pFGscKfVyd9KkubMF199etkfz=EPRg@mail.gmail.com>
<[email protected]>
<CAEG8a3K9dE2gt3+K+h=DwTqMenR84aeYuYS+cty3SR3LAeDBAQ@mail.gmail.com>
<[email protected]>
<CAEG8a3LSRhK601Bn50u71BgfNWm4q3kv-o-KEq=hrbyLbY_EsA@mail.gmail.com>
<[email protected]>
<CAEG8a3+BtS5UiKgectccCYQ367hRec7AeCUA6vLrkKAQmSA2Yg@mail.gmail.com>
<[email protected]>
<CAD21AoAhcZkAp_WDJ4sSv_+g2iCGjfyMFgeu7MxjnjX_FutZAg@mail.gmail.com>
<CAEG8a3+rto3btXDK9EON=udnCc7yN7XD4WomyUUO-MiBSeDrEw@mail.gmail.com>
<TY3PR01MB9889C9234CD220A3A7075F0DF589A@TY3PR01MB9889.jpnprd01.prod.outlook.com>
On Sat, Dec 9, 2023 at 10:43 AM Hayato Kuroda (Fujitsu)
<[email protected]> wrote:
>
> Dear Junagn, Sutou-san,
>
> Basically I agree your point - improving a extendibility is good.
> (I remember that this theme was talked at Japan PostgreSQL conference)
> Below are my comments for your patch.
>
> 01. General
>
> Just to confirm - is it OK to partially implement APIs? E.g., only COPY TO is
> available. Currently it seems not to consider a case which is not implemented.
>
For partially implements, we can leave the hook as NULL, and check the NULL
at *ProcessCopyOptions* and report error if not supported.
> 02. General
>
> It might be trivial, but could you please clarify how users can extend? Is it OK
> to do below steps?
>
> 1. Create a handler function, via CREATE FUNCTION,
> 2. Register a handler, via new SQL (CREATE COPY HANDLER),
> 3. Specify the added handler as COPY ... FORMAT clause.
>
My original thought was option 2, but as Michael point, option 1 is
the right way
to go.
> 03. General
>
> Could you please add document-related tasks to your TODO? I imagined like
> fdwhandler.sgml.
>
> 04. General - copyright
>
> For newly added files, the below copyright seems sufficient. See applyparallelworker.c.
>
> ```
> * Copyright (c) 2023, PostgreSQL Global Development Group
> ```
>
> 05. src/include/catalog/* files
>
> IIUC, 8000 or higher OIDs should be used while developing a patch. src/include/catalog/unused_oids
> would suggest a candidate which you can use.
Yeah, I will run renumber_oids.pl at last.
>
> 06. copy.c
>
> I felt that we can create files per copying methods, like copy_{text|csv|binary}.c,
> like indexes.
> How do other think?
Not sure about this, it seems others have put a lot of effort into
splitting TO and From.
Also like to hear from others.
>
> 07. fmt_to_name()
>
> I'm not sure the function is really needed. Can we follow like get_foreign_data_wrapper_oid()
> and remove the funciton?
I have referenced some code from greenplum, will remove this.
>
> 08. GetCopyRoutineByName()
>
> Should we use syscache for searching a catalog?
>
> 09. CopyToFormatTextSendEndOfRow(), CopyToFormatBinaryStart()
>
> Comments still refer CopyHandlerOps, whereas it was renamed.
>
> 10. copy.h
>
> Per foreign.h and fdwapi.h, should we add a new header file and move some APIs?
>
> 11. copy.h
>
> ```
> -/* These are private in commands/copy[from|to].c */
> -typedef struct CopyFromStateData *CopyFromState;
> -typedef struct CopyToStateData *CopyToState;
> ```
>
> Are above changes really needed?
>
> 12. CopyFormatOptions
>
> Can we remove `bool binary` in future?
>
> 13. external functions
>
> ```
> +extern void CopyToFormatTextStart(CopyToState cstate, TupleDesc tupDesc);
> +extern void CopyToFormatTextOneRow(CopyToState cstate, TupleTableSlot *slot);
> +extern void CopyToFormatTextEnd(CopyToState cstate);
> +extern void CopyFromFormatTextStart(CopyFromState cstate, TupleDesc tupDesc);
> +extern bool CopyFromFormatTextNext(CopyFromState cstate, ExprContext *econtext,
> +
> Datum *values, bool *nulls);
> +extern void CopyFromFormatTextErrorCallback(CopyFromState cstate);
> +
> +extern void CopyToFormatBinaryStart(CopyToState cstate, TupleDesc tupDesc);
> +extern void CopyToFormatBinaryOneRow(CopyToState cstate, TupleTableSlot *slot);
> +extern void CopyToFormatBinaryEnd(CopyToState cstate);
> +extern void CopyFromFormatBinaryStart(CopyFromState cstate, TupleDesc tupDesc);
> +extern bool CopyFromFormatBinaryNext(CopyFromState cstate,
> ExprContext *econtext,
> +
> Datum *values, bool *nulls);
> +extern void CopyFromFormatBinaryErrorCallback(CopyFromState cstate);
> ```
>
> FYI - If you add files for {text|csv|binary}, these declarations can be removed.
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
>
Thanks for all the valuable suggestions.
--
Regards
Junwang Zhao
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Make COPY format extendable: Extract COPY TO format implementations
In-Reply-To: <CAEG8a3LpvSjgYxr_x9z_Ddz+PsdBAaMRpTh23En6oi_S2udRtw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox