Re: PoC. The saving of the compiled jit-code in the plan cache - Mailing list pgsql-hackers

Matheus Alcantara писал(а) 2025-03-07 05:02:

Hi!

  Thank for interest to the patch.

>> 1. Changes in jit-code generation.
>> 
>> a) the load of the absolute address (as const) changed to the load of 
>> this
>> address from a struct member:
>> 
> If I understood correctly, this change is required to avoid making the
> jit code points to a memory address that was reallocated (making it
> invalid)? If that's the case, would be possible to split this change
> into a separated patch? It would help a lot to review.
Yes, you correct. In this patch the value of the pointers initialised
every time, when code called for new structure, to reffer to new 
address.
I did not put it into separate patch, because now I see all changes
in the system are interconnected and require to change many files.
It is hard to maintain, at least now.

Also I did it to check the possibility to implement it and check, is it
really better. During implementation I understood, that the design must
be changed and clarified, it looks dirty now.

> 
> It's not clear to me what's the difference between jit_context being 
> set
> to NULL or CACHED_JITCONTEXT_EMPTY and how it changes from NULL to
> CACHED_JITCONTEXT_EMPTY. I think some code comments will help on this,
> specially on llvm_compile_expr around if/else blocks.
  When llvm_compile_expr is called, I need to know, is it compilation for
cache or usual compilation. I use jit_context for this. It is always
initialised by NULL in palloc0(). If plan is saved to cache,
jit_context assigned CACHED_JITCONTEXT_EMPTY - it means, jit also must
me saved in cache, but not saved yet.
> 
>> Updated patch rebased to the current master. Also I resolved the 
>> problems
>> with the lookup of the compiled expressions.
>>  Cached jit compiles only expressions from cached plan - they are
>> recognized by memory context - if Expr is not NULL and belong to the 
>> same
>> memory context as cached plan, this expression is compiled to the 
>> function
>> with expression address in the name (expression address casted to Size
>> type).
>>  All other expression (generated later than plan, f.e. expressions in
>> aggregates, hash join, hash aggregates) are not compiled and are 
>> executed
>> by standard expression interpreter.
>> 
> What's stopping us from going back to the current code generation
> without caching on these scenarios? I'm a bit concerned for never jit
> compile these kind of expressions and have some kind of performance
> issue.
> 
I thought about this. The reason to store the jit in cache - avoid
compiling every time when query is executed. If we anyway compile
the code even for 1 function, we lose benefits of cached jit code.
We can choose the standard jit compilation for other functions.

> What's the relation of this exec time generated expressions (hash join,
> hash agg) problem with the function name lookup issue? It's seems
> different problems to me but I may be missing something.
This question and next question are correlated.
> If these problems is not fully correlated I think that it would be
> better to have some kind of hash map or use the number of the call to
> connect expressions with functions (as you did on v4) to lookup the
> cached compiled function. IMHO it sounds a bit strange to me to add the
> function pointer on the function name and have this memory context
> validation.
  This function lookup the biggest challenge in this patch, I see
it correct.
  In current implementation: the function is generated for expression,
the name of the function is saved in the ExprState,
then function is compiled (when first expression is executed),
then the function is called,
lookup by saved address is done to find compiled code,
the compiled code address is saved in ExprState as new execution code,
the compiled code is called.


  In cached implementation when expression is called the next time,
we need to find the link among the expression and the compiled code.
I tried to find reliable way to connect them, and see now only one
version - make compilation only for expressions saved with the plan -
they have the same address across the time and the same memory context
as the saved plan. Expressions generated during execution like
aggregate expression and Hash join expression are allocated in
query execution context and always have different addresses, and I did
not find any way to connect them with cached plan. The have "expr"
member, but it can be NULL, T_List or T_Expr (or anything, what
Custom node can assign to it).
  Function name with expression address now looks as reliable way to find
compiled function, though not elegant. Standard implementation just 
stores
generated name, uses it and pfree() it.

> I've also executed make check-world and it seems that
> test_misc/003_check_guc is falling:
> 
> [11:29:42.995](1.153s) not ok 1 - no parameters missing from
> postgresql.conf.sample
> [11:29:42.995](0.000s) #   Failed test 'no parameters missing from
> postgresql.conf.sample'
> #   at src/test/modules/test_misc/t/003_check_guc.pl line 85.
> [11:29:42.995](0.000s) #          got: '1'
> #     expected: '0'
> [11:29:42.995](0.000s) ok 2 - no parameters missing from guc_tables.c
> [11:29:42.995](0.000s) ok 3 - no parameters marked as NOT_IN_SAMPLE in
> postgresql.conf.sample
> found GUC jit_cached in guc_tables.c, missing from 
> postgresql.conf.sample

I tested in with make check and make installcheck . In v8 I found bugs, 
but
not published fix yet.



-- 
Best regards,

Vladlen Popolitov.