public inbox for [email protected]help / color / mirror / Atom feed
Re: Is it safe to cache data by GiST consistent function 4+ messages / 3 participants [nested] [flat]
* Re: Is it safe to cache data by GiST consistent function @ 2024-04-03 16:47 Michał Kłeczek <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Michał Kłeczek @ 2024-04-03 16:47 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Thanks for taking your time to answer. Not sure if I understand though. > On 3 Apr 2024, at 16:27, Tom Lane <[email protected]> wrote: > > =?utf-8?Q?Micha=C5=82_K=C5=82eczek?= <[email protected]> writes: >> When implementing a GiST consistent function I found the need to cache pre-processed query across invocations. >> I am not sure if it is safe to do (or I need to perform some steps to make sure cached info is not leaked between rescans). > > AFAIK it works. I don't see any of the in-core ones doing so, > but at least range_gist_consistent and multirange_gist_consistent > are missing a bet by repeating their cache search every time. pg_trgm consistent caches tigrams but it has some logic to make sure cached values are recalculated: cache = (gtrgm_consistent_cache *) fcinfo->flinfo->fn_extra; if (cache == NULL || cache->strategy != strategy || VARSIZE(cache->query) != querysize || memcmp((char *) cache->query, (char *) query, querysize) != 0) What I don’t understand is if it is necessary or it is enough to check fn_extra==NULL. > >> The comment in gistrescan says: > >> /* >> * If this isn't the first time through, preserve the fn_extra >> * pointers, so that if the consistentFns are using them to cache >> * data, that data is not leaked across a rescan. >> */ > >> which seems to me self-contradictory as fn_extra is preserved between rescans (so leaks are indeed possible). > > I think you're reading it wrong. If we cleared fn_extra during > rescan, access to the old extra value would be lost so a new one > would have to be created, leaking the old value for the rest of > the query. I understand that but not sure what “that data is not leaked across a rescan” means. — Michal ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Is it safe to cache data by GiST consistent function @ 2024-04-03 17:02 Tom Lane <[email protected]> parent: Michał Kłeczek <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Tom Lane @ 2024-04-03 17:02 UTC (permalink / raw) To: Michał Kłeczek <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> =?utf-8?Q?Micha=C5=82_K=C5=82eczek?= <[email protected]> writes: > On 3 Apr 2024, at 16:27, Tom Lane <[email protected]> wrote: >> AFAIK it works. I don't see any of the in-core ones doing so, >> but at least range_gist_consistent and multirange_gist_consistent >> are missing a bet by repeating their cache search every time. > pg_trgm consistent caches tigrams but it has some logic to make sure cached values are recalculated: > cache = (gtrgm_consistent_cache *) fcinfo->flinfo->fn_extra; > if (cache == NULL || > cache->strategy != strategy || > VARSIZE(cache->query) != querysize || > memcmp((char *) cache->query, (char *) query, querysize) != 0) > What I don’t understand is if it is necessary or it is enough to check fn_extra==NULL. Ah, I didn't think to search contrib. Yes, you need to validate the cache entry. In this example, a rescan could insert a new query value. In general, an opclass support function could get called using a pretty long-lived FunctionCallInfo (e.g. one in the index's relcache entry), so it's unwise to assume that cached data is relevant to the current call without checking. regards, tom lane ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Is it safe to cache data by GiST consistent function @ 2024-04-04 03:20 Michał Kłeczek <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Michał Kłeczek @ 2024-04-04 03:20 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> > On 3 Apr 2024, at 19:02, Tom Lane <[email protected]> wrote: > > =?utf-8?Q?Micha=C5=82_K=C5=82eczek?= <[email protected]> writes: > >> pg_trgm consistent caches tigrams but it has some logic to make sure cached values are recalculated: > >> cache = (gtrgm_consistent_cache *) fcinfo->flinfo->fn_extra; >> if (cache == NULL || >> cache->strategy != strategy || >> VARSIZE(cache->query) != querysize || >> memcmp((char *) cache->query, (char *) query, querysize) != 0) > >> What I don’t understand is if it is necessary or it is enough to check fn_extra==NULL. > > Ah, I didn't think to search contrib. Yes, you need to validate the > cache entry. In this example, a rescan could insert a new query > value. In general, an opclass support function could get called using > a pretty long-lived FunctionCallInfo (e.g. one in the index's relcache > entry), so it's unwise to assume that cached data is relevant to the > current call without checking. This actually sounds scary - looks like there is no way to perform cache clean-up after rescan then? Do you think it might be useful to introduce a way for per-rescan caching (ie. setting up a dedicated memory context in gistrescan and passing it to support functions)? — Michal ^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH v19 3/8] Row pattern recognition patch (rewriter). @ 2024-05-14 23:26 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Tatsuo Ishii @ 2024-05-14 23:26 UTC (permalink / raw) --- src/backend/utils/adt/ruleutils.c | 103 ++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 9a6d372414..f4dc60a7d2 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -426,6 +426,10 @@ static void get_rule_groupingset(GroupingSet *gset, List *targetlist, bool omit_parens, deparse_context *context); static void get_rule_orderby(List *orderList, List *targetList, bool force_colno, deparse_context *context); +static void get_rule_pattern(List *patternVariable, List *patternRegexp, + bool force_colno, deparse_context *context); +static void get_rule_define(List *defineClause, List *patternVariables, + bool force_colno, deparse_context *context); static void get_rule_windowclause(Query *query, deparse_context *context); static void get_rule_windowspec(WindowClause *wc, List *targetList, deparse_context *context); @@ -6465,6 +6469,67 @@ get_rule_orderby(List *orderList, List *targetList, } } +/* + * Display a PATTERN clause. + */ +static void +get_rule_pattern(List *patternVariable, List *patternRegexp, + bool force_colno, deparse_context *context) +{ + StringInfo buf = context->buf; + const char *sep; + ListCell *lc_var, + *lc_reg = list_head(patternRegexp); + + sep = ""; + appendStringInfoChar(buf, '('); + foreach(lc_var, patternVariable) + { + char *variable = strVal((String *) lfirst(lc_var)); + char *regexp = NULL; + + if (lc_reg != NULL) + { + regexp = strVal((String *) lfirst(lc_reg)); + + lc_reg = lnext(patternRegexp, lc_reg); + } + + appendStringInfo(buf, "%s%s", sep, variable); + if (regexp !=NULL) + appendStringInfoString(buf, regexp); + + sep = " "; + } + appendStringInfoChar(buf, ')'); +} + +/* + * Display a DEFINE clause. + */ +static void +get_rule_define(List *defineClause, List *patternVariables, + bool force_colno, deparse_context *context) +{ + StringInfo buf = context->buf; + const char *sep; + ListCell *lc_var, + *lc_def; + + sep = " "; + Assert(list_length(patternVariables) == list_length(defineClause)); + + forboth(lc_var, patternVariables, lc_def, defineClause) + { + char *varName = strVal(lfirst(lc_var)); + TargetEntry *te = (TargetEntry *) lfirst(lc_def); + + appendStringInfo(buf, "%s%s AS ", sep, varName); + get_rule_expr((Node *) te->expr, context, false); + sep = ",\n "; + } +} + /* * Display a WINDOW clause. * @@ -6602,6 +6667,44 @@ get_rule_windowspec(WindowClause *wc, List *targetList, appendStringInfoString(buf, "EXCLUDE GROUP "); else if (wc->frameOptions & FRAMEOPTION_EXCLUDE_TIES) appendStringInfoString(buf, "EXCLUDE TIES "); + /* RPR */ + if (wc->rpSkipTo == ST_NEXT_ROW) + appendStringInfoString(buf, + "\n AFTER MATCH SKIP TO NEXT ROW "); + else if (wc->rpSkipTo == ST_PAST_LAST_ROW) + appendStringInfoString(buf, + "\n AFTER MATCH SKIP PAST LAST ROW "); + else if (wc->rpSkipTo == ST_FIRST_VARIABLE) + appendStringInfo(buf, + "\n AFTER MATCH SKIP TO FIRST %s ", + wc->rpSkipVariable); + else if (wc->rpSkipTo == ST_LAST_VARIABLE) + appendStringInfo(buf, + "\n AFTER MATCH SKIP TO LAST %s ", + wc->rpSkipVariable); + else if (wc->rpSkipTo == ST_VARIABLE) + appendStringInfo(buf, + "\n AFTER MATCH SKIP TO %s ", + wc->rpSkipVariable); + + if (wc->initial) + appendStringInfoString(buf, "\n INITIAL"); + + if (wc->patternVariable) + { + appendStringInfoString(buf, "\n PATTERN "); + get_rule_pattern(wc->patternVariable, wc->patternRegexp, + false, context); + } + + if (wc->defineClause) + { + appendStringInfoString(buf, "\n DEFINE\n"); + get_rule_define(wc->defineClause, wc->patternVariable, + false, context); + appendStringInfoChar(buf, ' '); + } + /* we will now have a trailing space; remove it */ buf->len--; } -- 2.25.1 ----Next_Part(Wed_May_15_09_02_03_2024_008)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v19-0004-Row-pattern-recognition-patch-planner.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-05-14 23:26 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-04-03 16:47 Re: Is it safe to cache data by GiST consistent function Michał Kłeczek <[email protected]> 2024-04-03 17:02 ` Tom Lane <[email protected]> 2024-04-04 03:20 ` Michał Kłeczek <[email protected]> 2024-05-14 23:26 [PATCH v19 3/8] Row pattern recognition patch (rewriter). Tatsuo Ishii <[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