public inbox for [email protected]help / color / mirror / Atom feed
Re: Make COPY format extendable: Extract COPY TO format implementations 5+ messages / 4 participants [nested] [flat]
* Re: Make COPY format extendable: Extract COPY TO format implementations @ 2023-12-09 08:39 Junwang Zhao <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Junwang Zhao @ 2023-12-09 08:39 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Sutou Kouhei <[email protected]>; Andrew Dunstan <[email protected]>; Michael Paquier <[email protected]>; Masahiko Sawada <[email protected]>; [email protected] <[email protected]>; pgsql-hackers 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 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations @ 2023-12-09 11:38 Hannu Krosing <[email protected]> parent: Junwang Zhao <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Hannu Krosing @ 2023-12-09 11:38 UTC (permalink / raw) To: Junwang Zhao <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; Sutou Kouhei <[email protected]>; Andrew Dunstan <[email protected]>; Michael Paquier <[email protected]>; Masahiko Sawada <[email protected]>; [email protected] <[email protected]>; pgsql-hackers Hi Junwang Please also see my presentation slides from last years PostgreSQL Conference in Berlin (attached) The main Idea is to make not just "format", but also "transport" and "stream processing" extendable via virtual function tables. Btw, will any of you here be in Prague next week ? Would be a good opportunity to discuss this in person. Best Regards Hannu On Sat, Dec 9, 2023 at 9:39 AM Junwang Zhao <[email protected]> wrote: > > 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 > > Attachments: [application/pdf] Cloud-friendly COPY.pdf (682.8K, ../../CAMT0RQRqVo4fGDWHqOn+wr_eoiXQVfyC=8-c=H=y6VcNxi6BvQ@mail.gmail.com/2-Cloud-friendly%20COPY.pdf) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations @ 2023-12-09 21:01 Sutou Kouhei <[email protected]> parent: Hannu Krosing <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Sutou Kouhei @ 2023-12-09 21:01 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers Hi, In <CAMT0RQRqVo4fGDWHqOn+wr_eoiXQVfyC=8-c=H=y6VcNxi6BvQ@mail.gmail.com> "Re: Make COPY format extendable: Extract COPY TO format implementations" on Sat, 9 Dec 2023 12:38:46 +0100, Hannu Krosing <[email protected]> wrote: > Please also see my presentation slides from last years PostgreSQL > Conference in Berlin (attached) Thanks for sharing your idea here. > The main Idea is to make not just "format", but also "transport" and > "stream processing" extendable via virtual function tables. "Transport" and "stream processing" are out of scope in this thread. How about starting new threads for them and discuss them there? > Btw, will any of you here be in Prague next week ? Sorry. No. Thanks, -- kou ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Make COPY format extendable: Extract COPY TO format implementations @ 2023-12-11 10:44 Junwang Zhao <[email protected]> parent: Hannu Krosing <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Junwang Zhao @ 2023-12-11 10:44 UTC (permalink / raw) To: Hannu Krosing <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; Sutou Kouhei <[email protected]>; Andrew Dunstan <[email protected]>; Michael Paquier <[email protected]>; Masahiko Sawada <[email protected]>; [email protected] <[email protected]>; pgsql-hackers On Sat, Dec 9, 2023 at 7:38 PM Hannu Krosing <[email protected]> wrote: > > Hi Junwang > > Please also see my presentation slides from last years PostgreSQL > Conference in Berlin (attached) I read through the slides, really promising ideas, it's will be great if we can get there at last. > > The main Idea is to make not just "format", but also "transport" and > "stream processing" extendable via virtual function tables. The code is really coupled, it is not easy to do all of these in one round, it will be great if you have a POC patch. > > Btw, will any of you here be in Prague next week ? > Would be a good opportunity to discuss this in person. Sorry, no. > > > Best Regards > Hannu > > On Sat, Dec 9, 2023 at 9:39 AM Junwang Zhao <[email protected]> wrote: > > > > 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 > > > > -- Regards Junwang Zhao ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v2] Add "Backpatch" regions in wait_event_names.txt @ 2024-03-18 08:34 Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Bertrand Drouvot @ 2024-03-18 08:34 UTC (permalink / raw) When backpatching, adding an event will renumber others, which can make an extension report the wrong event until recompiled. This is due to the fact that generate-wait_event_types.pl sorts events to position them. The "Backpatch" region added here ensures no ordering is done for the wait events found after this delimiter. --- .../activity/generate-wait_event_types.pl | 26 ++++++++++++++++++- .../utils/activity/wait_event_names.txt | 19 +++++++++++++- 2 files changed, 43 insertions(+), 2 deletions(-) 100.0% src/backend/utils/activity/ diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl index f1adf0e8e7..d129d94889 100644 --- a/src/backend/utils/activity/generate-wait_event_types.pl +++ b/src/backend/utils/activity/generate-wait_event_types.pl @@ -38,7 +38,9 @@ die "Not possible to specify --docs and --code simultaneously" open my $wait_event_names, '<', $ARGV[0] or die; +my @backpatch_lines; my @lines; +my $backpatch = 0; my $section_name; my $note; my $note_name; @@ -59,10 +61,26 @@ while (<$wait_event_names>) { $section_name = $_; $section_name =~ s/^.*- //; + $backpatch = 0; next; } - push(@lines, $section_name . "\t" . $_); + # Are we dealing with backpatch wait events? + if (/^Backpatch:$/) + { + $backpatch = 1; + next; + } + + # Backpatch wait events won't be sorted during code generation + if ($gen_code && $backpatch) + { + push(@backpatch_lines, $section_name . "\t" . $_); + } + else + { + push(@lines, $section_name . "\t" . $_); + } } # Sort the lines based on the second column. @@ -70,6 +88,12 @@ while (<$wait_event_names>) my @lines_sorted = sort { uc((split(/\t/, $a))[1]) cmp uc((split(/\t/, $b))[1]) } @lines; +# If we are generating code then concat @lines_sorted and @backpatch_lines. +if ($gen_code) +{ + push(@lines_sorted, @backpatch_lines); +} + # Read the sorted lines and populate the hash table foreach my $line (@lines_sorted) { diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt index c08e00d1d6..b6303a0fdb 100644 --- a/src/backend/utils/activity/wait_event_names.txt +++ b/src/backend/utils/activity/wait_event_names.txt @@ -24,7 +24,11 @@ # SGML tables of wait events for inclusion in the documentation. # # When adding a new wait event, make sure it is placed in the appropriate -# ClassName section. +# ClassName section. If the wait event is backpatched from master to a version +# >= 17 then put it under a "Backpatch:" delimiter at the end of the related +# ClassName section (on the non master branches) or at its natural position on +# the master branch. +# Ensure that the backpatch regions are always empty on the master branch. # # WaitEventLWLock and WaitEventLock have their own C code for their wait event # enums and function names. Hence, for these, only the SGML documentation is @@ -61,6 +65,7 @@ WAL_SENDER_MAIN "Waiting in main loop of WAL sender process." WAL_SUMMARIZER_WAL "Waiting in WAL summarizer for more WAL to be generated." WAL_WRITER_MAIN "Waiting in main loop of WAL writer process." +Backpatch: # # Wait Events - Client @@ -82,6 +87,7 @@ WAIT_FOR_STANDBY_CONFIRMATION "Waiting for WAL to be received and flushed by the WAL_SENDER_WAIT_FOR_WAL "Waiting for WAL to be flushed in WAL sender process." WAL_SENDER_WRITE_DATA "Waiting for any activity when processing replies from WAL receiver in WAL sender process." +Backpatch: # # Wait Events - IPC @@ -149,6 +155,7 @@ WAL_RECEIVER_WAIT_START "Waiting for startup process to send initial data for st WAL_SUMMARY_READY "Waiting for a new WAL summary to be generated." XACT_GROUP_UPDATE "Waiting for the group leader to update transaction status at end of a parallel operation." +Backpatch: # # Wait Events - Timeout @@ -169,6 +176,7 @@ VACUUM_DELAY "Waiting in a cost-based vacuum delay point." VACUUM_TRUNCATE "Waiting to acquire an exclusive lock to truncate off any empty pages at the end of a table vacuumed." WAL_SUMMARIZER_ERROR "Waiting after a WAL summarizer error." +Backpatch: # # Wait Events - IO @@ -256,6 +264,7 @@ WAL_SYNC "Waiting for a WAL file to reach durable storage." WAL_SYNC_METHOD_ASSIGN "Waiting for data to reach durable storage while assigning a new WAL sync method." WAL_WRITE "Waiting for a write to a WAL file." +Backpatch: # # Wait Events - Buffer Pin @@ -265,6 +274,7 @@ Section: ClassName - WaitEventBufferPin BUFFER_PIN "Waiting to acquire an exclusive pin on a buffer." +Backpatch: # # Wait Events - Extension @@ -274,6 +284,8 @@ Section: ClassName - WaitEventExtension Extension "Waiting in an extension." +Backpatch: + # # Wait Events - LWLock # @@ -330,6 +342,8 @@ DSMRegistry "Waiting to read or update the dynamic shared memory registry." InjectionPoint "Waiting to read or update information related to injection points." SerialControl "Waiting to read or update shared <filename>pg_serial</filename> state." +# Don't create a Backpatch region here. + # # END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE) # @@ -377,6 +391,7 @@ SerialSLRU "Waiting to access the serializable transaction conflict SLRU cache." SubtransSLRU "Waiting to access the sub-transaction SLRU cache." XactSLRU "Waiting to access the transaction status SLRU cache." +# Don't create a Backpatch region here. # # Wait Events - Lock @@ -399,3 +414,5 @@ object "Waiting to acquire a lock on a non-relation database object." userlock "Waiting to acquire a user lock." advisory "Waiting to acquire an advisory user lock." applytransaction "Waiting to acquire a lock on a remote transaction being applied by a logical replication subscriber." + +Backpatch: -- 2.34.1 --Xg/TgUq/K7vNmQT2-- ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-03-18 08:34 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-12-09 08:39 Re: Make COPY format extendable: Extract COPY TO format implementations Junwang Zhao <[email protected]> 2023-12-09 11:38 ` Hannu Krosing <[email protected]> 2023-12-09 21:01 ` Sutou Kouhei <[email protected]> 2023-12-11 10:44 ` Junwang Zhao <[email protected]> 2024-03-18 08:34 [PATCH v2] Add "Backpatch" regions in wait_event_names.txt Bertrand Drouvot <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox