Skip to content

Commit

Permalink
Simplify tracking of recursive call depth inside PL/pgSQL functions
Browse files Browse the repository at this point in the history
This commit replaces and simplifies the calculation of the counter
tracking the recursion level inside PL/pgSQL functions by relying on
fmgr_start and end hooks within the function manager for this purpose.
The previous implementation had a huge flaw: it failed to properly
detect the case of exceptions in PL functions, hence it would be
possible to break the static counter so badly that the hint to use would
be incorrect, or utterly broken in some cases where it would finish by
being negative when stacking exceptions.

a9863af has provided a band-aid fix to address this issue, but it has
been relying on the erase_callback of PL/pgSQL to force a level of 0
when reaching the top-level, while the correct method would be to
decrement the counter and apply sanity checks to make sure that the
depth calculation is never messed up when coming back to the top-level.

This commit switches the calculation of the depth to not rely any more
on a resource owner callback or the plpgsql start/end hooks, as it
happens that the function manager is correctly able:
- To track if an exception happens within a PL/pgSQL function as a
TRY/CATCH block would call the fmgr_hook before re-throwing an error.
- To increment and decrement the count if facing a PL/pgSQL function.
- To add sanity checks on the depth level to make sure that this is
*never* less than zero after decrementing, and always higher than zero
after incrementing.

The only tweak that needs to be done is to check if a function needs to
call the fmgr hook, something that can be done by checking if the
function is written in PL/pgSQL.  Even-though this requires caching the
language OID on the first lookup, the change is quite intuitive.

Relying on the fmgr hook has the advantage to not require the
registration of the resowner callback for all the backends, saving some
cycles in the most common cases.

The test cases for PL functions are extended a bit to check for more
scenarios:
- test_hint_transaction() gains a before/after mode to control the timing
of the internal COMMIT and ROLLBACK sub-commands.
- test_hint_tab() gains a new query with a HashJoin hint, mixed with all
the others to check that the hint is correctly applied, across multiple
levels of depth.

This is arguably a bug fix, and structurally it could be backpatch, but
I am not doing so out of caution.

Per pull request #131.
  • Loading branch information
michaelpq committed Apr 26, 2023
1 parent a03bbdb commit 7c6d950
Show file tree
Hide file tree
Showing 3 changed files with 688 additions and 161 deletions.
Loading

0 comments on commit 7c6d950

Please sign in to comment.