Hook calling order
JimFAmazon opened this issue · 5 comments
The copy_generic_path_info_hook currently performs its action first, and then at the end checks to see if a prev_copy_generic_path_info_hook was defined, and (if a previous is defined) then does that previous action last. The test and set should occur at the start of aqo_prev_copy_generic_path_info so that the order of execution follows the order of execution as specified in shared_preload_libraries in the event that more than one hook override is specified.
A sequence of hook callbacks following the call-my-predecessor-first convention would call the hooks in the order specified in shared_preload_libraries.
fyi, pg_stat_statements overrides the ExecutorEnd hook and calls the standard_ExecutorEnd at the end if there are no previous hooks defined, which clears the queryDesc->planstate, so at least in the case of the ExecutorEnd hook the current convention requires any additional hooks that depend on the planstate to do whatever they need to do with the planstate first, before calling the previous ExecutorEnd hook or standard_ExecutorEnd. So in the case of references to the planstate in the ExecutorEnd hook, at least, the actions on the planstate need to happen in the REVERSE of the order specified in shared_preload_libraries.
Related to the comments above, since other extensions may overload the same hook that fills in dest->had_path, path_clauses, etc., then it would eliminate some unnecessary work to test if dest->had_path is already true, and if so, return. The convention will be that any extension that sets had_path is also responsible for setting path_clauses, path_jointype, path_relids, path_parallel_workers, and was_parameterized.
The copy_generic_path_info_hook currently performs its action first, and then at the end checks to see if a prev_copy_generic_path_info_hook was defined, and (if a previous is defined) then does that previous action last. The test and set should occur at the start of aqo_prev_copy_generic_path_info so that the order of execution follows the order of execution as specified in shared_preload_libraries in the event that more than one hook override is specified.
A sequence of hook callbacks following the call-my-predecessor-first convention would call the hooks in the order specified in shared_preload_libraries.
Ok, fixed.
Related to the comments above, since other extensions may overload the same hook that fills in dest->had_path, path_clauses, etc., then it would eliminate some unnecessary work to test if dest->had_path is already true, and if so, return. The convention will be that any extension that sets had_path is also responsible for setting path_clauses, path_jointype, path_relids, path_parallel_workers, and was_parameterized.
I changed the code and added an assert to check this convention.
fyi, pg_stat_statements overrides the ExecutorEnd hook and calls the standard_ExecutorEnd at the end if there are no previous hooks defined, which clears the queryDesc->planstate, so at least in the case of the ExecutorEnd hook the current convention requires any additional hooks that depend on the planstate to do whatever they need to do with the planstate first, before calling the previous ExecutorEnd hook or standard_ExecutorEnd. So in the case of references to the planstate in the ExecutorEnd hook, at least, the actions on the planstate need to happen in the REVERSE of the order specified in shared_preload_libraries.
Ok. We do so. I added some warn comments into the code.