Re: Make query ID more portable

On 12/10/21 13:35, Julien Rouhaud wrote:
> Hi,
> 
> On Tue, Oct 12, 2021 at 4:12 PM Andrey V. Lepikhov
> <a.lepikhov@postgrespro.ru> wrote:
>> See the patch in attachment as an POC. Main idea here is to break
>> JumbleState down to a 'clocations' part that can be really interested in
>> a post parse hook and a 'context data', that needed to build query or
>> subquery signature (hash) and, I guess, isn't really needed in any
>> extensions.
> 
> There have been quite a lot of threads about that in the past, and
> almost every time people wanted to change how the hash was computed.
> So it seems to me that extensions would actually be quite interested
> in that.  This is even more the case now that an extension can be used
> to replace the queryid calculation only and keep the rest of the
> extension relying on it as is.
Yes, I know. I have been using such self-made queryID for four years. 
And I will use it further.
But core jumbling code is good, fast and much easier in support. The 
purpose of this work is extending of jumbling to use in more flexible 
way to avoid meaningless copying of this code to an extension.
>> I think, it adds not much complexity and overhead.
> 
> I think the biggest change in your patch is:
> 
>    case RTE_RELATION:
> - APP_JUMB(rte->relid);
> - JumbleExpr(jstate, (Node *) rte->tablesample);
> + {
> + char *relname = regclassout_ext(rte->relid, true);
> +
> + APP_JUMB_STRING(relname);
> + JumbleExpr(jstate, (Node *) rte->tablesample, ctx);
>    APP_JUMB(rte->inh);
>    break;
> 
> Have you done any benchmark on OLTP workload?  Adding catalog access
> there is likely to add significant overhead.
Yes, I should do benchmarking. But I guess, main goal of Query ID is 
monitoring, that can be switched off, if necessary.
This part made for a demo. It can be replaced by a hook, for example.
> 
> Also, why only using the fully qualified relation name for stable
> hashes?  At least operators and functions should also be treated the
> same way.  If you do that you will probably have way too much overhead
> to be usable in a busy production environment.  Why not using the new
> possibility of 3rd party extension for the queryid calculation that
> exactly suits your need?
> 
I fully agree with these arguments. This code is POC. Main part here is 
breaking down JumbleState, using a local context for subqueries and 
sorting of a range table entries hashes.
I think, we can call one routine (APP_JUMB_OBJECT(), as an example) for 
all oids in this code. It would allow an extension to intercept this 
call and replace oid with an arbitrary value.

-- 
regards,
Andrey Lepikhov
Postgres Professional