Load hypercore handler eagerly

[mkindahl]

If a crossmodule wrapper function is called as part of a query on a fresh backend, it will generate a license error even if the license is set correctly. This happens because the module load happens very late (in post_parse_analyze_hook) but the function call is quite early.

This is fixed by eagerly loading the TSL module in the hypercore handler function and hypercore index proxy handler function.

[codecov]

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.93%. Comparing base (59f50f2) to head (6b1fdea).
Report is 783 commits behind head on main.

Files with missing lines	Patch %	Lines
src/cross_module_fn.c	70.00%	2 Missing and 1 partial ⚠️
src/init.c	33.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7711      +/-   ##
==========================================
+ Coverage   80.06%   81.93%   +1.87%     
==========================================
  Files         190      246      +56     
  Lines       37181    45127    +7946     
  Branches     9450    11259    +1809     
==========================================
+ Hits        29770    36977    +7207     
- Misses       2997     3727     +730     
- Partials     4414     4423       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[erimatnor]

What does this function actually do? From the function name it sounds like module loading is enabled after the function succeeds, but is the tsl module actually loaded? Or, can it enable module loading but actually not load the module.

Is there a case when it fails to load the module? I assume that it can fail given the check below.

In that case, I think the function has a confusing name. Maybe it should be called ts_license_try_enable_module_loading() and then it returns true or false or error depending on whether it succeeds.

[mkindahl]

Seems reasonable, but I prefer to take a more serious refactoring as a separate pull request.

[erimatnor]

Understandable. But refactoring aside, I still don't understand what is going on here. I had several questions around what is happening.

[mkindahl]

Ok, I'll bite.

• "What does this function actually do?" It turns on the load_enabled flag and re-sets the timescaledb.license GUC, so your answer partially correct. The missing piece is that since the GUC in question has a check hook (ts_license_guc_check_hook to be precise), it will attempt to load the module as a result of setting the variable by calling tsl_module_load() if you use the license timescale.
• "Is there a case when it fails to load the module?" Yes, but it will either throw an error inside ts_license_guc_check_hook or not reset the crossmodule call inside the crossmodule table.

[erimatnor]

Ok, so if I get this right, the function ts_license_enable_module_loading has the following behavior:

1. If license is community (or whatever the string is), it will be silent and not update ts_cm_functions
2. If license is timescale it will also be silent but update ts_cm_functions
3. If license is anything else it will throw an error?

and as the function doesn't return anything useful about what actually happened, we're forced to compare function pointers to check for case 1 or 2 above?

Then I think I understand what is going on. But my feedback is that this probably needs refactoring because it is very subtle and potentially error prone. I think this should be handled automatically by all function stubs. In other words, the standard behavior for a "cross-module" function should be to try load the module and then fail if license doesn't allow execution of the function.

Not saying the refactoring needs to happen in this PR, but I am sure we will keep on stumbling on the same issues unless we fix this.

[mkindahl]

Ok, so if I get this right, the function ts_license_enable_module_loading has the following behavior:

1. If license is `community` (or whatever the string is), it will be silent and not update ts_cm_functions

2. If license is `timescale`  it will also be silent but update ts_cm_functions

3. If license is anything else it will throw an error?

There are a few more safety-guards, but this is essentially it.

and as the function doesn't return anything useful about what actually happened, we're forced to compare function pointers to check for case 1 or 2 above?

Correct.

Then I think I understand what is going on. But my feedback is that this probably needs refactoring because it is very subtle and potentially error prone. I think this should be handled automatically by all function stubs. In other words, the standard behavior for a "cross-module" function should be to try load the module and then fail if license doesn't allow execution of the function.

Not saying the refactoring needs to happen in this PR, but I am sure we will keep on stumbling on the same issues unless we fix this.

Yes, and this is why I discuss doing that refactoring as a separate pull request: it is subtle and error prone and requires slightly deeper investigations and refactoring, but we have an outstanding issue that needs fixing and this pull request fixes it.

[erimatnor]

Approving, but IMO this is band-aid and we should probably refactor cross-module stub functions so that they always try to load the module.

[erimatnor]

Ok, so if I get this right, the function ts_license_enable_module_loading has the following behavior:

1. If license is community (or whatever the string is), it will be silent and not update ts_cm_functions
2. If license is timescale it will also be silent but update ts_cm_functions
3. If license is anything else it will throw an error?

and as the function doesn't return anything useful about what actually happened, we're forced to compare function pointers to check for case 1 or 2 above?

Then I think I understand what is going on. But my feedback is that this probably needs refactoring because it is very subtle and potentially error prone. I think this should be handled automatically by all function stubs. In other words, the standard behavior for a "cross-module" function should be to try load the module and then fail if license doesn't allow execution of the function.

Not saying the refactoring needs to happen in this PR, but I am sure we will keep on stumbling on the same issues unless we fix this.