Multiple Query IDs for a rewritten parse tree
On 5/1/2022 10:13, Tom Lane wrote: > I feel like we need to get away from the idea that there is just > one query hash, and somehow let different extensions attach > differently-calculated hashes to a query. I don't have any immediate > ideas about how to do that in a reasonably inexpensive way. Now, queryId field represents an query class (depending on an jumbling implementation). It is used by extensions as the way for simple tracking a query from a parse tree creation point to the end of its life along all hook calls, which an extension uses (remember about possible plan caching). I know at least two different cases of using queryId: 1) for monitoring purposes - pg_stat_statements is watching how often queries of a class emerge in the database and collects a stat on each class. 2) adaptive purposes - some extensions influence a planner decision during the optimization stage and want to learn on a performance shift at the end of execution stage. Different purposes may require different jumbling implementations. But users can want to use such extensions at the same time. So we should allow to attach many different query IDs to a query (maybe better to call it as 'query label'?). Thinking for a while I invented three different ways to implement it: 1. queryId will be a trivial 64-bit counter. So, each extension can differ each query from any other, track it along all hooks, use an jumbling code and store an queryId internally. Here only one big problem I see - increasing overhead in the case of many consumers of queryId feature. 2. Instead of simple queryId we can store a list of pairs (QueryId, funcOid). An extension can register a callback for queryId generation and the core will form a list of queryIds right after an query tree rewriting. funcOid is needed to differ jumbling implementations. Here we should invent an additional node type for an element of the list. 3. Instead of queryId we could add a multi-purpose 'private' list in the Query struct. Any extension can add to this list additional object(s) (with registered object type, of course). As an example, i can imagine a kind of convention for queryIds in such case - store a String node with value: ' - '. This way we should implement a registered callback mechanism too. I think, third way is the cheapest, flexible and simple for implementation. Any thoughts, comments, criticism ? -- regards, Andrey Lepikhov Postgres Professional
Re: Multiple Query IDs for a rewritten parse tree
Andrey Lepikhov writes: > On 5/1/2022 10:13, Tom Lane wrote: >>> I feel like we need to get away from the idea that there is just >>> one query hash, and somehow let different extensions attach >>> differently-calculated hashes to a query. I don't have any immediate >>> ideas about how to do that in a reasonably inexpensive way. > Thinking for a while I invented three different ways to implement it: > 1. queryId will be a trivial 64-bit counter. This seems pretty useless. The whole point of the query hash, at least for many use-cases, is to allow recognizing queries that are the same or similar. > 2. Instead of simple queryId we can store a list of pairs (QueryId, > funcOid). An extension can register a callback for queryId generation > and the core will form a list of queryIds right after an query tree > rewriting. funcOid is needed to differ jumbling implementations. Here we > should invent an additional node type for an element of the list. I'm not sure that funcOid is a reasonable way to tag different hash calculation methods, because it isn't necessarily stable across installations. For the same reason, it'd be hard for two extensions to collaborate on a common query-hash definition. > 3. Instead of queryId we could add a multi-purpose 'private' list in the > Query struct. Any extension can add to this list additional object(s) > (with registered object type, of course). As an example, i can imagine a > kind of convention for queryIds in such case - store a String node with > value: ' - '. Again, this is presuming that every extension is totally independent and has no interest in what any other code is doing. But I don't think we want to make every extension that wants a hash duplicate the whole of queryjumble.c. The idea I'd been vaguely thinking about is to allow attaching a list of query-hash nodes to a Query, where each node would contain a "tag" identifying the specific hash calculation method, and also the value of the query's hash calculated according to that method. We could probably get away with saying that all such hash values must be uint64. The main difference from your function-OID idea, I think, is that I'm envisioning the tags as being small integers with well-known values, similarly to the way we manage stakind values in pg_statistic. In this way, an extension that wants a hash that the core knows how to calculate doesn't need its own copy of the code, and similarly one extension could publish a calculation method for use by other extensions. We'd also need some mechanism for registering a function to be used to calculate the hash for any given tag value, of course. regards, tom lane
Re: Multiple Query IDs for a rewritten parse tree
> On Sat, Jan 08, 2022 at 07:49:59PM -0500, Tom Lane wrote: > > The idea I'd been vaguely thinking about is to allow attaching a list > of query-hash nodes to a Query, where each node would contain a "tag" > identifying the specific hash calculation method, and also the value > of the query's hash calculated according to that method. We could > probably get away with saying that all such hash values must be uint64. > The main difference from your function-OID idea, I think, is that > I'm envisioning the tags as being small integers with well-known > values, similarly to the way we manage stakind values in pg_statistic. > In this way, an extension that wants a hash that the core knows how > to calculate doesn't need its own copy of the code, and similarly > one extension could publish a calculation method for use by other > extensions. An extension that wants a slightly modified version of hash calculation implementation from the core would still need to copy everything. The core probably has to provide more than one (hash, method) pair to cover some basic needs.
Re: Multiple Query IDs for a rewritten parse tree
On Sun, Jan 09, 2022 at 12:43:06PM +0100, Dmitry Dolgov wrote: > > An extension that wants a slightly modified version of hash calculation > implementation from the core would still need to copy everything. The > core probably has to provide more than one (hash, method) pair to cover > some basic needs. Or just GUC(s) to adapt the behavior. But in any case there isn't much that can be done that won't result in a huge performance drop (like e.g. the wanted stability over logical replication or backup/restore).
Re: Multiple Query IDs for a rewritten parse tree
On Sat, Jan 08, 2022 at 07:49:59PM -0500, Tom Lane wrote: > > The idea I'd been vaguely thinking about is to allow attaching a list > of query-hash nodes to a Query, where each node would contain a "tag" > identifying the specific hash calculation method, and also the value > of the query's hash calculated according to that method. For now the queryid mixes two different things: fingerprinting and query text normalization. Should each calculation method be allowed to do a different normalization too, and if yes where should be stored the state data needed for that? If not, we would need some kind of primary hash for that purpose. Looking at Andrey's use case for wanting multiple hashes, I don't think that adaptive optimization needs a normalized query string. The only use would be to output some statistics, but this could be achieved by storing a list of "primary queryid" for each adaptive entry. That's probably also true for anything that's not monitoring intended. Also, all monitoring consumers should probably agree on the same queryid, both fingerprint and normalized string, as otherwise it's impossible to cross-reference metric data.
Re: Multiple Query IDs for a rewritten parse tree
On 1/9/22 5:13 PM, Julien Rouhaud wrote: For now the queryid mixes two different things: fingerprinting and query text normalization. Should each calculation method be allowed to do a different normalization too, and if yes where should be stored the state data needed for that? If not, we would need some kind of primary hash for that purpose. Do You mean JumbleState? I think, registering queryId generator we should store also a pointer (void **args) to an additional data entry, as usual. Looking at Andrey's use case for wanting multiple hashes, I don't think that adaptive optimization needs a normalized query string. The only use would be to output some statistics, but this could be achieved by storing a list of "primary queryid" for each adaptive entry. That's probably also true for anything that's not monitoring intended. Also, all monitoring consumers should probably agree on the same queryid, both fingerprint and normalized string, as otherwise it's impossible to cross-reference metric data. I can add one more use case. Our extension for freezing query plan uses query tree comparison technique to prove, that the plan can be applied (and we don't need to execute planning procedure at all). The procedure of a tree equality checking is expensive and we use cheaper queryId comparison to identify possible candidates. So here, for the better performance and queries coverage, we need to use query tree normalization - queryId should be stable to some modifications in a query text which do not change semantics. As an example, query plan with external parameters can be used to execute constant query if these constants correspond by place and type to the parameters. So, queryId calculation technique returns also pointers to all constants and parameters found during the calculation. -- regards, Andrey Lepikhov Postgres Professional
Re: Multiple Query IDs for a rewritten parse tree
On Mon, Jan 10, 2022 at 09:10:59AM +0500, Andrey V. Lepikhov wrote: > On 1/9/22 5:13 PM, Julien Rouhaud wrote: > > For now the queryid mixes two different things: fingerprinting and query > > text > > normalization. Should each calculation method be allowed to do a different > > normalization too, and if yes where should be stored the state data needed > > for > > that? If not, we would need some kind of primary hash for that purpose. > > > Do You mean JumbleState? Yes, or some other abstraction. For instance with the proposed patch to handle differently the IN clauses, you needs a to change JumbleState. > I think, registering queryId generator we should store also a pointer (void > **args) to an additional data entry, as usual. Well, yes but only if you need to produce different versions of the normalized query text, and I'm not convinced that's really necessary. > > Looking at Andrey's use case for wanting multiple hashes, I don't think that > > adaptive optimization needs a normalized query string. The only use would > > be > > to output some statistics, but this could be achieved by storing a list of > > "primary queryid" for each adaptive entry. That's probably also true for > > anything that's not monitoring intended. Also, all monitoring consumers > > should > > probably agree on the same queryid, both fingerprint and normalized string, > > as > > otherwise it's impossible to cross-reference metric data. > > > I can add one more use case. > Our extension for freezing query plan uses query tree comparison technique > to prove, that the plan can be applied (and we don't need to execute > planning procedure at all). > The procedure of a tree equality checking is expensive and we use cheaper > queryId comparison to identify possible candidates. So here, for the better > performance and queries coverage, we need to use query tree normalization - > queryId should be stable to some modifications in a query text which do not > change semantics. > As an example, query plan with external parameters can be used to execute > constant query if these constants correspond by place and type to the > parameters. So, queryId calculation technique returns also pointers to all > constants and parameters found during the calculation. I'm also working on a similar extension, and yes you can't accept any fingerprinting approach for that. I don't know what are the exact heuristics of your cheaper queryid calculation are, but is it reasonable to use it with something like pg_stat_statements? If yes, you don't really need two queryid approach for the sake of this single extension and therefore don't need to store multiple jumble state or similar per statement. Especially since requiring another one would mean a performance drop as soon as you want to use something as common as pg_stat_statements.
Re: Multiple Query IDs for a rewritten parse tree
On 1/10/22 9:51 AM, Julien Rouhaud wrote: On Mon, Jan 10, 2022 at 09:10:59AM +0500, Andrey V. Lepikhov wrote: I can add one more use case. Our extension for freezing query plan uses query tree comparison technique to prove, that the plan can be applied (and we don't need to execute planning procedure at all). The procedure of a tree equality checking is expensive and we use cheaper queryId comparison to identify possible candidates. So here, for the better performance and queries coverage, we need to use query tree normalization - queryId should be stable to some modifications in a query text which do not change semantics. As an example, query plan with external parameters can be used to execute constant query if these constants correspond by place and type to the parameters. So, queryId calculation technique returns also pointers to all constants and parameters found during the calculation. I'm also working on a similar extension, and yes you can't accept any fingerprinting approach for that. I don't know what are the exact heuristics of your cheaper queryid calculation are, but is it reasonable to use it with something like pg_stat_statements? If yes, you don't really need two queryid approach for the sake of this single extension and therefore don't need to store multiple jumble state or similar per statement. Especially since requiring another one would mean a performance drop as soon as you want to use something as common as pg_stat_statements. I think, pg_stat_statements can live with an queryId generator of the sr_plan extension. But It replaces all constants with $XXX parameter at the query string. In our extension user defines which plan is optimal and which constants can be used as parameters in the plan. One drawback I see here - creating or dropping of my extension changes behavior of pg_stat_statements that leads to distortion of the DB load profile. Also, we haven't guarantees, that another extension will work correctly (or in optimal way) with such queryId. -- regards, Andrey Lepikhov Postgres Professional
Re: Multiple Query IDs for a rewritten parse tree
On Mon, Jan 10, 2022 at 12:37:34PM +0500, Andrey V. Lepikhov wrote: > I think, pg_stat_statements can live with an queryId generator of the > sr_plan extension. But It replaces all constants with $XXX parameter at the > query string. In our extension user defines which plan is optimal and which > constants can be used as parameters in the plan. I don't know the details of that extension, but I think that it should work as long as you have the constants information in the jumble state, whatever the resulting normalized query string is right? > One drawback I see here - creating or dropping of my extension changes > behavior of pg_stat_statements that leads to distortion of the DB load > profile. Also, we haven't guarantees, that another extension will work > correctly (or in optimal way) with such queryId. But then, if generating 2 queryid is a better for performance, does the extension really need an additional jumble state and/or normalized query string?
Re: Multiple Query IDs for a rewritten parse tree
On 10/1/2022 13:56, Julien Rouhaud wrote: On Mon, Jan 10, 2022 at 12:37:34PM +0500, Andrey V. Lepikhov wrote: I think, pg_stat_statements can live with an queryId generator of the sr_plan extension. But It replaces all constants with $XXX parameter at the query string. In our extension user defines which plan is optimal and which constants can be used as parameters in the plan. I don't know the details of that extension, but I think that it should work as long as you have the constants information in the jumble state, whatever the resulting normalized query string is right? Yes. the same input query string doesn't prove that frozen query plan can be used, because rewrite rules could be changed. So we use only a query tree. Here we must have custom jumbling implementation. queryId in this extension defines two aspects: 1. How many input queries will be compared with a query tree template of the frozen statement. 2. As a result, performance overheads on unsuccessful comparings. One drawback I see here - creating or dropping of my extension changes behavior of pg_stat_statements that leads to distortion of the DB load profile. Also, we haven't guarantees, that another extension will work correctly (or in optimal way) with such queryId. But then, if generating 2 queryid is a better for performance, does the extension really need an additional jumble state and/or normalized query string? Additional Jumble state isn't necessary, but it would be better for performance to collect pointers to all constant nodes during a process of hash generation. -- regards, Andrey Lepikhov Postgres Professional
Re: Multiple Query IDs for a rewritten parse tree
On Mon, Jan 10, 2022 at 03:24:46PM +0500, Andrey Lepikhov wrote: > On 10/1/2022 13:56, Julien Rouhaud wrote: > > > > I don't know the details of that extension, but I think that it should work > > as > > long as you have the constants information in the jumble state, whatever the > > resulting normalized query string is right? > Yes. the same input query string doesn't prove that frozen query plan can be > used, because rewrite rules could be changed. So we use only a query tree. Yes, I'm fully aware of that. I wasn't asking about using the input query string but the need for generating a dedicated normalized output query string that would be potentially different from the one generated by pg_stat_statements (or similar). > > But then, if generating 2 queryid is a better for performance, does the > > extension really need an additional jumble state and/or normalized query > > string? > Additional Jumble state isn't necessary, but it would be better for > performance to collect pointers to all constant nodes during a process of > hash generation. I agree, but it's even better for performance if this is collected only once. I still don't know if this extension (or any extension) would require something different from a common jumble state that would serve for all purpose or if we can work with a single jumble state and only handle multiple queryid.
Re: Multiple Query IDs for a rewritten parse tree
On 10/1/2022 15:39, Julien Rouhaud wrote: On Mon, Jan 10, 2022 at 03:24:46PM +0500, Andrey Lepikhov wrote: On 10/1/2022 13:56, Julien Rouhaud wrote: Yes. the same input query string doesn't prove that frozen query plan can be used, because rewrite rules could be changed. So we use only a query tree. Yes, I'm fully aware of that. I wasn't asking about using the input query string but the need for generating a dedicated normalized output query string that would be potentially different from the one generated by pg_stat_statements (or similar). Thanks, now I got it. I don't remember a single situation where we would need to normalize a query string. But then, if generating 2 queryid is a better for performance, does the extension really need an additional jumble state and/or normalized query string? Additional Jumble state isn't necessary, but it would be better for performance to collect pointers to all constant nodes during a process of hash generation. I agree, but it's even better for performance if this is collected only once. I still don't know if this extension (or any extension) would require something different from a common jumble state that would serve for all purpose or if we can work with a single jumble state and only handle multiple queryid. I think, JumbleState in highly monitoring-specific, maybe even pg_stat_statements specific (maybe you can designate another examples). I haven't ideas how it can be used in arbitrary extension. But, it is not so difficult to imagine an implementation (as part of Tom's approach, described earlier) such kind of storage for each generation method. -- regards, Andrey Lepikhov Postgres Professional
Re: Multiple Query IDs for a rewritten parse tree
On 1/9/22 5:49 AM, Tom Lane wrote: The idea I'd been vaguely thinking about is to allow attaching a list of query-hash nodes to a Query, where each node would contain a "tag" identifying the specific hash calculation method, and also the value of the query's hash calculated according to that method. We could probably get away with saying that all such hash values must be uint64. The main difference from your function-OID idea, I think, is that I'm envisioning the tags as being small integers with well-known values, similarly to the way we manage stakind values in pg_statistic. In this way, an extension that wants a hash that the core knows how to calculate doesn't need its own copy of the code, and similarly one extension could publish a calculation method for use by other extensions. To move forward, I have made a patch that implements this idea (see attachment). It is a POC, but passes all regression tests. Registration of an queryId generator implemented by analogy with extensible methods machinery. Also, I switched queryId to int64 type and renamed to 'label'. Some lessons learned: 1. Single queryId implementation is deeply tangled with the core code (stat reporting machinery and parallel workers as an example). 2. We need a custom queryId, that is based on a generated queryId (according to the logic of pg_stat_statements). 3. We should think about safety of de-registering procedure. 4. We should reserve position of default in-core generator and think on logic of enabling/disabling it. 5. We should add an EXPLAIN hook, to allow an extension to print this custom queryId. -- regards, Andrey Lepikhov Postgres Professional >From f54bb60bd71b49ac8e1b85cd2ad86332a8a81e84 Mon Sep 17 00:00:00 2001 From: Andrey Lepikhov Date: Thu, 20 Jan 2022 16:05:17 +0500 Subject: [PATCH] Initial commit --- .../pg_stat_statements/pg_stat_statements.c | 57 -- src/backend/commands/explain.c| 11 +- src/backend/executor/execMain.c | 4 +- src/backend/executor/execParallel.c | 3 +- src/backend/nodes/copyfuncs.c | 21 ++- src/backend/nodes/outfuncs.c | 17 +- src/backend/nodes/readfuncs.c | 17 +- src/backend/optimizer/plan/planner.c | 2 +- src/backend/parser/analyze.c | 15 +- src/backend/rewrite/rewriteHandler.c | 4 +- src/backend/tcop/postgres.c | 11 +- src/backend/utils/misc/guc.c | 2 +- src/backend/utils/misc/queryjumble.c | 164 -- src/include/nodes/nodes.h | 1 + src/include/nodes/parsenodes.h| 10 +- src/include/nodes/plannodes.h | 2 +- src/include/parser/analyze.h | 3 +- src/include/utils/queryjumble.h | 12 +- 18 files changed, 294 insertions(+), 62 deletions(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 082bfa8f77..ebfc3331df 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -307,8 +307,7 @@ PG_FUNCTION_INFO_V1(pg_stat_statements_info); static void pgss_shmem_startup(void); static void pgss_shmem_shutdown(int code, Datum arg); -static void pgss_post_parse_analyze(ParseState *pstate, Query *query, - JumbleState *jstate); +static void pgss_post_parse_analyze(ParseState *pstate, Query *query); static PlannedStmt *pgss_planner(Query *parse, const char *query_string, int cursorOptions, @@ -813,13 +812,29 @@ error: } /* - * Post-parse-analysis hook: mark query with a queryId + * Post-parse-analysis hook: create a label for the query. */ static void -pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate) +pgss_post_parse_analyze(ParseState *pstate, Query *query) { + JumbleState *jstate; + QueryLabel *label = get_query_label(query->queryIds, 0); + if (prev_post_parse_analyze_hook) - prev_post_parse_analyze_hook(pstate, query, jstate); + prev_post_parse_analyze_hook(pstate, query); + + if (label) + { + add_custom_query_label(&query->queryIds, -1, label->hash); + jstate = (JumbleState *) label->context; + } + else + { + add_custom_query_label(&query->queryIds, -1, UINT64CONST(0)); + jstate = NULL; + } + + label = get_query_label(query->queryIds, -1); /* Safety check... */ if (!pgss || !pgss_hash || !pgss_enabled(exec_nested_level)) @@ -833,7 +848,7 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate) if (query->utilityStmt) { if (pgss_track_utility && !PGSS_HANDLED_UTILITY(query->utilityStmt)) - query->queryId = UINT64CONST(0); + label->hash = UINT64CONST(0); return; } @@ -846,7 +861,7 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate) */ if (jstate && jstate->clocations_count > 0) pgss_store(pstate->p
Re: Multiple Query IDs for a rewritten parse tree
> On Fri, Jan 21, 2022 at 11:33:22AM +0500, Andrey V. Lepikhov wrote: > On 1/9/22 5:49 AM, Tom Lane wrote: > > The idea I'd been vaguely thinking about is to allow attaching a list > > of query-hash nodes to a Query, where each node would contain a "tag" > > identifying the specific hash calculation method, and also the value > > of the query's hash calculated according to that method. We could > > probably get away with saying that all such hash values must be uint64. > > The main difference from your function-OID idea, I think, is that > > I'm envisioning the tags as being small integers with well-known > > values, similarly to the way we manage stakind values in pg_statistic. > > In this way, an extension that wants a hash that the core knows how > > to calculate doesn't need its own copy of the code, and similarly > > one extension could publish a calculation method for use by other > > extensions. > > To move forward, I have made a patch that implements this idea (see > attachment). It is a POC, but passes all regression tests. Thanks. Couple of comments off the top of my head: > Registration of an queryId generator implemented by analogy with extensible > methods machinery. Why not more like suggested with stakind and slots in some data structure? All of those generators have to be iterated anyway, so not sure if a hash table makes sense. > Also, I switched queryId to int64 type and renamed to > 'label'. A name with "id" in it would be better I believe. Label could be think of as "the query belongs to a certain category", while the purpose is identification. > 2. We need a custom queryId, that is based on a generated queryId (according > to the logic of pg_stat_statements). Could you clarify? > 4. We should reserve position of default in-core generator >From the discussion above I was under the impression that the core generator should be distinguished by a predefined kind. > 5. We should add an EXPLAIN hook, to allow an extension to print this custom > queryId. Why? It would make sense if custom generation code will be generating some complex structure, but the queryId itself is still a hash.
Re: Multiple Query IDs for a rewritten parse tree
Hi, On Fri, Jan 28, 2022 at 05:51:56PM +0100, Dmitry Dolgov wrote: > > On Fri, Jan 21, 2022 at 11:33:22AM +0500, Andrey V. Lepikhov wrote: > > On 1/9/22 5:49 AM, Tom Lane wrote: > > > The idea I'd been vaguely thinking about is to allow attaching a list > > > of query-hash nodes to a Query, where each node would contain a "tag" > > > identifying the specific hash calculation method, and also the value > > > of the query's hash calculated according to that method. We could > > > probably get away with saying that all such hash values must be uint64. > > > The main difference from your function-OID idea, I think, is that > > > I'm envisioning the tags as being small integers with well-known > > > values, similarly to the way we manage stakind values in pg_statistic. > > > In this way, an extension that wants a hash that the core knows how > > > to calculate doesn't need its own copy of the code, and similarly > > > one extension could publish a calculation method for use by other > > > extensions. > > > > To move forward, I have made a patch that implements this idea (see > > attachment). It is a POC, but passes all regression tests. > [...] > > 4. We should reserve position of default in-core generator > > From the discussion above I was under the impression that the core > generator should be distinguished by a predefined kind. I don't really like this approach. IIUC, this patch as-is is meant to break pg_stat_statements extensibility. If kind == 0 is reserved for in-core queryid then you can't use pg_stat_statement with a different queryid generator anymore. Unless you meant that kind == 0 is reserved for monitoring / pg_stat_statement purpose and custom extension should register that specific kind too if that's what they are supposed to implement? The patch also reserves kind == -1 for pg_stat_statements internal purpose, and I'm not really sure why that's needed. Are additional extension that want to agree with pg_stat_statements on what the monitoring queryid is supposed to do that too? I'm also unsure of how are extensions supposed to cooperate in general, as I feel that queryids should be implemented for some "intent" (like monitoring, planning optimization...). That being said I'm not sure if e.g. AQO heuristics are too specific for its need or if it could be shared with other extension that might be dealing with similar concerns.
Re: Multiple Query IDs for a rewritten parse tree
> On Sat, Jan 29, 2022 at 03:51:33PM +0800, Julien Rouhaud wrote: > Hi, > > On Fri, Jan 28, 2022 at 05:51:56PM +0100, Dmitry Dolgov wrote: > > > On Fri, Jan 21, 2022 at 11:33:22AM +0500, Andrey V. Lepikhov wrote: > > > On 1/9/22 5:49 AM, Tom Lane wrote: > > > > The idea I'd been vaguely thinking about is to allow attaching a list > > > > of query-hash nodes to a Query, where each node would contain a "tag" > > > > identifying the specific hash calculation method, and also the value > > > > of the query's hash calculated according to that method. We could > > > > probably get away with saying that all such hash values must be uint64. > > > > The main difference from your function-OID idea, I think, is that > > > > I'm envisioning the tags as being small integers with well-known > > > > values, similarly to the way we manage stakind values in pg_statistic. > > > > In this way, an extension that wants a hash that the core knows how > > > > to calculate doesn't need its own copy of the code, and similarly > > > > one extension could publish a calculation method for use by other > > > > extensions. > > > > > > To move forward, I have made a patch that implements this idea (see > > > attachment). It is a POC, but passes all regression tests. > > [...] > > > 4. We should reserve position of default in-core generator > > > > From the discussion above I was under the impression that the core > > generator should be distinguished by a predefined kind. > > I don't really like this approach. IIUC, this patch as-is is meant to break > pg_stat_statements extensibility. If kind == 0 is reserved for in-core > queryid > then you can't use pg_stat_statement with a different queryid generator > anymore. Unless you meant that kind == 0 is reserved for monitoring / > pg_stat_statement purpose and custom extension should register that specific > kind too if that's what they are supposed to implement? > > [...] > > I'm also unsure of how are extensions supposed to cooperate in general, as > I feel that queryids should be implemented for some "intent" (like monitoring, > planning optimization...). That being said I'm not sure if e.g. AQO > heuristics > are too specific for its need or if it could be shared with other extension > that might be dealing with similar concerns. Assuming there are multiple providers and consumers of queryIds, every such consumer extension needs to know which type of queryId it wants to use. E.g. in case of pg_stat_statements, it needs to be somehow configured to know which of those kinds to take, to preserve extensibility you're talking about. Does the answer make sense, or did you mean something else? Btw, the approach in this thread still doesn't give a clue what to do when an extension needs to reuse some parts of core queryId generator, as in case with pg_stat_statements and "IN" condition merging.
Re: Multiple Query IDs for a rewritten parse tree
Hi, On Sat, Jan 29, 2022 at 06:12:05PM +0100, Dmitry Dolgov wrote: > > On Sat, Jan 29, 2022 at 03:51:33PM +0800, Julien Rouhaud wrote: > > > > I'm also unsure of how are extensions supposed to cooperate in general, as > > I feel that queryids should be implemented for some "intent" (like > > monitoring, > > planning optimization...). That being said I'm not sure if e.g. AQO > > heuristics > > are too specific for its need or if it could be shared with other extension > > that might be dealing with similar concerns. > > Assuming there are multiple providers and consumers of queryIds, every > such consumer extension needs to know which type of queryId it wants to > use. E.g. in case of pg_stat_statements, it needs to be somehow > configured to know which of those kinds to take, to preserve > extensibility you're talking about. Does the answer make sense, or did > you mean something else? I guess, but I don't think that the proposed approach does that. The DBA should be able to configure a monitoring queryid provider, a planning queryid provider... and the extensions should have a way to know which is which. And also I don't think that the DBA should be allowed to setup multiple monitoring queryid providers, nor change them dynamically. > Btw, the approach in this thread still doesn't give a clue what to do > when an extension needs to reuse some parts of core queryId generator, > as in case with pg_stat_statements and "IN" condition merging. Indeed, as the query text normalization is not extensible.
Re: Multiple Query IDs for a rewritten parse tree
> On Sun, Jan 30, 2022 at 01:48:20AM +0800, Julien Rouhaud wrote: > Hi, > > On Sat, Jan 29, 2022 at 06:12:05PM +0100, Dmitry Dolgov wrote: > > > On Sat, Jan 29, 2022 at 03:51:33PM +0800, Julien Rouhaud wrote: > > > > > > I'm also unsure of how are extensions supposed to cooperate in general, as > > > I feel that queryids should be implemented for some "intent" (like > > > monitoring, > > > planning optimization...). That being said I'm not sure if e.g. AQO > > > heuristics > > > are too specific for its need or if it could be shared with other > > > extension > > > that might be dealing with similar concerns. > > > > Assuming there are multiple providers and consumers of queryIds, every > > such consumer extension needs to know which type of queryId it wants to > > use. E.g. in case of pg_stat_statements, it needs to be somehow > > configured to know which of those kinds to take, to preserve > > extensibility you're talking about. Does the answer make sense, or did > > you mean something else? > > I guess, but I don't think that the proposed approach does that. Yes, it doesn't, I'm just channeling my understanding of the problem.
Re: Multiple Query IDs for a rewritten parse tree
On 1/28/22 9:51 PM, Dmitry Dolgov wrote: On Fri, Jan 21, 2022 at 11:33:22AM +0500, Andrey V. Lepikhov wrote: Registration of an queryId generator implemented by analogy with extensible methods machinery. Why not more like suggested with stakind and slots in some data structure? All of those generators have to be iterated anyway, so not sure if a hash table makes sense. Maybe. But it is not obvious. We don't really know, how many extensions could set an queryId. For example, adaptive planning extensions definitely wants to set an unique id (for example, simplistic counter) to trace specific {query,plan} across all executions (remember plancache too). And they would register a personal generator for such purpose. Also, I switched queryId to int64 type and renamed to 'label'. A name with "id" in it would be better I believe. Label could be think of as "the query belongs to a certain category", while the purpose is identification. I think, it is not a full true. Current jumbling generates not unique queryId (i hope, intentionally) and pg_stat_statements uses queryId to group queries into classes. For tracking specific query along execution path it performs additional efforts (to remember nesting query level, as an example). BTW, before [1], I tried to improve queryId, that can be stable for permutations of tables in 'FROM' section and so on. It would allow to reduce a number of pg_stat_statements entries (critical factor when you use an ORM, like 1C for example). So, i think queryId is an Id and a category too. 2. We need a custom queryId, that is based on a generated queryId (according to the logic of pg_stat_statements). Could you clarify? pg_stat_statements uses origin queryId and changes it for a reason (sometimes zeroed it, sometimes not). So you can't use this value in another extension and be confident that you use original value, generated by JumbleQuery(). Custom queryId allows to solve this problem. 4. We should reserve position of default in-core generator From the discussion above I was under the impression that the core generator should be distinguished by a predefined kind. Yes, but I think we should have a range of values, enough for use in third party extensions. 5. We should add an EXPLAIN hook, to allow an extension to print this custom queryId. Why? It would make sense if custom generation code will be generating some complex structure, but the queryId itself is still a hash. Extension can print not only queryId, but an explanation of a kind, maybe additional logic. Moreover why an extension can't show some useful monitoring data, collected during an query execution, in verbose mode? [1] https://www.postgresql.org/message-id/flat/e50c1e8f-e5d6-5988-48fa-63dd992e9565%40postgrespro.ru -- regards, Andrey Lepikhov Postgres Professional
Re: Multiple Query IDs for a rewritten parse tree
> On Mon, Jan 31, 2022 at 02:59:17PM +0500, Andrey V. Lepikhov wrote: > On 1/28/22 9:51 PM, Dmitry Dolgov wrote: > > > On Fri, Jan 21, 2022 at 11:33:22AM +0500, Andrey V. Lepikhov wrote: > > > Registration of an queryId generator implemented by analogy with > > > extensible > > > methods machinery. > > > > Why not more like suggested with stakind and slots in some data > > structure? All of those generators have to be iterated anyway, so not > > sure if a hash table makes sense. > Maybe. But it is not obvious. We don't really know, how many extensions > could set an queryId. > For example, adaptive planning extensions definitely wants to set an unique > id (for example, simplistic counter) to trace specific {query,plan} across > all executions (remember plancache too). And they would register a personal > generator for such purpose. I don't see how the number of extensions justify a hash table? I mean, all of the generators will still most likely be executed at once (not on demand) and store the result somewhere. In any way I would like to remind, that this functionality wants to be on the pretty much hot path, which means strong evidences of no significant performance overhead are needed. And sure, there could be multiple queryId consumers, but in the vast majority of cases only one queryId will be generated. > > > 2. We need a custom queryId, that is based on a generated queryId > > > (according > > > to the logic of pg_stat_statements). > > > > Could you clarify? > pg_stat_statements uses origin queryId and changes it for a reason > (sometimes zeroed it, sometimes not). IIRC it does that only for utility statements. I still fail to see the problem, why would a custom extension not register a new generator if it needs something different? > > > 5. We should add an EXPLAIN hook, to allow an extension to print this > > > custom > > > queryId. > > > > Why? It would make sense if custom generation code will be generating > > some complex structure, but the queryId itself is still a hash. > > > Extension can print not only queryId, but an explanation of a kind, maybe > additional logic. > Moreover why an extension can't show some useful monitoring data, collected > during an query execution, in verbose mode? A good recommendation which pops up every now and then in hackers, is to concentrace on what is immediately useful, leaving "maybe useful" things for later. I have a strong feeling this is the case here.
Re: Multiple Query IDs for a rewritten parse tree
On 29/1/2022 12:51, Julien Rouhaud wrote: 4. We should reserve position of default in-core generator From the discussion above I was under the impression that the core generator should be distinguished by a predefined kind. I don't really like this approach. IIUC, this patch as-is is meant to break pg_stat_statements extensibility. If kind == 0 is reserved for in-core queryid then you can't use pg_stat_statement with a different queryid generator anymore. Yes, it is one more problem. Maybe if we want to make it extensible, we could think about hooks in the pg_stat_statements too? The patch also reserves kind == -1 for pg_stat_statements internal purpose, and I'm not really sure why that's needed. My idea - tags with positive numbers are reserved for generation results, that is performance-critical. As I see during the implementation, pg_stat_statements makes additional changes on queryId (no matter which ones). Because our purpose is to avoid interference in this place, I invented negative values, where extensions can store their queryIds, based on any generator or not. Maybe it is redundant - main idea here was to highlight the issue. I'm also unsure of how are extensions supposed to cooperate in general, as I feel that queryids should be implemented for some "intent" (like monitoring, planning optimization...). That being said I'm not sure if e.g. AQO heuristics are too specific for its need or if it could be shared with other extension that might be dealing with similar concerns. I think, it depends on a specific purpose of an extension. -- regards, Andrey Lepikhov Postgres Professional