We have a two groups of issues related to nested levels.
1) We found that const "char *query_string" from planner hook is always valid and we would like to use it (as it was done in the pg_hint_plan master branch).
The current logic for PG13 that tries to extract the query from ParseState/Query is perhaps sometimes redundant (because the planner code already does it) and sometimes leads to errors and hint parsing of the irrelevant string.
2) We have a complex functions that use error handling inside that functions. We found that when we catch any error within a function/procedure then pg_hint_plan does not process any new hints until the end of the transaction.
In this case the plpgsql_recurse_level counter become invalid because the pg_hint_plan_plpgsql_stmt_end is not called when a handled exception occurs.
-- Example
begin;
create or replace function pg_temp.func_with_exception(p_var varchar) returns numeric language plpgsql as
$$
begin
return p_var::numeric;
exception when others then null;
-- pg_hint_plan_plpgsql_stmt_end is not called
return -1;
end;
$$;
select pg_temp.func_with_exception('abc');
explain (COSTS false) /*+MergeJoin(d1 d2) */ with dual as (select 'x' as dummy) select * from dual d1, dual d2 where d1.dummy = d2.dummy;
-- merge will not work till commit/rollback
drop function pg_temp.func_with_exception(varchar);
rollback;
explain (COSTS false) /*+MergeJoin(d1 d2) */ with dual as (select 'x' as dummy) select * from dual d1, dual d2 where d1.dummy = d2.dummy;
We tried to merge the behavior associated with plpgsql_recurse_level from the master branch but found that we still have issues - old parsed hints from old queries are applied to new queries.
We also tried to use ExecutorRun_hook and ExecutorFinish_hook instead of PLpgSQL_plugin similar as it was done in the contrib/pg_stat_statements.c and our tests works fine after that.
However we noticed that one of the regression tests failed after this fix (after change to ExecutorRun_hook/ExecutorFinish_hook) - the nested_planner case from sql/ut-A.sql.
In our point of view this case begin works more correctly after fix but we don't know how many users of pg_hint_plan assumpted the current behavior and will be affected by this fix..
We also see that there was fixes for extracting queries from Query/State in PG13 branch and this affects further reading and hint parsing. We also don't know how many users have assumpted this behavior and will be affected.
Therefore as an alternative to keep the old behavior and fix our issues with nested levels in PG13 branch we suggest to using a new flag pg_hint_plan.disable_parsed_hint_reuse that:
a) Always use the actual constant char* query_string
b) Ignores the current value of the plpgsql_recurse_level counter.
This is done because with the current planner behavior and the order in which other planner hooks are fired in PG13/PG14 we have not found any cases where this counter would be really needed.
So instead of duplicating the ExecutorRun/ExecutorFinish logic from pg_stat_statements we suggest to not to use this counter.
We have a two groups of issues related to nested levels.
1) We found that const "char *query_string" from planner hook is always valid and we would like to use it (as it was done in the pg_hint_plan master branch). The current logic for PG13 that tries to extract the query from ParseState/Query is perhaps sometimes redundant (because the planner code already does it) and sometimes leads to errors and hint parsing of the irrelevant string.
2) We have a complex functions that use error handling inside that functions. We found that when we catch any error within a function/procedure then pg_hint_plan does not process any new hints until the end of the transaction. In this case the plpgsql_recurse_level counter become invalid because the pg_hint_plan_plpgsql_stmt_end is not called when a handled exception occurs.
We tried to merge the behavior associated with plpgsql_recurse_level from the master branch but found that we still have issues - old parsed hints from old queries are applied to new queries.
We also tried to use ExecutorRun_hook and ExecutorFinish_hook instead of PLpgSQL_plugin similar as it was done in the contrib/pg_stat_statements.c and our tests works fine after that.
However we noticed that one of the regression tests failed after this fix (after change to ExecutorRun_hook/ExecutorFinish_hook) - the nested_planner case from sql/ut-A.sql. In our point of view this case begin works more correctly after fix but we don't know how many users of pg_hint_plan assumpted the current behavior and will be affected by this fix..
We also see that there was fixes for extracting queries from Query/State in PG13 branch and this affects further reading and hint parsing. We also don't know how many users have assumpted this behavior and will be affected.
Therefore as an alternative to keep the old behavior and fix our issues with nested levels in PG13 branch we suggest to using a new flag pg_hint_plan.disable_parsed_hint_reuse that: a) Always use the actual constant char* query_string b) Ignores the current value of the plpgsql_recurse_level counter. This is done because with the current planner behavior and the order in which other planner hooks are fired in PG13/PG14 we have not found any cases where this counter would be really needed. So instead of duplicating the ExecutorRun/ExecutorFinish logic from pg_stat_statements we suggest to not to use this counter.