public inbox for [email protected]
help / color / mirror / Atom feedRe: 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]>
2024-04-03 17:02 ` Re: Is it safe to cache data by GiST consistent function Tom Lane <[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 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 ` Re: Is it safe to cache data by GiST consistent function 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-03 16:47 Re: Is it safe to cache data by GiST consistent function Michał Kłeczek <[email protected]>
2024-04-03 17:02 ` Re: Is it safe to cache data by GiST consistent function Tom Lane <[email protected]>
@ 2024-04-04 03:20 ` Michał Kłeczek <[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