Improve transaction check in refresh_cagg

[staticlibs]

Intro: Hi, I was investigating an issue with portal snapshots (0) did not account for all active snapshots (1) error inside another Postgres extension (wdb-97, unrelated to Timescale) and stumbled upon the #6533 issue in Timescale that was reporting the same error. Decided to have a deeper look into it.

This PR allows better error messages to be reported from refresh_continuous_aggregate when it is called from an atomic (no transaction allowed) context. One of the following messages:

• ERROR: refresh_continuous_aggregate() cannot run inside a transaction block
• ERROR: refresh_continuous_aggregate() cannot be executed from a function

is reported now instead of: ERROR: portal snapshots (N) did not account for all active snapshots (N+1). There are no other changes to refresh_continuous_aggregate logic.

Longer description, also included in process_utility.h:

Procedures that use multiple transactions cannot be run in a transaction block (from a function, from dynamic SQL) or in a subtransaction (from a procedure block with an EXCEPTION clause). Such procedures use PreventInTransactionBlock function to check whether they can be run.

Though currently such checks are incomplete, because PreventInTransactionBlock requires isTopLevel argument to throw a consistent error when the call originates from a function. This isTopLevel flag (that is a bit poorly named - see below) is not readily available inside C procedures. The source of truth for it - ProcessUtilityContext parameter is passed to ProcessUtility hooks, but is not included with the function calls. There is an undocumented SPI_inside_nonatomic_context function, that would have been sufficient for isTopLevel flag, but it currently returns false when SPI connection is absent (that is a valid scenario when C procedures are called from top-level SQL instead of PLPG procedures or DO blocks) so it cannot be used.

To work around this the value of ProcessUtilityContext parameter is saved when TS ProcessUtility hook is entered and can be accessed from C procedures using new ts_process_utility_is_context_nonatomic function. The result is called "non-atomic" instead of "top-level" because the way how isTopLevel flag is determined from the ProcessUtilityContext value in standard_ProcessUtility is insufficient for C procedures - it excludes PROCESS_UTILITY_QUERY_NONATOMIC value (used when called from PLPG procedure without an EXCEPTION clause) that is a valid use case for C procedures with transactions. See details in the description of ExecuteCallStmt function.

It is expected that calls to C procedures are done with CALL and always pass though the ProcessUtility hook. The ProcessUtilityContext parameter is set to PROCESS_UTILITY_TOPLEVEL value by default. In unlikely case when a C procedure is called without passing through ProcessUtility hook and the call is done in atomic context, then PreventInTransactionBlock checks will pass, but SPI_commit will fail when checking that all current active snapshots are portal-owned snapshots (the same behaviour that was observed before this change). In atomic context there will be an additional snapshot set in _SPI_execute_plan, see the snapshot handling invariants description in that function.

With initial version of this PR, in TS ProcessUtility hook the saved ProcessUtilityContext value is reset back to PROCESS_UTILITY_TOPLEVEL on normal exit but is NOT reset in case of ereport exit. C procedures can call ts_process_utility_context_reset function to reset the saved value before doing the checks that can result in ereport exit. The scenario when more thorough reset may be necessary - when subsequent calls after the failed atomic call are not passed through the ProcessUtility hook - seems to be unlikely.

Closes #6533.

[github-actions]

@erimatnor, @gayyappan: please review this pull request.

Powered by pull-review

[fabriziomello]

@staticlibs thanks a lot for the PR. I'll have a look on it!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.32%. Comparing base (59f50f2) to head (5091105).
Report is 672 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7566      +/-   ##
==========================================
+ Coverage   80.06%   82.32%   +2.25%     
==========================================
  Files         190      238      +48     
  Lines       37181    43733    +6552     
  Branches     9450    10971    +1521     
==========================================
+ Hits        29770    36004    +6234     
- Misses       2997     3392     +395     
+ Partials     4414     4337      -77     

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

[staticlibs]

@fabriziomello thanks! I've just added a few fixes to format and spelling (no code changes) and added the changelog file to .unreleased dir.

[fabriziomello]

@staticlibs BTW very good catch... thanks a lot.

[fabriziomello]

Why don't error out here when nonatomic is false?

[staticlibs]

The idea was to continue to use PreventInTransactionBlock check (that is also used in a few other places, for example in tsl_move_chunk). Though in current PR version opening the SPI connection in atomic mode at this place (after we got nonatomic=false) may indeed be confusing/useless. Maybe it would be better instead of getting the flag value from ts_process_utility_is_context_nonatomic - instead to perform a check (and throw an error) inside some new ProcessUtility helper function like ts_process_utility_check_context_nonatomic? Perhaps also to reset the saved ProcessUtilityContext value in this helper function just before the check and do not expose ts_process_utility_context_reset?

[fabriziomello]

Maybe we can just move the Prevent* checks to before opening the SPI connection. WDYT? IMHO make no sense open the SPI connection and then bail out because of one of those checks.

[staticlibs]

Moved Prevent* checks before the SPI call.