Skip to content

INSERT-EXECUTE Redesign#4678

Open
monk0707 wants to merge 21 commits intobabelfish-for-postgresql:BABEL_5_X_DEVfrom
monk0707:dest_receiver_and_temp_table
Open

INSERT-EXECUTE Redesign#4678
monk0707 wants to merge 21 commits intobabelfish-for-postgresql:BABEL_5_X_DEVfrom
monk0707:dest_receiver_and_temp_table

Conversation

@monk0707
Copy link
Copy Markdown

Description

What changed:

Currently, Babelfish's INSERT...EXECUTE implementation uses a tuple store approach that has several limitations including wrong column insertions, memory leaks and transaction issues. With this change, INSERT...EXECUTE now uses a temp table for buffering and dest receiver for redirecting results into temp table.

This new approach properly handles:

  • Correct transaction semantics matching SQL Server behavior
  • Proper column mapping and type coercion
  • Stable execution without crashes or hangs
  • Proper memory management
  • OUTPUT clause handling
  • All syntax variations (procedures, dynamic SQL, sp_executesql)
  • Ownership chaining (procedures calling other procedures owned by the same user)
  • Table variables (@tablevar) and temp tables (#temp) as targets
  • TRY-CATCH error handling
  • INSTEAD OF triggers on target tables

Why the change was made:

The previous tuple store approach had critical issues that couldn't be fixed incrementally:

  1. Transaction behavior inconsistencies: The rewriting approach didn't properly handle @@TRANCOUNT, implicit transactions, and rollback semantics
  2. Wrong Column Insertion: Column mapping was incorrect when procedure output didn't match target table column order
  3. Server Crashes and Hangs: Complex scenarios with nested procedures, triggers, or error handling caused server instability
  4. Memory Management Problems: The rewriting approach had memory leaks and improper cleanup in error scenarios
  5. OUTPUT Clause Mishandling: OUTPUT clause on INSERT EXEC wasn't properly supported
  6. Syntax Parsing Gaps: Many valid T-SQL syntax variations weren't recognized by the parser

The DestReceiver + temp table approach solves these by:

  • Using PostgreSQL's native tuple routing mechanism
  • Isolating procedure execution from the INSERT operation
  • Providing clean transaction boundaries
  • Enabling proper error handling and cleanup

How the code was changed (high-level roadmap):

  1. pl_exec-2.c: Core INSERT EXEC implementation

    • create_insert_exec_temp_table(): Creates temp table using catalog queries (pg_attribute) to avoid permission issues
    • CreateInsertExecDestReceiver(): Custom DestReceiver that captures procedure output
    • insertexec_receive(): Handles type coercion and inserts into temp table
    • flush_insert_exec_temp_table(): Copies data from temp to target table
    • Security context switching for ownership chaining support
  2. pl_exec.c / iterative_exec.c: Statement execution

    • Detection of INSERT EXEC context
    • Temp table lifecycle management
    • Error handling and cleanup
  3. tsqlIface.cpp: Parser changes

    • Proper detection of INSERT EXEC syntax
    • Handling of temp tables (#), table variables (@), and regular tables
    • Column list extraction
  4. hooks.c: Utility hooks

    • DROP TABLE protection for active INSERT EXEC target tables (SQL Server error 556)
  5. pltsql.h / pltsql-2.h: New declarations

    • INSERT EXEC context management functions
    • DestReceiver structures

Issues Resolved

  • Transaction behavior inconsistencies with @@TRANCOUNT and implicit transactions
  • Wrong column insertion when procedure output order differs from target
  • Server crashes with nested procedures and complex error scenarios
  • Memory leaks and improper cleanup
  • OUTPUT clause not working with INSERT EXEC
  • Various syntax parsing gaps for valid T-SQL
  • Ownership chaining broken for INSERT EXEC
  • Table variables (@tablevar) not recognized correctly
  • Type coercion not handling typmod differences (e.g., varchar(max) to varchar(10))

Test Scenarios Covered

  • Use case based - Basic INSERT EXEC with procedures, dynamic SQL, sp_executesql; Multiple result sets; Ownership chaining scenarios; Column list specifications; Table variables and temp tables as targets; Nested procedure calls; INSTEAD OF triggers on target tables
  • Boundary conditions - Empty result sets; Single row vs multiple rows; Maximum column counts; Type coercion edge cases (varchar(max) to varchar(10), int to datetime); Large result sets
  • Arbitrary inputs - Various data types; NULL handling; Unicode data
  • Negative test cases - Nested INSERT EXEC (error 8164); Column count mismatch (error 213); Type conversion failures; DROP TABLE on active target (error 556); Permission denied scenarios; Transaction errors
  • Minor version upgrade tests - N/A (feature redesign)
  • Major version upgrade tests - N/A (feature redesign)
  • Performance tests - Basic performance validation with large result sets
  • Tooling impact - None
  • Client tests - JDBC tests: BABEL-INSERT-EXEC, BABEL-INSERT-EXECUTE, proc_ownership_chaining, BABEL-2999, temp_table_rollback, BABEL-1944, datareader_datawriter

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.

monk0707 added 19 commits March 15, 2026 00:42
Error Handling Fixes:
- Fix memory corruption (pfree invalid pointer) by allocating INSERT EXEC
  context strings in TopMemoryContext so they survive error handling
- Fix PG_CATCH cleanup - don't call drop_insert_exec_temp_table() in error
  context since SPI_execute cannot be used when transaction is aborted
- Add proper nested INSERT EXEC detection with correct error code
  (ERRCODE_PROGRAM_LIMIT_EXCEEDED) and message matching old code path

sp_executesql Support:
- Add INSERT EXEC fields to PLtsql_stmt_exec_sp struct in pltsql-2.h
- Add PLTSQL_STMT_EXEC_SP case to parser in tsqlIface.cpp
- Add INSERT EXEC temp table lifecycle handling to exec_stmt_exec_sp()

Schema-Qualified Table Name Resolution:
- Fix create_insert_exec_temp_table() to include schema in target table name
- Parse schema from target_table, convert to physical schema using
  get_physical_schema_name(), include namespace in pg_attribute query

All three EXEC code paths now support INSERT EXEC with proper error handling:
- PLtsql_stmt_exec (EXEC proc_name)
- PLtsql_stmt_exec_batch (EXEC @variable)
- PLtsql_stmt_exec_sp (EXEC sp_executesql)
This commit fixes the handling of statement-terminating errors (like division
by zero) during and after INSERT EXEC operations to match SQL Server behavior.

Problem:
1. When INSERT EXEC completed inside TRY-CATCH but a subsequent error occurred,
   Babelfish rolled back all data. SQL Server preserves the INSERT EXEC data.
2. When INSERT EXEC completed without TRY-CATCH and a subsequent error occurred,
   Babelfish rolled back all data. SQL Server preserves the INSERT EXEC data.

Root Cause:
- Statement-terminating errors should only terminate the failing statement,
  not roll back previous work.
- Babelfish was calling AbortCurrentTransaction() for all errors without
  explicit transactions.
- TRY-CATCH subtransaction rollback was undoing INSERT EXEC flush.

Solution:
1. Skip AbortCurrentTransaction() for statement-terminating errors (identified
   by is_ignorable_error()) regardless of TRY-CATCH presence.
2. In exec_stmt_try_catch, release (commit) the subtransaction instead of
   rolling back when a statement-terminating error occurs and INSERT EXEC
   was active.
3. Wrap flush INSERT in its own subtransaction that commits immediately,
   so subsequent errors don't undo the flushed data.
4. Add TRY-CATCH depth tracking for INSERT EXEC context.

Files modified:
- iterative_exec.c: dispatch_stmt_handle_error - skip abort for stmt-terminating errors
- pl_exec-2.c: exec_stmt_try_catch - release subtxn for stmt-terminating errors
- pl_exec-2.c: flush_insert_exec_temp_table - wrap flush in committed subtxn
- pl_exec-2.c: Added TRY-CATCH depth tracking functions
- pltsql.h: Added declarations for TRY-CATCH depth tracking

Test results (all match SQL Server):
- Test 1: Basic INSERT EXEC - 3 rows
- Test 2: TRY-CATCH, error after INSERT EXEC - 3 rows
- Test 7: Error without TRY-CATCH - 3 rows (was 0 before fix)
- Test 8: Procedure with inner TRY-CATCH - 1 row
- Test 9: Nested TRY-CATCH with THROW - 3 rows
Previously, when a procedure called via INSERT EXEC contained a ROLLBACK
statement, the server would crash with 'FATAL: UserAbortTransactionBlock:
unexpected state STARTED' instead of returning an error.

The fix ensures that ROLLBACK is blocked during INSERT EXEC by:
1. Adding a check in hooks.c to route ROLLBACK statements through
   PLTsqlProcessTransaction when INSERT EXEC is active
2. Adding pltsql_insert_exec_active() check in pltsql_utils.c alongside
   the existing estate->insert_exec check
3. Adding backup checks in pl_exec.c for exec_stmt_execsql and
   exec_stmt_rollback

Also removes debug logging from tsqlIface.cpp and adds error mapping
for nested INSERT EXEC (error 8164).
When a statement-terminating error (like division by zero) occurs during
INSERT EXEC, we skip AbortCurrentTransaction() to preserve the buffered
data. However, SPI pushes an active snapshot that may not be popped if
the error is thrown.

Without cleanup, subsequent statements fail with:
'portal snapshots did not account for all active snapshots'

This fix adds snapshot cleanup whenever we skip AbortCurrentTransaction(),
whether for INSERT EXEC or for statement-terminating errors. This ensures
proper cleanup while preserving the INSERT EXEC data.

Test results:
- Test 1 (Basic INSERT EXEC): 3 rows ✓
- Test 2 (Division by zero, no TRY-CATCH): 2 rows ✓ (was 0)
- Test 4 (RAISERROR, no TRY-CATCH): 2 rows ✓
- Test 5 (RAISERROR with TRY-CATCH): 0 rows ✓
- Test 6 (Procedure with internal TRY-CATCH): 2 rows ✓
When a procedure executed via INSERT EXEC has its own internal TRY-CATCH
that catches an error, the INSERT EXEC context should NOT be cleaned up
because the procedure may continue to produce more rows.

The fix tracks the call stack depth when INSERT EXEC is started, and only
cleans up the context when the TRY-CATCH that catches the error is at the
same level or higher than where INSERT EXEC was started.

Changes:
- Add insert_exec_call_stack_depth to track when INSERT EXEC started
- Add pltsql_insert_exec_should_cleanup_on_trycatch() to check if cleanup
  is appropriate based on call stack depth comparison
- Update sigsetjmp handler to use the new function instead of just checking
  if INSERT EXEC is active
- Remove debug logging from pltsql_clear_insert_exec_context()
When a procedure returns more or fewer columns than the target table has,
the server would crash when trying to insert the tuple into the temp table.

Added column count validation in insertexec_startup() that compares the
number of columns in the result set with the temp table structure. If
there's a mismatch, we raise error 213 (ERRCODE_DATATYPE_MISMATCH) with
the message 'Column name or number of supplied values does not match
table definition.' - matching SQL Server's behavior.
This commit fixes several issues with INSERT EXEC:

1. OUTPUT clause check: Added check in tsqlIface.cpp to throw error 483
   when OUTPUT clause is used with INSERT EXEC.

2. @@TRANCOUNT handling during INSERT EXEC:
   - Modified PLTsqlStartTransaction to skip BeginTransactionBlock() during
     INSERT EXEC since we're already inside a subtransaction
   - Modified PLTsqlCommitTransaction to skip EndTransactionBlock() during
     INSERT EXEC and just decrement NestedTranCount
   - Added tracking of original NestedTranCount to restore on cleanup

3. Transaction routing in hooks.c:
   - Added TRANS_STMT_COMMIT and TRANS_STMT_BEGIN to the INSERT EXEC
     condition so they route through PLTsqlProcessTransaction

4. Error handling improvements:
   - Added column count validation in insertexec_startup() with proper
     error message
   - Updated error code for nested INSERT EXEC from 33557097 to 8164
   - Improved cleanup in iterative_exec.c when no TRY-CATCH is present

5. Row count handling:
   - Added pltsql_insert_exec_active to protocol plugin for TDS layer
   - Updated tdsresponse.c to properly report row counts for INSERT EXEC

6. Updated expected test output for BABEL-INSERT-EXECUTE.out
…cking

When TRY-CATCH is inside the procedure being executed by INSERT EXEC,
the INSERT EXEC context was incorrectly cleaned up, causing rows to be
lost (0 rows instead of expected rows).

This fix adds insert_exec_call_stack_depth to track the call stack depth
when INSERT EXEC was started. The new pltsql_insert_exec_should_cleanup_on_trycatch()
function uses this to determine if cleanup should occur:
- If TRY-CATCH is at the same level or higher than INSERT EXEC: clean up
- If TRY-CATCH is inside the procedure (deeper call stack): don't clean up

Changes:
- Add insert_exec_call_stack_depth variable to pl_exec-2.c
- Record call stack depth in pltsql_set_insert_exec_context_info()
- Reset call stack depth in pltsql_clear_insert_exec_context()
- Add pltsql_insert_exec_should_cleanup_on_trycatch() function
- Update iterative_exec.c to use the new function for cleanup decision
- Add function declaration in pltsql.h
The anchoring code (INSERT DEFAULT VALUES + DELETE) was originally added
to ensure temp table storage files survive subtransaction rollbacks.
However, testing confirmed this is unnecessary because:

1. PostgreSQL's SELECT INTO (used to create the temp table) already
   initializes the storage files properly
2. SPI_execute internally handles command counter increments
3. All comprehensive tests pass without the anchoring code

Removing this simplifies the code and eliminates:
- Unnecessary subtransaction overhead
- Potential issues with NOT NULL constraints on target tables
- Extra INSERT/DELETE operations on every INSERT EXEC

Tested with all 16 demo queries from midpoint review document and
comprehensive test scenarios - all pass correctly.
This commit fixes several issues with INSERT INTO ... EXEC functionality:

1. Transaction count mismatch handling (BABEL-1944):
   - Fixed insert_exec_had_error flag not being cleared between batches
   - Modified mismatch check to allow error propagation for INSERT-EXEC
     at trancount >= 2 (where COMMIT is allowed but mismatch should fire)
   - Added pltsql_insert_exec_set_error_flag(), pltsql_insert_exec_had_error(),
     and pltsql_insert_exec_clear_error_flag() functions

2. Type coercion for INSERT-EXEC (BABEL-INSERT-EXEC):
   - Modified insertexec_receive() to perform explicit type coercion when
     source and target column types differ (e.g., INT to VARCHAR)
   - SQL Server implicitly converts types during INSERT EXEC, but PostgreSQL's
     table_tuple_insert doesn't - this fix converts via text representation

3. IDENTITY_INSERT ON support:
   - flush_insert_exec_temp_table() now accepts both SPI_OK_INSERT and
     SPI_OK_INSERT_RETURNING return codes

4. NestedTranCount restoration fix:
   - Only restore NestedTranCount if INSERT EXEC incremented it (from 0 to 1)
   - If COMMIT was called inside INSERT EXEC (allowed at trancount > 1),
     the decrement persists so mismatch warning is generated

Test files:
- Added BABEL-INSERT-EXEC.sql comprehensive test
- Updated BABEL-1944.out expected output with correct error codes
This commit fixes the INSERT EXEC ownership chaining regression and several
related issues discovered during testing.

OWNERSHIP CHAINING FIX:
-----------------------
The main problem was that when a procedure containing INSERT EXEC calls
another procedure owned by the same user, the ownership chaining was not
working correctly, resulting in permission denied errors.

The fix involves three key changes:

1. Catalog-based temp table creation: Instead of using 'SELECT ... INTO ...
   FROM target_table' (which requires SELECT permission), we now query
   pg_attribute catalog to get column definitions. The catalog is readable
   by everyone, so this works with ownership chaining.

2. Security context switch for inner procedure execution: Before executing
   the inner procedure, we switch to the procedure owner's security context
   using SetUserIdAndSecContext(get_func_owner(estate->func->fn_oid), ...).
   This ensures the EXECUTE permission check uses the procedure owner's
   identity, which is required for ownership chaining.

3. Security context switch for flush INSERT: The flush operation (copying
   data from temp table to target table) also switches to the procedure
   owner's context to ensure INSERT permission is checked correctly.

TABLE VARIABLE AND TEMP TABLE HANDLING:
---------------------------------------
- Fixed table variable detection: Table variables start with '@', but the
  code only checked for '#' (temp tables). Now both are handled correctly.

- Fixed temp table detection in parser: Temp tables in the full_object_name
  branch were getting schema prefixes added (e.g., 'dbo.#temp'), which
  doesn't work. Now we check if the table name starts with '#' and skip
  the schema prefix.

- For temp tables and table variables, we use SELECT ... INTO (simpler and
  more reliable) since they're always owned by the current user.

COLUMN LIST HANDLING:
--------------------
When a column list is specified (e.g., 'INSERT INTO t (c, a) EXEC proc'),
the code was using 'SELECT column_list INTO temp FROM target WHERE 1=0',
which requires SELECT permission. Now for regular tables with a column
list, we parse the column list and query pg_attribute for just those
columns, then use CREATE TEMP TABLE.

TYPE COERCION IMPROVEMENTS:
--------------------------
- Fixed typmod handling: The type coercion now properly handles typmod
  differences (e.g., varchar(max) to varchar(10)) by using PostgreSQL's
  find_typmod_coercion_function.

- Improved coercion path handling: Uses find_coercion_pathway to properly
  handle all coercion types (FUNC, COERCEVIAIO, RELABELTYPE).

- Added clean_format_type_string helper: Removes ' without time zone'
  suffix from format_type() output to make it valid for CREATE TABLE.

CLEANUP AND ERROR HANDLING:
--------------------------
- Added pending temp table cleanup: If INSERT EXEC fails during type
  coercion, the temp table couldn't be dropped because SPI wasn't
  available in the error context. Now we set a pending drop flag that's
  checked at the start of the next INSERT EXEC.

- Added DROP TABLE protection: Prevents dropping the target table of an
  active INSERT EXEC (SQL Server error 556).

TEST RESULTS:
------------
All tests pass:
- BABEL-INSERT-EXEC, BABEL-INSERT-EXECUTE
- proc_ownership_chaining-vu-*
- BABEL-2999-vu-*, temp_table_rollback-vu-*
- datareader_datawriter-vu-*, BABEL-1944

Files modified:
- contrib/babelfishpg_tsql/src/pl_exec-2.c (main changes)
- contrib/babelfishpg_tsql/src/tsqlIface.cpp (parser fix)
- contrib/babelfishpg_tsql/src/hooks.c (DROP TABLE protection)
- contrib/babelfishpg_tsql/src/pltsql.h (new declarations)
- test/JDBC/expected/*.out (updated expected outputs)
When a statement-terminating error (like constraint violation SQL_ERROR_2627)
occurs outside of a TRY-CATCH block, we were incorrectly skipping the
transaction abort. This left afterTriggers.query_depth not reset to -1.

When the TDS protocol layer then called CommitTransactionCommand() in its
PG_CATCH block, AfterTriggerFireDeferred() was called which asserts
query_depth == -1, causing the server crash.

The fix ensures we only skip the abort for statement-terminating errors when
inside a TRY-CATCH block. Without TRY-CATCH, we need to abort the transaction
to properly clean up the trigger state.
…ERT EXEC in function check

1. Remove statement-terminating error handling that was causing server crash:
   - When skipping transaction abort for statement-terminating errors inside
     TRY-CATCH, the expression context cleanup was failing with assertion
     'context != CurrentMemoryContext' because the memory context was left
     in an inconsistent state
   - The statement-terminating error handling was added as part of INSERT EXEC
     work but caused issues with TRY-CATCH cleanup
   - Now only skip abort for INSERT EXEC (which has proper cleanup handling)

2. Add INSERT EXEC function check in tsqlIface.cpp:
   - INSERT EXEC is not allowed inside functions unless target is a table variable
   - The check was being bypassed due to early return in INSERT EXEC handling
   - Now properly throws error 'INSERT EXEC cannot be used within a function'

3. Improve memory context handling in INSERT EXEC skip_abort path:
   - Switch to estate->stmt_mcontext_parent instead of cur_ctxt
   - This ensures we're in a safe context that won't be deleted during cleanup
1. Fix 'no columns found for INSERT EXEC temp table' error:
   - The column name comparison in pg_attribute query was case-sensitive
   - Column names from user's query (e.g., 'DbFixedRole') were not being
     lowercased before comparison with lower(a.attname)
   - Now lowercase column names using downcase_identifier() before adding
     to the IN clause

2. Fix 'Only functions can be executed within a function' error:
   - INSERT EXEC into table variable is allowed inside T-SQL functions
   - The runtime check was blocking all procedure calls inside functions,
     including INSERT EXEC scenarios
   - Now skip the check when stmt->insert_exec is true, allowing INSERT EXEC
     into table variables while still blocking direct procedure calls
When an error occurs inside a nested EXECUTE statement, the database
context was not being properly restored to the original context before
the error was re-thrown. This caused the outer batch to see the wrong
database context after the error.

The fix adds database context restoration in the PG_CATCH block of
exec_stmt_exec_batch, ensuring that when an error occurs, the database
context is restored before the error is propagated up the call stack.

This fixes the usedb_inside_execute test which verifies that database
context is properly restored after errors in nested EXECUTE statements.
When INSERT EXEC is used with parameters containing double-quoted strings
(e.g., EXEC sp_columns_managed "master"), the double-quote to single-quote
conversion was not being applied because clear_rewritten_query_fragment()
was called before the mutator could run on the EXEC statement's expression.

This fix applies the PLtsql_expr_query_mutator to the EXEC statement's
expression before clearing the rewritten_query_fragment, ensuring that
double-quoted strings are properly converted to single-quoted strings
for PostgreSQL compatibility.

Without this fix, double-quoted strings like "master" would be interpreted
as column references instead of string literals, causing errors like
'column "master" does not exist'.

This fixes the BABEL-SP_COLUMNS_MANAGED-dep-vu-verify test failure.
Added error code 8164 (nested INSERT EXEC not allowed) to the expected
output of sys.fn_mapped_system_error_list() and updated the count from
167 to 168. This error mapping was added as part of the INSERT EXEC
ownership chaining fix.
This fix addresses SQL Server error 556: 'INSERT EXEC failed because the
stored procedure altered the schema of the target table.'

The issue was that our INSERT EXEC implementation didn't hold the target
table open during procedure execution. This meant PostgreSQL's
CheckTableNotInUse() couldn't detect when the procedure tried to ALTER
TABLE on the target, allowing the ALTER to succeed when it should fail.

Changes:
- Add insert_exec_target_rel and insert_exec_target_rel_oid globals to
  track the held target table relation
- Add pltsql_insert_exec_open_target_table() to open and hold the target
  table with RowExclusiveLock at INSERT EXEC start
- Add pltsql_insert_exec_close_target_table() to close the target table
  after flush completes or on error cleanup
- Call open_target_table after setting up INSERT EXEC context in all
  three code paths (exec_stmt_exec, exec_stmt_execsql, sp_executesql)
- Call close_target_table in error handlers and after successful flush

Benefits:
- Properly detects schema alterations during INSERT EXEC (error 556)
- Prevents concurrent modifications to the target table structure
- Maintains consistency with SQL Server behavior

Note: Temp tables and table variables (starting with # or @) are skipped
since they're session-local and can't be modified by other sessions.
Problem:
SQL Server raises error 556 ('INSERT EXEC failed because the stored procedure
altered the schema of the target table') when a procedure executed via INSERT
EXEC modifies the target table's schema (e.g., ALTER TABLE ADD COLUMN).
Babelfish was not detecting this scenario.

Previous Approaches (Failed):
1. Holding target table open with table_open() - Fixed 556_1 test but caused
   'relcache reference not owned by resource owner' errors in upgrade tests
   because Relation pointers cannot be held across subtransaction boundaries.

2. Using locks only with LockRelationOid() - Fixed upgrade tests but broke
   556_1 because CheckTableNotInUse() only checks reference count, not locks.

Solution (Schema Signature Approach):
Instead of holding the Relation pointer open, we now:
1. Capture the target table's 'schema signature' at INSERT EXEC start:
   - Column count (natts)
   - Column type OIDs (atttypids)
   - Column type modifiers (atttypmods)
2. Acquire RowExclusiveLock on the target table (released at transaction end)
3. Before flushing buffered data to the target, verify the schema signature
   hasn't changed
4. If schema changed, raise error 556 with the appropriate SQL Server message

Major Version Upgrade Handling:
In major version upgrade scenarios, table OIDs may change (tables are recreated
with different OIDs). The schema verification and lock release functions now
wrap table_open/UnlockRelationOid calls in PG_TRY blocks to gracefully handle
stale OIDs - if the relation cannot be opened, we skip the schema check and
let the actual INSERT validate the data.

This approach:
- Detects ALTER TABLE operations that change column count or types
- Avoids holding Relation pointers across subtransaction boundaries
- Works correctly with upgrade tests (no relcache reference issues)
- Handles stale OIDs gracefully in major version upgrade scenarios
- Properly handles temp tables and table variables (skips schema capture)

Implementation Details:
- Added InsertExecSchemaSignature struct to store column metadata
- Added pltsql_insert_exec_verify_schema() to check schema before flush
- Modified pltsql_insert_exec_open_target_table() to capture schema signature
- Modified pltsql_insert_exec_close_target_table() to free schema signature
- Modified flush_insert_exec_temp_table() to call verify and raise error 556
- Added error mapping for the new error message to map to SQL Server error 556
- Added PG_TRY/PG_CATCH blocks to handle stale OIDs in upgrade scenarios

Files Changed:
- contrib/babelfishpg_tsql/src/pl_exec-2.c: Schema signature implementation
- contrib/babelfishpg_tsql/src/pltsql.h: Added pltsql_insert_exec_verify_schema()
- contrib/babelfishpg_tds/error_mapping.txt: Added error 556 mapping
- test/JDBC/expected/TestErrorHelperFunctions.out: Updated expected output
- test/JDBC/expected/TestErrorHelperFunctionsUpgrade-vu-verify.out: Updated

Tests Verified:
- 556_1: Passed (schema change detection)
- BABEL-1944: Passed (upgrade test - no relcache issues)
- BABEL-INSERT-EXEC: Passed (general INSERT EXEC functionality)
- BABEL-INSERT-EXECUTE: Passed (INSERT EXECUTE functionality)
- TestErrorHelperFunctions: Passed (error mapping validation)
@monk0707 monk0707 force-pushed the dest_receiver_and_temp_table branch from 1bee46c to 112b039 Compare March 23, 2026 10:07
Add PG_TRY/PG_CATCH protection to LockRelationOid and table_open calls
in pltsql_insert_exec_open_target_table(). During major version upgrades,
table OIDs can become stale between RangeVarGetRelid and subsequent
operations. This fix gracefully handles such scenarios by catching errors
and skipping schema capture rather than propagating the error.

This addresses 'could not open relation with OID' errors seen in
BABEL-1944 during version upgrade CI tests.
@monk0707 monk0707 force-pushed the dest_receiver_and_temp_table branch 2 times, most recently from a09245a to 46759fc Compare March 24, 2026 07:32
…ing context

During major version upgrade tests, BABEL-1944 was failing with could not
open relation with OID errors. The root cause was that error cleanup paths
were calling pltsql_clear_insert_exec_context() WITHOUT first calling
pltsql_insert_exec_close_target_table(), leaving insert_exec_target_rel_oid
set to a stale OID.

Fix: Add pltsql_insert_exec_close_target_table() calls before
pltsql_clear_insert_exec_context() in all error cleanup paths:
- iterative_exec.c: No TRY-CATCH cleanup path in !IsTransactionBlockActive
- iterative_exec.c: TRY-CATCH cleanup path in exec_stmt_iterative
- pl_exec-2.c: exec_stmt_try_catch cleanup

This ensures the target table OID is properly cleared and the lock is
released before the INSERT EXEC context is cleared.
@monk0707 monk0707 force-pushed the dest_receiver_and_temp_table branch from 46759fc to 00823dd Compare March 24, 2026 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant