public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v37 03/11] Allow to prolong life span of transition tables until transaction end 7+ messages / 5 participants [nested] [flat]
* [PATCH v37 03/11] Allow to prolong life span of transition tables until transaction end @ 2019-12-20 01:09 Yugo Nagata <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Yugo Nagata @ 2019-12-20 01:09 UTC (permalink / raw) Originally, tuplestores of AFTER trigger's transition tables were freed for each query depth. For our IVM implementation, we would like to prolong life of the tuplestores because we have to preserve them for a whole query assuming that some base tables might be changed in some trigger functions. --- src/backend/commands/trigger.c | 80 ++++++++++++++++++++++++++++++++-- src/include/commands/trigger.h | 2 + 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index b87b4b40d07..49fe531198a 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -3819,6 +3819,10 @@ typedef struct AfterTriggerEventList * end of the list, so it is relatively easy to discard them. The event * list chunks themselves are stored in event_cxt. * + * prolonged_tuplestored is a list of transition table tuplestores whose + * life are prolonged to the end of the outmost query instead of each nested + * query. + * * query_depth is the current depth of nested AfterTriggerBeginQuery calls * (-1 when the stack is empty). * @@ -3884,6 +3888,7 @@ typedef struct AfterTriggersData SetConstraintState state; /* the active S C state */ AfterTriggerEventList events; /* deferred-event list */ MemoryContext event_cxt; /* memory context for events, if any */ + List *prolonged_tuplestores; /* list of prolonged tuplestores */ /* per-query-level data: */ AfterTriggersQueryData *query_stack; /* array of structs shown below */ @@ -3932,6 +3937,7 @@ struct AfterTriggersTableData bool closed; /* true when no longer OK to add tuples */ bool before_trig_done; /* did we already queue BS triggers? */ bool after_trig_done; /* did we already queue AS triggers? */ + bool prolonged; /* are transition tables prolonged? */ AfterTriggerEventList after_trig_events; /* if so, saved list pointer */ /* "old" transition table for UPDATE/DELETE, if any */ @@ -3978,6 +3984,7 @@ static void TransitionTableAddTuple(EState *estate, TupleTableSlot *original_insert_tuple, Tuplestorestate *tuplestore); static void AfterTriggerFreeQuery(AfterTriggersQueryData *qs); +static void release_or_prolong_tuplestore(Tuplestorestate *ts, bool prolonged); static SetConstraintState SetConstraintStateCreate(int numalloc); static SetConstraintState SetConstraintStateCopy(SetConstraintState origstate); static SetConstraintState SetConstraintStateAddItem(SetConstraintState state, @@ -4873,6 +4880,45 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events, } +/* + * SetTransitionTablePreserved + * + * Prolong lifespan of transition tables corresponding specified relid and + * command type to the end of the outmost query instead of each nested query. + * This enables to use nested AFTER trigger's transition tables from outer + * query's triggers. Currently, only immediate incremental view maintenance + * uses this. + */ +void +SetTransitionTablePreserved(Oid relid, CmdType cmdType) +{ + AfterTriggersTableData *table; + AfterTriggersQueryData *qs; + bool found = false; + ListCell *lc; + + /* Check state, like AfterTriggerSaveEvent. */ + if (afterTriggers.query_depth < 0) + elog(ERROR, "SetTransitionTablePreserved() called outside of query"); + + qs = &afterTriggers.query_stack[afterTriggers.query_depth]; + + foreach(lc, qs->tables) + { + table = (AfterTriggersTableData *) lfirst(lc); + if (table->relid == relid && table->cmdType == cmdType && + table->closed) + { + table->prolonged = true; + found = true; + } + } + + if (!found) + elog(ERROR,"could not find table with OID %d and command type %d", relid, cmdType); +} + + /* * GetAfterTriggersTableData * @@ -5113,6 +5159,7 @@ AfterTriggerBeginXact(void) afterTriggers.firing_depth = 0; afterTriggers.batch_callbacks = NIL; afterTriggers.firing_batch_callbacks = false; + afterTriggers.prolonged_tuplestores = NIL; /* * Verify that there is no leftover state remaining. If these assertions @@ -5287,11 +5334,11 @@ AfterTriggerFreeQuery(AfterTriggersQueryData *qs) ts = table->old_tuplestore; table->old_tuplestore = NULL; if (ts) - tuplestore_end(ts); + release_or_prolong_tuplestore(ts, table->prolonged); ts = table->new_tuplestore; table->new_tuplestore = NULL; if (ts) - tuplestore_end(ts); + release_or_prolong_tuplestore(ts, table->prolonged); if (table->storeslot) { TupleTableSlot *slot = table->storeslot; @@ -5309,8 +5356,33 @@ AfterTriggerFreeQuery(AfterTriggersQueryData *qs) qs->tables = NIL; list_free_deep(tables); - list_free_deep(qs->batch_callbacks); - qs->batch_callbacks = NIL; + /* Release prolonged tuplestores at the end of the outmost query */ + if (afterTriggers.query_depth == 0) + { + foreach(lc, afterTriggers.prolonged_tuplestores) + { + ts = (Tuplestorestate *) lfirst(lc); + if (ts) + tuplestore_end(ts); + } + afterTriggers.prolonged_tuplestores = NIL; + } +} + +/* + * Release the tuplestore, or append it to the prolonged tuplestores list. + */ +static void +release_or_prolong_tuplestore(Tuplestorestate *ts, bool prolonged) +{ + if (prolonged && afterTriggers.query_depth > 0) + { + MemoryContext oldcxt = MemoryContextSwitchTo(CurTransactionContext); + afterTriggers.prolonged_tuplestores = lappend(afterTriggers.prolonged_tuplestores, ts); + MemoryContextSwitchTo(oldcxt); + } + else + tuplestore_end(ts); } diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 1d9869973c0..e5dd915096d 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -269,6 +269,8 @@ extern void AfterTriggerEndSubXact(bool isCommit); extern void AfterTriggerSetState(ConstraintsSetStmt *stmt); extern bool AfterTriggerPendingOnRel(Oid relid); +extern void SetTransitionTablePreserved(Oid relid, CmdType cmdType); + /* * in utils/adt/ri_triggers.c -- 2.43.0 --Multipart=_Fri__29_May_2026_23_14_17_+0900_Te0o73X2VqYK57Gd Content-Type: text/x-diff; name="v37-0002-Add-relisivm-column-to-pg_class-system-catalog.patch" Content-Disposition: attachment; filename="v37-0002-Add-relisivm-column-to-pg_class-system-catalog.patch" Content-Transfer-Encoding: 7bit ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: confusing / inefficient "need_transcoding" handling in copy @ 2024-02-06 04:49 Michael Paquier <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Michael Paquier @ 2024-02-06 04:49 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Sutou Kouhei <[email protected]>; Tom Lane <[email protected]>; Tatsuo Ishii <[email protected]> On Mon, Feb 05, 2024 at 06:05:04PM -0800, Andres Freund wrote: > I don't really understand why we need to validate anything during COPY TO? > Which is good, because it turns out that we don't actually validate anything, > as pg_server_to_any() returns without doing anything if the encoding matches: > > if (encoding == DatabaseEncoding->encoding || > encoding == PG_SQL_ASCII) > return unconstify(char *, s); /* assume data is valid */ > > This means that the strlen() we do in the call do pg_server_to_any(), which on > its own takes 14.25% of the cycles, computes something that will never be > used. Indeed, that's wasting cycles for nothing when the client and server encoding match. > Unsurprisingly, only doing transcoding when encodings differ yields a sizable > improvement, about 18% for [2]. > > I haven't yet dug into the code history. One guess is that this should only > have been set this way for COPY FROM. Looking the git history, this looks like an oversight of c61a2f58418e that has added the condition on pg_database_encoding_max_length(), no? Adding Tom and Ishii-san, even if this comes from 2005. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: confusing / inefficient "need_transcoding" handling in copy @ 2024-02-06 17:51 Tom Lane <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Tom Lane @ 2024-02-06 17:51 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Sutou Kouhei <[email protected]>; Tatsuo Ishii <[email protected]> Michael Paquier <[email protected]> writes: > On Mon, Feb 05, 2024 at 06:05:04PM -0800, Andres Freund wrote: >> I haven't yet dug into the code history. One guess is that this should only >> have been set this way for COPY FROM. > Looking the git history, this looks like an oversight of c61a2f58418e > that has added the condition on pg_database_encoding_max_length(), no? > Adding Tom and Ishii-san, even if this comes from 2005. Yeah, back in 8.1 that code was applied for both directions, but probably it should have enforced validation for same-encoding cases only for COPY FROM. It looks like now we have a mess, because the condition was copied verbatim into copyto.c but not copyfrom.c. Aren't we failing to validate encoding in this case in COPY FROM, which is where we actually need to? regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: confusing / inefficient "need_transcoding" handling in copy @ 2024-02-06 22:24 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 2 replies; 7+ messages in thread From: Andres Freund @ 2024-02-06 22:24 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers; Sutou Kouhei <[email protected]>; Tatsuo Ishii <[email protected]> Hi, On 2024-02-06 12:51:48 -0500, Tom Lane wrote: > Michael Paquier <[email protected]> writes: > > On Mon, Feb 05, 2024 at 06:05:04PM -0800, Andres Freund wrote: > >> I haven't yet dug into the code history. One guess is that this should only > >> have been set this way for COPY FROM. > > > Looking the git history, this looks like an oversight of c61a2f58418e > > that has added the condition on pg_database_encoding_max_length(), no? > > Adding Tom and Ishii-san, even if this comes from 2005. > > Yeah, back in 8.1 that code was applied for both directions, but > probably it should have enforced validation for same-encoding > cases only for COPY FROM. > > It looks like now we have a mess, because the condition was copied > verbatim into copyto.c but not copyfrom.c. Aren't we failing to > validate encoding in this case in COPY FROM, which is where we > actually need to? I think the code is just very confusing - there actually *is* verification of the encoding, it just happens at a different, earlier, layer, namely in copyfromparse.c: CopyConvertBuf() which says: /* * If the file and server encoding are the same, no encoding conversion is * required. However, we still need to verify that the input is valid for * the encoding. */ And does indeed verify that. One unfortunate issue: We don't have any tests verifying that COPY FROM catches encoding issues. Regards, Andres ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: confusing / inefficient "need_transcoding" handling in copy @ 2024-02-08 07:05 Michael Paquier <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 0 replies; 7+ messages in thread From: Michael Paquier @ 2024-02-08 07:05 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers; Sutou Kouhei <[email protected]>; Tatsuo Ishii <[email protected]>; Heikki Linnakangas <[email protected]> On Tue, Feb 06, 2024 at 02:24:45PM -0800, Andres Freund wrote: > I think the code is just very confusing - there actually *is* verification of > the encoding, it just happens at a different, earlier, layer, namely in > copyfromparse.c: CopyConvertBuf() which says: > /* > * If the file and server encoding are the same, no encoding conversion is > * required. However, we still need to verify that the input is valid for > * the encoding. > */ > > And does indeed verify that. This has been switched by Heikki in f82de5c46bdf, in 2021, that has removed pg_database_encoding_max_length() in the COPY FROM case. (Adding him now in CC, in case he has any comments). > One unfortunate issue: We don't have any tests verifying that COPY FROM > catches encoding issues. Oops. Anyway, I was looking at the copyto.c code because I need to get something on this thread to be able to do something about the pluggable APIs in COPY, and echoing with what you mentioned upthread, what we only need to do is to set need_transcoding only when the client and the server encodings are not the same? Am I missing something? Runtime gets much better in average, around 1260ms on HEAD vs 1023ms with the attached for the example of upthread on a single process. Some profile data from CopyOneRowTo(), if relevant: * HEAD: - 82.78% 10.96% postgres postgres [.] CopyOneRowTo - 71.82% CopyOneRowTo + 30.87% OutputFunctionCall - 13.21% CopyAttributeOutText pg_server_to_any - 9.48% appendBinaryStringInfo 4.93% enlargeStringInfo 3.33% 0xffffa4e1e234 + 3.20% CopySendEndOfRow 2.66% 0xffffa4e1e214 1.02% pgstat_progress_update_param 0.86% memcpy@plt 0.74% 0xffffa4e1cba4 0.72% MemoryContextReset 0.72% 0xffffa4e1cba8 0.59% enlargeStringInfo 0.55% 0xffffa4e1cb40 0.54% 0xffffa4e1cb74 0.52% 0xffffa4e1cb8c + 10.96% _start * patch: - 80.82% 12.25% postgres postgres [.] CopyOneRowTo - 68.57% CopyOneRowTo + 36.55% OutputFunctionCall 11.44% CopyAttributeOutText + 8.87% appendBinaryStringInfo + 3.79% CopySendEndOfRow 1.01% pgstat_progress_update_param 0.79% int2out 0.66% MemoryContextReset 0.63% 0xffffaa624ba8 0.60% memcpy@plt 0.60% enlargeStringInfo 0.53% 0xffffaa624ba4 + 12.25% _start That's a performance-only change, but there may be a good argument for backpatching something, perhaps? -- Michael Attachments: [text/x-diff] 0001-Speedup-COPY-TO.patch (1.3K, ../../[email protected]/2-0001-Speedup-COPY-TO.patch) download | inline diff: From 6ddbcd4d6333bc96c21ca95d632d83f1e1459064 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 8 Feb 2024 16:03:39 +0900 Subject: [PATCH] Speedup COPY TO --- src/backend/commands/copyto.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index bd4710a79b..c6b45cab53 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -612,13 +612,15 @@ BeginCopyTo(ParseState *pstate, cstate->file_encoding = cstate->opts.file_encoding; /* - * Set up encoding conversion info. Even if the file and server encodings - * are the same, we must apply pg_any_to_server() to validate data in - * multibyte encodings. + * Set up encoding conversion info, validating data if server and + * client encodings are not the same (see also pg_server_to_any). */ - cstate->need_transcoding = - (cstate->file_encoding != GetDatabaseEncoding() || - pg_database_encoding_max_length() > 1); + if (cstate->file_encoding == GetDatabaseEncoding() || + cstate->file_encoding == PG_SQL_ASCII) + cstate->need_transcoding = false; + else + cstate->need_transcoding = true; + /* See Multibyte encoding comment above */ cstate->encoding_embeds_ascii = PG_ENCODING_IS_CLIENT_ONLY(cstate->file_encoding); -- 2.43.0 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: confusing / inefficient "need_transcoding" handling in copy @ 2024-02-08 08:25 Sutou Kouhei <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Sutou Kouhei @ 2024-02-08 08:25 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers; [email protected] Hi, In <[email protected]> "Re: confusing / inefficient "need_transcoding" handling in copy" on Tue, 6 Feb 2024 14:24:45 -0800, Andres Freund <[email protected]> wrote: > One unfortunate issue: We don't have any tests verifying that COPY FROM > catches encoding issues. How about the attached patch for it? How about the following to avoid needless transcoding? ---- diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c index bd4710a79b..80ec26cafe 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -612,13 +612,14 @@ BeginCopyTo(ParseState *pstate, cstate->file_encoding = cstate->opts.file_encoding; /* - * Set up encoding conversion info. Even if the file and server encodings - * are the same, we must apply pg_any_to_server() to validate data in - * multibyte encodings. + * Set up encoding conversion info. We use pg_server_to_any() for the + * conversion. pg_server_to_any() does nothing with an encoding that + * equals to GetDatabaseEncoding() and PG_SQL_ASCII. If + * cstate->file_encoding equals to GetDatabaseEncoding() and PG_SQL_ASCII, + * we don't need to transcode. */ - cstate->need_transcoding = - (cstate->file_encoding != GetDatabaseEncoding() || - pg_database_encoding_max_length() > 1); + cstate->need_transcoding = !(cstate->file_encoding == GetDatabaseEncoding() || + cstate->file_encoding == PG_SQL_ASCII); /* See Multibyte encoding comment above */ cstate->encoding_embeds_ascii = PG_ENCODING_IS_CLIENT_ONLY(cstate->file_encoding); ---- Numbers on my environment: master: 861.646ms patched: 697.547ms SQL: COPY (SELECT 1::int2,2::int2,3::int2,4::int2,5::int2,6::int2,7::int2,8::int2,9::int2,10::int2,11::int2,12::int2,13::int2,14::int2,15::int2,16::int2,17::int2,18::int2,19::int2,20::int2, generate_series(1, 1000000::int4)) TO '/dev/null' \watch c=5 Thanks, -- kou Attachments: [text/x-patch] v1-0001-Add-a-test-for-invalid-encoding-for-COPY-FROM.patch (2.1K, ../../[email protected]/2-v1-0001-Add-a-test-for-invalid-encoding-for-COPY-FROM.patch) download | inline diff: From 387f11bde4eb928af23f6a66101aa9d63b3dcfdf Mon Sep 17 00:00:00 2001 From: Sutou Kouhei <[email protected]> Date: Thu, 8 Feb 2024 17:17:25 +0900 Subject: [PATCH v1] Add a test for invalid encoding for COPY FROM The test data uses UTF-8 "hello" in Japanese but the test specifies EUC_JP. So it's an invalid data. --- src/test/regress/expected/copyencoding.out | 8 ++++++++ src/test/regress/parallel_schedule | 2 +- src/test/regress/sql/copyencoding.sql | 10 ++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 src/test/regress/expected/copyencoding.out create mode 100644 src/test/regress/sql/copyencoding.sql diff --git a/src/test/regress/expected/copyencoding.out b/src/test/regress/expected/copyencoding.out new file mode 100644 index 0000000000..aa4ecea09e --- /dev/null +++ b/src/test/regress/expected/copyencoding.out @@ -0,0 +1,8 @@ +-- +-- Test cases for COPY WITH (ENCODING) +-- +CREATE TABLE test (t text); +COPY test FROM stdin WITH (ENCODING 'EUC_JP'); +ERROR: invalid byte sequence for encoding "EUC_JP": 0xe3 0x81 +CONTEXT: COPY test, line 1 +DROP TABLE test; diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule index 1d8a414eea..238cef28c4 100644 --- a/src/test/regress/parallel_schedule +++ b/src/test/regress/parallel_schedule @@ -36,7 +36,7 @@ test: geometry horology tstypes regex type_sanity opr_sanity misc_sanity comment # execute two copy tests in parallel, to check that copy itself # is concurrent safe. # ---------- -test: copy copyselect copydml insert insert_conflict +test: copy copyselect copydml copyencoding insert insert_conflict # ---------- # More groups of parallel tests diff --git a/src/test/regress/sql/copyencoding.sql b/src/test/regress/sql/copyencoding.sql new file mode 100644 index 0000000000..07c85e988b --- /dev/null +++ b/src/test/regress/sql/copyencoding.sql @@ -0,0 +1,10 @@ +-- +-- Test cases for COPY WITH (ENCODING) +-- + +CREATE TABLE test (t text); +COPY test FROM stdin WITH (ENCODING 'EUC_JP'); +こんにちは +\. + +DROP TABLE test; -- 2.43.0 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: confusing / inefficient "need_transcoding" handling in copy @ 2024-02-13 21:56 Michael Paquier <[email protected]> parent: Sutou Kouhei <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Michael Paquier @ 2024-02-13 21:56 UTC (permalink / raw) To: Sutou Kouhei <[email protected]>; +Cc: [email protected]; [email protected]; pgsql-hackers; [email protected] On Thu, Feb 08, 2024 at 05:25:01PM +0900, Sutou Kouhei wrote: > In <[email protected]> > "Re: confusing / inefficient "need_transcoding" handling in copy" on Tue, 6 Feb 2024 14:24:45 -0800, > Andres Freund <[email protected]> wrote: > >> One unfortunate issue: We don't have any tests verifying that COPY FROM >> catches encoding issues. > > How about the attached patch for it? > > +CREATE TABLE test (t text); > +COPY test FROM stdin WITH (ENCODING 'EUC_JP'); > +こんにちは > +\. > + > +DROP TABLE test; We have a couple of non-ASCII characters in the tests, but I suspect that this one will not be digested correctly everywhere, even if EUC_JP should be OK to use for the check. How about writing an arbitrary sequence of bytes into a temporary file that gets used for the COPY FROM instead? See for example how we do that with abs_builddir in copy.sql. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-02-13 21:56 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-12-20 01:09 [PATCH v37 03/11] Allow to prolong life span of transition tables until transaction end Yugo Nagata <[email protected]> 2024-02-06 04:49 Re: confusing / inefficient "need_transcoding" handling in copy Michael Paquier <[email protected]> 2024-02-06 17:51 ` Re: confusing / inefficient "need_transcoding" handling in copy Tom Lane <[email protected]> 2024-02-06 22:24 ` Re: confusing / inefficient "need_transcoding" handling in copy Andres Freund <[email protected]> 2024-02-08 07:05 ` Re: confusing / inefficient "need_transcoding" handling in copy Michael Paquier <[email protected]> 2024-02-08 08:25 ` Re: confusing / inefficient "need_transcoding" handling in copy Sutou Kouhei <[email protected]> 2024-02-13 21:56 ` Re: confusing / inefficient "need_transcoding" handling in copy Michael Paquier <[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