Skip to content

feat: ROWNUM Implementation #1000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
rophy wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: ROWNUM Implementation #1000

rophy wants to merge 3 commits into from

Conversation

@rophy
Copy link
Contributor

@rophy rophy commented Dec 3, 2025 •
edited by coderabbitai bot
Loading

Overview

ROWNUM is an Oracle pseudocolumn that assigns a unique number to each row as it is retrieved from a query. IvorySQL implements ROWNUM to provide Oracle compatibility.

Oracle ROWNUM Semantics

In Oracle:

  1. ROWNUM is assigned to each row before the WHERE clause is evaluated (for ROWNUM conditions)
  2. ROWNUM is assigned only to rows that pass non-ROWNUM filters
  3. ROWNUM values are sequential starting from 1
  4. Each query block (including UNION branches) has its own independent ROWNUM counter
  5. Correlated subqueries reset ROWNUM for each invocation

Key Oracle Behaviors

-- Returns exactly 5 rows
SELECT * FROM t WHERE ROWNUM <= 5;

-- Returns 0 rows (ROWNUM starts at 1, so first row has ROWNUM=1, fails ROWNUM > 5)
SELECT * FROM t WHERE ROWNUM > 5;

-- Returns 0 rows (first row has ROWNUM=1, fails ROWNUM = 2, no second row to check)
SELECT * FROM t WHERE ROWNUM = 2;

-- ROWNUM only assigned to rows passing id >= 5
SELECT ROWNUM, id FROM t WHERE id >= 5;  -- Returns ROWNUM 1,2,3... for id 5,6,7...

-- Each UNION branch has independent ROWNUM
SELECT ROWNUM, id FROM t WHERE id <= 3
UNION
SELECT ROWNUM, id FROM t WHERE id > 7;
-- Oracle: rn=1,1,2,2,3,3 for id=1,8,2,9,3,10

IvorySQL Implementation

Architecture

The ROWNUM counter is stored in the executor state (EState->es_rownum) and is managed during query execution.

Key Source Files

File Purpose
src/include/nodes/execnodes.h Defines es_rownum in EState, rownum_reset in SubqueryScanState
src/include/executor/execScan.h ROWNUM increment/revert logic in ExecScanExtended
src/backend/executor/execExprInterp.c ExecEvalRownum reads es_rownum
src/backend/executor/nodeSubqueryscan.c SubqueryScan ROWNUM reset logic
src/backend/executor/nodeSubplan.c Correlated subquery ROWNUM save/restore
src/backend/executor/execUtils.c es_rownum initialization

Execution Flow

Basic ROWNUM in WHERE Clause

For a query like SELECT * FROM t WHERE ROWNUM <= 5:

  1. The planner transforms ROWNUM <= N to a Limit node (optimization)
  2. Each row fetched by the scan increments es_rownum
  3. The Limit node stops after N rows

ROWNUM with Non-ROWNUM Filters

For a query like SELECT ROWNUM, id FROM t WHERE id >= 5:

ExecScanExtended:
1. Fetch row from table
2. Pre-increment es_rownum (tentative assignment)
3. Evaluate qual (id >= 5)
4. If qual passes:
   - Keep the increment
   - Project tuple (ROWNUM reads es_rownum)
   - Return row
5. If qual fails:
   - Revert es_rownum (decrement)
   - Try next row

This ensures ROWNUM is only assigned to rows that pass non-ROWNUM filters.

ROWNUM with ROWNUM Conditions

For a query like SELECT * FROM t WHERE ROWNUM <= 5 AND id > 2:

  1. es_rownum is pre-incremented before qual check
  2. Both conditions are evaluated together
  3. If id > 2 fails, es_rownum is reverted
  4. If ROWNUM <= 5 fails (after 5 rows), execution continues but all subsequent rows fail

SubqueryScan with ORDER BY

For a query like SELECT ROWNUM FROM (SELECT * FROM t ORDER BY value DESC) sub:

Plan Structure:
SubqueryScan (projects ROWNUM)
  -> Sort (ORDER BY value DESC)
       -> SeqScan on t

Execution:
1. Sort buffers all tuples from SeqScan (SeqScan increments es_rownum)
2. SubqueryNext is called for first tuple
3. On first call, es_rownum is reset to 0 (via rownum_reset flag)
4. SubqueryScan increments es_rownum for each tuple it returns
5. ROWNUM projection reads the correct value (1, 2, 3...)

Correlated Subqueries

For a query like:

SELECT id, (SELECT ROWNUM FROM t2 WHERE t2.id = t1.id) as rn FROM t1;
Execution:
1. Outer scan fetches row from t1
2. ExecSubPlan is called for the scalar subquery
3. ExecSubPlan saves es_rownum, resets to 0
4. Inner scan executes (increments es_rownum for its rows)
5. ExecSubPlan restores es_rownum
6. This ensures each subquery invocation starts fresh at ROWNUM=1

Code Changes Summary

execScan.h - Pre-increment with Revert

/* Pre-increment ROWNUM before qual check */
if (node->ps.state)
    node->ps.state->es_rownum++;

if (qual == NULL || ExecQual(qual, econtext))
{
    /* Row passed - keep increment, project and return */
    ...
}
else
{
    /* Row failed - revert increment */
    if (node->ps.state)
        node->ps.state->es_rownum--;
    ...
}

nodeSubqueryscan.c - Reset on First Tuple

typedef struct SubqueryScanState
{
    ScanState   ss;
    PlanState  *subplan;
    bool        rownum_reset;  /* has ROWNUM been reset for this scan? */
} SubqueryScanState;

static TupleTableSlot *
SubqueryNext(SubqueryScanState *node)
{
    bool first_call = !node->rownum_reset;
    if (first_call)
        node->rownum_reset = true;

    slot = ExecProcNode(node->subplan);

    /* Reset after first ExecProcNode to ignore inner plan's increments */
    if (first_call)
        node->ss.ps.state->es_rownum = 0;

    return slot;
}

nodeSubplan.c - Save/Restore for Correlated Subqueries

Datum
ExecSubPlan(SubPlanState *node, ExprContext *econtext, bool *isNull)
{
    EState *estate = node->planstate->state;
    int64 save_rownum = estate->es_rownum;

    estate->es_rownum = 0;  /* Reset for subquery */

    /* Execute subplan */
    ...

    estate->es_rownum = save_rownum;  /* Restore */
    return retval;
}

Oracle Compatibility Test Results

All tests verified against Oracle Database 23.26 Free container.

Passing Tests (Match Oracle)

Test Query Pattern Result
Basic ROWNUM <= N WHERE ROWNUM <= 5 5 rows
ROWNUM = 1 WHERE ROWNUM = 1 1 row
ROWNUM < N WHERE ROWNUM < 4 3 rows
ROWNUM in SELECT SELECT ROWNUM, id WHERE ROWNUM <= 3 ROWNUM 1,2,3
Top-N pattern SELECT * FROM (... ORDER BY) WHERE ROWNUM <= 3 Top 3 rows
ROWNUM > 0 WHERE ROWNUM > 0 All rows (tautology)
ROWNUM > N WHERE ROWNUM > 5 0 rows
ROWNUM = N (N>1) WHERE ROWNUM = 2 0 rows
ROWNUM with filter WHERE id >= 5 ROWNUM 1-6 for id 5-10
Combined conditions WHERE ROWNUM <= 3 AND id >= 5 ROWNUM 1-3 for id 5-7
COUNT with ROWNUM SELECT COUNT(*) WHERE ROWNUM <= 5 5
ORDER BY with ROWNUM WHERE ROWNUM <= 5 ORDER BY value First 5, then sorted
Correlated subquery (SELECT ROWNUM FROM sub WHERE ...) Resets to 1 each time
Nested subqueries Multiple ROWNUM levels Correct at each level

UNION/UNION ALL ROWNUM Support

Oracle Behavior:

SELECT ROWNUM, id FROM t WHERE id <= 3
UNION ALL
SELECT ROWNUM, id FROM t WHERE id > 7;

-- Result: Each branch has independent ROWNUM
-- rn=1 id=1, rn=2 id=2, rn=3 id=3, rn=1 id=8, rn=2 id=9, rn=3 id=10

IvorySQL matches this behavior by resetting es_rownum when the executor switches between UNION branches.

Implementation

The UNION ROWNUM fix adds an is_union flag to both Append and MergeAppend nodes:

  • UNION ALL uses Append nodes
  • UNION (with deduplication) uses MergeAppend + Unique nodes

Planner changes:

  1. The is_union flag is set on AppendPath/MergeAppendPath nodes created for UNION operations
  2. Simple UNION ALL queries are "pulled up" via pull_up_simple_union_all and detected in add_paths_to_append_rel by checking if all child rels have rtekind == RTE_SUBQUERY

Executor changes:

  1. ExecAppend: Resets es_rownum when switching to a new subplan (during iteration)
  2. ExecMergeAppend: Resets es_rownum before each child in the initialization loop (Sort nodes buffer all input upfront)

Key files:

File Change
src/include/nodes/plannodes.h Added is_union to Append and MergeAppend structs
src/include/nodes/pathnodes.h Added is_union to AppendPath and MergeAppendPath structs
src/include/nodes/execnodes.h Added as_is_union to AppendState, ms_is_union to MergeAppendState
src/backend/optimizer/util/pathnode.c Initialize is_union=false in create functions
src/backend/optimizer/prep/prepunion.c Sets is_union=true in generate_union_paths
src/backend/optimizer/path/allpaths.c Sets is_union=true for UNION ALL appendrels
src/backend/optimizer/plan/createplan.c Copies is_union from path to plan
src/backend/executor/nodeAppend.c Resets es_rownum on branch switch
src/backend/executor/nodeMergeAppend.c Resets es_rownum before each child in init

MergeAppend Specifics

MergeAppend requires special handling because it initializes all children during ExecInitMergeAppend:

/* In ExecMergeAppend initialization loop */
while ((i = bms_next_member(node->ms_valid_subplans, i)) >= 0)
{
    /* Reset ROWNUM before each branch executes */
    if (node->ms_is_union)
        node->ps.state->es_rownum = 0;

    /* Each child's Sort buffers ALL tuples from its scan */
    node->ms_slots[i] = ExecProcNode(node->mergeplans[i]);
    ...
}

Each Sort child buffers all its input tuples during initialization, so ROWNUM must be reset before each child starts.

Performance Considerations

  1. Pre-increment/Revert Overhead: Each filtered row requires an increment and a decrement. This is minimal overhead (two integer operations).

  2. Optimizer Transformations: The planner transforms simple ROWNUM conditions to Limit nodes, avoiding the need for runtime ROWNUM checking in many cases.

  3. SubqueryScan Reset: The rownum_reset flag ensures the reset only happens once per scan, not per tuple.

Testing

The ROWNUM implementation is tested via:

  • src/oracle_test/regress/sql/rownum.sql - Comprehensive test cases
  • src/oracle_test/regress/expected/rownum.out - Expected output

Run tests with:

cd src/oracle_test/regress
make oracle-check

DML with ROWNUM

IvorySQL supports ROWNUM in DELETE and UPDATE statements, matching Oracle behavior:

-- Delete first 2 rows
DELETE FROM t WHERE ROWNUM <= 2;

-- Update first 3 rows
UPDATE t SET val = 'updated' WHERE ROWNUM <= 3;

Known Limitations

The following Oracle ROWNUM behaviors are not yet implemented in IvorySQL:

INTERSECT/EXCEPT ROWNUM Reset

Issue: #23

Oracle resets ROWNUM independently for each side of INTERSECT/EXCEPT operations. IvorySQL shares the counter across both sides.

-- Oracle: Each side produces (1,1), (2,2), (3,3) -> intersection = 3 rows
-- IvorySQL: Left=(1,1),(2,2),(3,3), Right=(4,1),(5,2),(6,3) -> intersection = 0 rows
SELECT ROWNUM, id FROM (SELECT 1 as id FROM dual UNION ALL SELECT 2 FROM dual UNION ALL SELECT 3 FROM dual)
INTERSECT
SELECT ROWNUM, id FROM (SELECT 1 as id FROM dual UNION ALL SELECT 2 FROM dual UNION ALL SELECT 3 FROM dual);

Technical: INTERSECT/EXCEPT use SetOp nodes with two child paths. The fix would require resetting es_rownum before executing the second child.

LATERAL Subquery ROWNUM Reset

Issue: #24

Oracle resets ROWNUM for each outer row in LATERAL/CROSS APPLY subqueries. IvorySQL continues the counter across outer rows.

-- Oracle: (1,1), (2,1), (2,2), (3,1), (3,2), (3,3) - resets for each outer row
-- IvorySQL: (1,1), (2,2), (2,3), (3,4), (3,5), (3,6) - counter continues
SELECT t.id, sub.rn
FROM t,
LATERAL (SELECT ROWNUM as rn FROM t2 WHERE t2.id <= t.id) sub;

Technical: LATERAL subqueries are executed via SubqueryScan nodes. The existing reset logic may need to be extended for LATERAL joins.

Future Work

  1. INTERSECT/EXCEPT ROWNUM reset: Implement ROWNUM reset in SetOp executor (Issue ROWNUM: INTERSECT/EXCEPT shares counter across both sides (differs from Oracle) rophy/IvorySQL#23)
  2. LATERAL subquery ROWNUM reset: Extend SubqueryScan reset logic for LATERAL joins (Issue ROWNUM: LATERAL subquery doesn't reset counter for each outer row (differs from Oracle) rophy/IvorySQL#24)
  3. ROWNUM optimization: Additional planner optimizations for complex ROWNUM patterns
  4. Parallel query support: Ensure ROWNUM works correctly with parallel execution

Summary by CodeRabbit

  • New Features

    • Adds Oracle-compatible ROWNUM pseudocolumn: parsing, planner/optimizer support, expression evaluation, and deparsing.
    • Transforms applicable ROWNUM predicates into LIMIT where safe.
    • Ensures ROWNUM is evaluated/materialized consistently across projections, subqueries, and set-operation branches (UNION/UNION ALL).

  • Tests

    • Added extensive SQL and PL/iSQL test suites exercising ROWNUM behavior across many query patterns.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025 •
edited
Loading

Walkthrough

Adds Oracle ROWNUM support across parser, planner, and executor: new RownumExpr node and EEOP_ROWNUM opcode; optimizer transforms/rewrites ROWNUM predicates (including LIMIT conversion); executor tracks and manages es_rownum, resets across subplans/UNION branches, and materializes projections when needed.

Changes

Cohort / File(s) Summary
Core node/type & headers
src/include/nodes/primnodes.h, src/include/executor/execExpr.h, src/include/nodes/execnodes.h, src/include/nodes/pathnodes.h, src/include/nodes/plannodes.h		 Add RownumExpr node type, EEOP_ROWNUM opcode and ExecEvalRownum declaration; add EState.es_rownum, ProjectionInfo.pi_needsMaterialization, and is_union/rownum-related fields to path/plan/exec structs.
Parser / Keywords
src/backend/oracle_parser/ora_gram.y, src/include/oracle_parser/ora_kwlist.h, src/backend/parser/parse_expr.c		 Register ROWNUM as unreserved/bare-label keyword; transform unqualified rownum refs into RownumExpr in Oracle compatibility mode.
Expression & utilities
src/backend/nodes/nodeFuncs.c, src/backend/optimizer/util/clauses.c, src/backend/utils/adt/ruleutils.c		 Recognize RownumExpr in type/collation/walkers/mutators; mark it mutable/volatile; support deparse to the literal ROWNUM.
Optimizer — planning & path flags
src/backend/optimizer/path/allpaths.c, src/backend/optimizer/prep/prepunion.c, src/backend/optimizer/util/pathnode.c, src/backend/optimizer/plan/createplan.c, src/backend/optimizer/plan/planner.c		 Propagate is_union on Append/MergeAppend paths/plans; implement helpers to detect/collect/replace ROWNUM in tlists and restructure Sort+ROWNUM cases; add transform_rownum_to_limit() to convert ROWNUM predicates into LIMIT where safe.
Executor — eval & state
src/backend/executor/execExpr.c, src/backend/executor/execExprInterp.c, src/backend/executor/execUtils.c, src/include/executor/execScan.h, src/include/executor/execExpr.h		 Implement ExecEvalRownum, add EEOP_ROWNUM handling in interpreter, initialize es_rownum, increment/reset es_rownum pre-qual, revert on qual-fail, and support projection materialization when ROWNUM present.
Executor — nodes and subplan handling
src/backend/executor/nodeAppend.c, src/backend/executor/nodeMergeAppend.c, src/backend/executor/nodeResult.c, src/backend/executor/nodeSubplan.c, src/backend/executor/nodeSubqueryscan.c		 Propagate is_union into state and reset es_rownum between UNION branches; save/restore and reset es_rownum around subplan/init/correlated evaluations; materialize Result projections when required; track per-SubqueryScan rownum_reset.
Tests & regressions
src/oracle_test/regress/parallel_schedule, src/oracle_test/regress/sql/rownum.sql, src/pl/plisql/src/Makefile, src/pl/plisql/src/sql/plisql_rownum.sql		 Add comprehensive SQL and PL/iSQL regression tests and test scheduling entries covering ROWNUM behavior across many scenarios.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant Parser
    participant Optimizer
    participant Executor
    participant EState as EState(es_rownum)

    Client->>Parser: Send query with ROWNUM
    Parser->>Parser: Build RownumExpr node
    Parser->>Optimizer: Pass query tree

    Optimizer->>Optimizer: detect ROWNUM predicate
    Optimizer->>Optimizer: transform_rownum_to_limit() or rewrite projection
    Optimizer->>Executor: Emit plan (may include LIMIT / is_union flags)

    Executor->>EState: Initialize es_rownum=0
    loop per input tuple
        Executor->>EState: es_rownum++ (pre-qual)
        Executor->>Executor: Evaluate qual
        alt qual passes
            Executor->>Executor: ExecProject()
            Executor->>Executor: If pi_needsMaterialization: ExecMaterializeSlot()
            Executor->>Client: return row
        else qual fails
            Executor->>EState: es_rownum-- (revert)
        end
    end
Loading 
sequenceDiagram
    participant OuterExec
    participant Subplan
    participant EState as EState(es_rownum)

    OuterExec->>EState: save es_rownum
    OuterExec->>EState: reset es_rownum=0
    OuterExec->>Subplan: execute subplan (correlated/init)
    loop subplan tuples
        Subplan->>EState: es_rownum++ / evaluate / emit
    end
    OuterExec->>EState: restore es_rownum
Loading 
sequenceDiagram
    participant AppendExec
    participant BranchA
    participant BranchB
    participant EState as EState(es_rownum)

    AppendExec->>AppendExec: detect is_union
    AppendExec->>EState: reset es_rownum=0
    AppendExec->>BranchA: scan branch A
    loop A tuples
        BranchA->>EState: es_rownum++ / emit
    end
    AppendExec->>EState: reset es_rownum=0
    AppendExec->>BranchB: scan branch B
    loop B tuples
        BranchB->>EState: es_rownum++ / emit
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Areas needing extra attention:

  • Optimizer transformations and Sort+ROWNUM restructuring (createplan.c, planner.c) — correctness of rewrites and safety checks.
  • Executor state save/restore and es_rownum lifecycle across all code paths, errors, and rescans (nodeSubplan.c, nodeSubqueryscan.c, execScan.h).
  • UNION branch handling and parallel/partial path interactions (nodeAppend.c, nodeMergeAppend.c, path/allpaths.c, prepunion.c`).
  • Materialization triggers (execExpr.c, nodeResult.c) — ensure pi_needsMaterialization is set/observed consistently.

Suggested reviewers

  • NotHimmel
  • jiaoshuntian
  • OreoYang

Poem

🐰 I counted rows with nimble feet,
ROWNUM glows when queries meet,
Branches reset, subcalls tidy and neat,
A rabbit’s hop makes numbers sweet.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'feat: ROWNUM Implementation' clearly summarizes the main change—implementing Oracle-compatible ROWNUM semantics—which is the primary focus across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 84.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot reviewed Dec 3, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (10)
src/backend/optimizer/util/clauses.c (1)

464-468: ROWNUM volatility classification looks right, but COPY helper ignores it

Marking RownumExpr as non-immutable in contain_mutable_functions_walker and volatile in contain_volatile_functions_walker is consistent with its per-row behavior. However, contain_volatile_functions_not_nextval_walker() does not recognize RownumExpr, so expressions that are “volatile” only because of ROWNUM will be treated as non‑volatile in COPY’s special check. If you want COPY’s volatility checks to see ROWNUM as volatile (which seems safest), consider adding a parallel IsA(node, RownumExpr) branch there as well.

Also applies to: 595-599, 715-751

src/backend/optimizer/util/pathnode.c (1)

1576-1576: Good initialization; minor whitespace inconsistency.

The default initialization is correct and mirrors the pattern in create_append_path. However, line 1363 uses a tab before the comment while this line uses spaces. Consider standardizing to match the project's style.

src/backend/utils/adt/ruleutils.c (1)

10841-10844: Deparsing T_RownumExpr as ROWNUM is correct and minimal

The dedicated T_RownumExpr branch that emits the literal ROWNUM cleanly supports Oracle-style pseudocolumn deparsing and avoids colliding with user columns (which still go through the Var path). Behavior is straightforward and looks correct. If you ever see this node outside Oracle-compatible mode, an assertion or mode check here could help catch misuse, but it's not strictly necessary.

src/backend/nodes/nodeFuncs.c (1)

1423-1856: Optional: add T_RownumExpr handling in exprLocation for better diagnostics

Right now exprLocation falls through to the default case for T_RownumExpr, returning -1. If RownumExpr carries a location field in primnodes.h, adding a dedicated case (similar to T_NextValueExpr/others that just expose their location) would give more precise error locations when ROWNUM appears in reported expressions.

src/backend/executor/nodeAppend.c (1)

293-295: Per-branch ROWNUM reset for UNION via as_is_union looks good; watch async-only UNION cases

  • Copying node->is_union into appendstate->as_is_union in ExecInitAppend cleanly separates UNION vs non‑UNION Appends without impacting inheritance/appendrel use cases.
  • In ExecAppend, resetting es_rownum to 0 immediately after choose_next_subplan advances to a new subplan gives each UNION branch its own ROWNUM sequence in the synchronous case, matching the stated Oracle‑style semantics.

One thing to be aware of: the reset hook is tied to choose_next_subplan, i.e., synchronous branch changes. If an Append used as a UNION aggregator ever ends up with only async‑capable children (or yields most rows via the async path), es_rownum will not be reset between those async branches. If such plans are possible with ROWNUM, you may want to either (a) disable async Append for UNION queries involving ROWNUM, or (b) add equivalent per‑branch reset logic on the async path.

Also applies to: 393-399

src/backend/optimizer/path/allpaths.c (1)

1337-1362: UNION detection and is_union propagation are partial; parameterized/ordered appends remain untagged

The is_union_all heuristic:

is_union_all = true;
foreach (childrel in live_childrels)
    if (childrel->rtekind != RTE_SUBQUERY)
        is_union_all = false;

is a reasonable way to distinguish UNION/UNION ALL append relations from inheritance/partitioning (which use RTE_RELATION children).

You then set appendpath->is_union = is_union_all for:

  • The main unparameterized Append path,
  • The startup-optimized Append path,
  • The unordered partial Append path, and
  • The parallel‑aware mixed partial/non‑partial Append path.

Two potential gaps:

  1. Parameterized AppendPaths built later in the function:
create_append_path(root, rel, subpaths, NIL, NIL, required_outer, 0, false, -1);

are not tagged with is_union, so a UNION appendrel that ends up parameterized would not trigger per‑branch ROWNUM reset in the executor.

  1. Ordered Append paths created in generate_orderedappend_paths (for appendrels that can use Append instead of MergeAppend) are also not tagged, even when they originate from a UNION appendrel. If such a path is chosen for a UNION in Oracle mode, its executor state would see as_is_union == false.

If UNION/UNION ALL appendrels can never reach these code paths in practice, it’d be helpful to document that invariant. Otherwise, consider propagating is_union_all to all create_append_path calls associated with the same appendrel, including parameterized and ordered variants, to avoid subtle mode‑dependent differences in ROWNUM behavior.

Also applies to: 1547-1555, 1559-1568, 1607-1614, 1639-1665

src/backend/executor/execExprInterp.c (2)

3354-3359: Simplify redundant null check.

The check if (planstate) on line 3357 is redundant since planstate was just assigned from state->parent, which was already verified non-NULL by the enclosing if (state && state->parent) condition.

 	/* Safely get the PlanState and EState */
 	if (state && state->parent)
 	{
 		planstate = state->parent;
-		if (planstate)
-			estate = planstate->state;
+		estate = planstate->state;
 	}

3346-3371: Consider adding an assertion for defensive coding.

The function correctly handles the case where estate is NULL by defaulting to 1. However, since ROWNUM evaluation requires proper executor state setup, consider adding a debug assertion to catch unexpected scenarios where state->parent is NULL during normal query execution.

 void
 ExecEvalRownum(ExprState *state, ExprEvalStep *op)
 {
 	PlanState  *planstate;
 	EState	   *estate = NULL;
 	int64		rownum_value = 1;  /* default */

+	Assert(state != NULL);
+
 	/* Safely get the PlanState and EState */
 	if (state && state->parent)
 	{

This makes the expectation explicit while preserving the graceful fallback for edge cases.

src/backend/optimizer/plan/createplan.c (2)

2084-2228: ROWNUM-before-Sort rewrite is sound; a few refinements suggested

The transformation that pushes RownumExpr evaluation below Sort/IncrementalSort and replaces them with Vars in the projection tlist correctly achieves “ROWNUM assigned before ORDER BY” semantics and handles multiple/nested ROWNUM occurrences. A couple of points to tighten:

  • The synthetic ROWNUM columns added to the sort input tlist and the sort’s own tlist are created with resjunk = false. Since these are purely internal carriers, consider marking them resjunk = true in both places to avoid them ever being treated as user-visible or accidentally included in any “physical tlist” decisions.
  • rownum_type is hard-coded as INT8OID. If the declared SQL type of ROWNUM in IvorySQL ever differs, this will silently produce Vars of a different type than the original RownumExpr. It would be safer to use the same type as the expression (e.g., via a dedicated ROWNUM type constant or exprType on a representative RownumExpr) so planner and executor stay consistent.
  • count_rownum_exprs is called once per TargetEntry twice (once when building the scan-input tlist, once when building rownum_vars), and replace_rownum_expr_mutator also walks the same trees. Not a correctness issue, but you could cache per-TLE counts or store the total length in the context (instead of recomputing list_length(context->rownum_vars) inside the mutator) if you want to avoid redundant tree walks; this is mostly micro-optimization.

Overall, the structure and ordering look correct; these suggestions are mainly for clarity and robustness.

7575-7683: ROWNUM helpers integrate cleanly; align walker signature and clarify intent

The new helpers correctly reuse expression_tree_walker/mutator to (a) detect ROWNUM, (b) count occurrences, and (c) substitute them with Vars:

  • For count_rownum_exprs_walker, the second parameter is currently int *count but it’s passed as the callback to expression_tree_walker, whose convention is a void * context. It would be cleaner (and more in line with existing walkers in this file) to declare it as static bool count_rownum_exprs_walker(Node *node, void *context) and cast context to int * inside. That avoids any prototype/ABI mismatches under stricter compilers.
  • contain_rownum_expr_walker and collect_rownum_exprs are straightforward and match how other contain-XYZ and collect-XYZ helpers are written; no functional issues there.

These are small API-alignment improvements; the current code should still work, but tightening the signature will make it more future-proof.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7261f1c and 7a0723c.

⛔ Files ignored due to path filters (2)
  • src/oracle_test/regress/expected/rownum.out is excluded by !**/*.out
  • src/pl/plisql/src/expected/plisql_rownum.out is excluded by !**/*.out
📒 Files selected for processing (29)
  • src/backend/executor/execExpr.c (3 hunks)
  • src/backend/executor/execExprInterp.c (3 hunks)
  • src/backend/executor/execUtils.c (1 hunks)
  • src/backend/executor/nodeAppend.c (2 hunks)
  • src/backend/executor/nodeMergeAppend.c (2 hunks)
  • src/backend/executor/nodeResult.c (1 hunks)
  • src/backend/executor/nodeSubplan.c (6 hunks)
  • src/backend/executor/nodeSubqueryscan.c (3 hunks)
  • src/backend/nodes/nodeFuncs.c (5 hunks)
  • src/backend/optimizer/path/allpaths.c (4 hunks)
  • src/backend/optimizer/plan/createplan.c (5 hunks)
  • src/backend/optimizer/plan/planner.c (4 hunks)
  • src/backend/optimizer/prep/prepunion.c (3 hunks)
  • src/backend/optimizer/util/clauses.c (2 hunks)
  • src/backend/optimizer/util/pathnode.c (2 hunks)
  • src/backend/oracle_parser/ora_gram.y (3 hunks)
  • src/backend/parser/parse_expr.c (1 hunks)
  • src/backend/utils/adt/ruleutils.c (2 hunks)
  • src/include/executor/execExpr.h (2 hunks)
  • src/include/executor/execScan.h (3 hunks)
  • src/include/nodes/execnodes.h (5 hunks)
  • src/include/nodes/pathnodes.h (2 hunks)
  • src/include/nodes/plannodes.h (2 hunks)
  • src/include/nodes/primnodes.h (1 hunks)
  • src/include/oracle_parser/ora_kwlist.h (1 hunks)
  • src/oracle_test/regress/parallel_schedule (1 hunks)
  • src/oracle_test/regress/sql/rownum.sql (1 hunks)
  • src/pl/plisql/src/Makefile (1 hunks)
  • src/pl/plisql/src/sql/plisql_rownum.sql (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T10:43:25.257Z 
Learnt from: syasirs
Repo: IvorySQL/IvorySQL PR: 858
File: src/Makefile.global.in:513-514
Timestamp: 2025-09-02T10:43:25.257Z
Learning: In IvorySQL Makefile.global.in, $(wildcard t/oracle/*.pl) is used instead of t/oracle/*.pl in Oracle TAP test discovery because the t/oracle directory is optional and may not exist in most builds. The wildcard function safely returns empty string when the directory is missing, preventing shell expansion errors.

Applied to files:

  • src/pl/plisql/src/Makefile
🧬 Code graph analysis (7)
src/include/executor/execExpr.h (1)
src/backend/executor/execExprInterp.c (2)
  • EEOP_ROWNUM (1597-1607)
  • ExecEvalRownum (3346-3371)
src/backend/executor/nodeResult.c (2)
src/include/executor/executor.h (1)
  • ExecProject (479-503)
src/include/executor/tuptable.h (1)
  • ExecMaterializeSlot (475-479)
src/backend/optimizer/path/allpaths.c (1)
src/backend/optimizer/util/pathnode.c (1)
  • create_append_path (1299-1420)
src/backend/executor/nodeSubqueryscan.c (1)
src/include/executor/executor.h (1)
  • ExecProcNode (310-317)
src/backend/utils/adt/ruleutils.c (1)
src/common/stringinfo.c (1)
  • appendStringInfoString (229-233)
src/backend/optimizer/plan/createplan.c (2)
src/include/nodes/pg_list.h (2)
  • list_length (151-155)
  • list_nth (298-303)
src/backend/nodes/makefuncs.c (2)
  • makeTargetEntry (289-313)
  • makeVar (66-100)
src/backend/executor/execExpr.c (1)
src/backend/executor/execExprInterp.c (1)
  • EEOP_ROWNUM (1597-1607)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: meson_build (ubuntu-latest)
  • GitHub Check: oracle_regression (ubuntu-latest)
  • GitHub Check: pg_regression (ubuntu-latest)
  • GitHub Check: build (ubuntu-latest)
  • GitHub Check: oracle_pg_regression (ubuntu-latest)
  • GitHub Check: contrib_regression
🔇 Additional comments (41)
src/pl/plisql/src/Makefile (1)

57-61: Adding plisql_rownum to REGRESS suite looks consistent

The new plisql_rownum test is wired into the existing PL/iSQL regression list right after plisql_exception, which keeps naming and grouping consistent. No issues from the Makefile side.

src/oracle_test/regress/sql/rownum.sql (1)

1-659: ROWNUM regression coverage is broad and well‑structured

This script exercises ROWNUM across the key semantic areas you’re modifying (filters, ORDER BY, aggregates, joins, set ops, correlated subqueries, DML, and known INTERSECT/EXCEPT/LATERAL limitations), with clean setup/teardown of helper tables. From the test‑harness perspective the additions look consistent and complete.

src/oracle_test/regress/parallel_schedule (1)

162-167: Scheduling rownum as its own test group is reasonable

Placing the new rownum regression after existing Oracle‑compat tests and running it alone is a safe, simple choice; it avoids any subtle interference with earlier tests and matches how other Oracle‑specific suites are scheduled.

src/backend/executor/execUtils.c (1)

165-167: es_rownum initialization matches ExecEvalRownum expectations

Initializing estate->es_rownum to 0 at creation is consistent with ExecEvalRownum’s “default to 1 unless the executor has started counting” logic and avoids any dependence on uninitialized state. No issues here.

src/include/executor/execExpr.h (1)

195-205: EEOP_ROWNUM opcode and ExecEvalRownum declaration are consistent

The new EEOP_ROWNUM opcode is added in the same “special‑purpose expression types” block as EEOP_NEXTVALUEEXPR, and the ExecEvalRownum(ExprState *, ExprEvalStep *) prototype matches the interpreter case that calls it without an ExprContext. From the header’s perspective, the wiring looks correct and doesn’t affect ExprEvalStep layout.

Also applies to: 888-889

src/backend/optimizer/util/clauses.c (1)

865-903: ROWNUM should probably be treated as parallel‑unsafe (or at least restricted)

max_parallel_hazard_walker() currently special‑cases NextValueExpr as PROPARALLEL_UNSAFE, but does not mention RownumExpr. If RownumExpr is implemented to access session state (such as a global counter from EState like ExecEvalRownum does), allowing the planner to treat ROWNUM expressions as parallel‑safe could yield incorrect per‑query row numbering in parallel plans. I'd recommend classifying RownumExpr here as at least PROPARALLEL_RESTRICTED, and probably PROPARALLEL_UNSAFE, to prevent parallelization of queries that reference ROWNUM until parallel semantics are explicitly implemented.

src/backend/optimizer/util/pathnode.c (1)

1363-1363: LGTM: Proper default initialization for UNION path handling.

The default false initialization is appropriate, and the comment clearly documents that callers will override this when building UNION paths for ROWNUM reset logic.

src/include/oracle_parser/ora_kwlist.h (1)

461-461: LGTM: Correct keyword addition for ROWNUM support.

The keyword entry is properly formatted and alphabetically positioned between "rowid" and "rows". The categorization as UNRESERVED_KEYWORD with BARE_LABEL is appropriate and consistent with similar keywords in the list.

src/backend/parser/parse_expr.c (2)

689-703: ROWNUM ColumnRef detection and hook interaction

Recognizing unqualified rownum only in DB_ORACLE mode and only as a bare, case‑insensitive identifier is consistent with Oracle semantics and placed correctly after p_pre_columnref_hook. Note that this path returns a RownumExpr directly and therefore never invokes p_post_columnref_hook; if any extension expects to see or override ROWNUM via the post‑hook, we’d need to set node and fall through instead of an early return.

976-1006: Condition for raising “missing column” errors now depends on parser mode

The new guard

if ((compatible_db == ORA_PARSER && !missing_ok) ||
    compatible_db == PG_PARSER)

preserves the previous missing_ok behavior for the Oracle parser while ensuring PG parser mode still always reports undefined columns. That looks sound given transformColumnRefOrFunCall is Oracle‑only, but it’s worth double‑checking that there is no PG‑parser caller that intentionally passes missing_ok = true and relies on suppressing these errors.

src/include/nodes/pathnodes.h (1)

2070-2071: Initialize and propagate is_union consistently for Append/MergeAppend paths

Adding bool is_union to AppendPath and MergeAppendPath is a reasonable way to flag UNION/UNION ALL branches for executor behavior (e.g., ROWNUM resets). Please ensure every constructor/call site that builds these paths explicitly sets is_union (or clearly relies on makeNode() zero‑initialization) and that any path copying/cloning logic propagates it correctly; otherwise UNION and non‑UNION appends could be misclassified.

Also applies to: 2093-2094

src/include/nodes/plannodes.h (2)

362-367: Append.is_union flag is a reasonable hook for UNION‑scoped ROWNUM handling

Appending a dedicated is_union flag at the end of Append is structurally safe and the comment clearly documents its ROWNUM/UNION purpose; assuming the planner sets this only for UNION/UNION ALL append paths, this looks correct and non‑intrusive.

407-412: MergeAppend.is_union mirrors Append semantics appropriately

Adding is_union to MergeAppend with matching semantics (per‑branch ROWNUM reset for UNION/UNION ALL) is consistent with the Append change and gives the executor the information it needs without impacting other MergeAppend uses.

src/backend/executor/nodeResult.c (1)

135-146: ROWNUM‑aware materialization in ExecResult is consistent with executor patterns

Capturing the projection into result and materializing only when pi_needsMaterialization is set follows existing slot semantics and ensures ROWNUM (and similar expressions) become stable values without penalizing other projections; the change is localized and looks correct.

src/include/nodes/primnodes.h (1)

2150-2163: RownumExpr node definition is minimal and appropriate

Defining a dedicated RownumExpr as an Expr wrapper with just a location field cleanly models the pseudocolumn; it keeps all behavioral logic in planner/executor while giving the parser and expression machinery a distinct node tag to key off of.

src/backend/utils/adt/ruleutils.c (1)

9332-9351: ROWNUM treated correctly as a simple expression in isSimpleNode

Adding T_RownumExpr to the simple/function-like node set makes ROWNUM participate cleanly in pretty-print parenthesis decisions and avoids unnecessary wrapping. This is consistent with other scalar, argument-less nodes and looks correct.

src/backend/oracle_parser/ora_gram.y (3)

789-793: ROWNUM added as ordinary keyword token is consistent

Adding ROWNUM alongside ROWID in the main %token <keyword> list is consistent with other Oracle‑style pseudocolumns and makes it available to the keyword category nonterminals below. No issues here; it matches the PR’s intent to recognize ROWNUM at parse time.

20388-20392: ROWNUM classified as unreserved keyword allows identifier usage

Placing ROWNUM in unreserved_keyword means it:

  • Can be used wherever ColId or NonReservedWord is allowed (e.g., in expressions, qualified names).
  • Remains non‑reserved, so it can technically be used as a user‑defined identifier (table/column/role, etc.).

This is appropriate if IvorySQL intends ROWNUM to behave like other Oracle pseudocolumns that are recognized semantically (e.g., via ColumnRef transformation) rather than reserved in the grammar. If exact Oracle behavior is desired (i.e., disallowing user columns named ROWNUM), that restriction will need to be enforced during DDL analysis, not here.

Please confirm that allowing ROWNUM as a user‑defined identifier is acceptable for your Oracle‑compatibility goals; if not, you’ll want an explicit check in DDL validation to reject ROWNUM as a column/table name while still treating it as an unreserved keyword in queries.

21106-21111: Allowing ROWNUM as BareColLabel matches target‑list usage

Adding ROWNUM to bare_label_keyword allows constructs like:

  • SELECT expr ROWNUM (alias named ROWNUM)
  • SELECT 1 ROWNUM FROM t

while SELECT ROWNUM FROM t still parses as a column reference, not as an alias, because the a_expr BareColLabel production requires two tokens.

This is consistent with how other pseudocolumn‑adjacent names (e.g., ROWID) are treated and should not interfere with the ROWNUM pseudocolumn semantics implemented in parse analysis/executor. No grammar‑level issues spotted.

src/backend/nodes/nodeFuncs.c (1)

294-297: RownumExpr integration into core node helpers looks consistent

The new T_RownumExpr handling in exprType, exprCollation, exprSetCollation, expression_tree_walker_impl, and expression_tree_mutator_impl is aligned with how other primitive, non‑collatable scalar nodes are treated. No functional issues spotted here.

Also applies to: 1079-1082, 1340-1343, 2179-2180, 3061-3062

src/backend/optimizer/prep/prepunion.c (1)

814-818: Correctly tagging UNION Append/MergeAppend paths with is_union

Setting is_union = true on the Append and MergeAppend paths constructed specifically for UNION/UNION ALL ensures only true set-operation Append/MergeAppend plans participate in the per-branch ROWNUM reset logic, while appendrel/inheritance Appends stay untouched. This matches the intended Oracle‑style UNION semantics and keeps the flag well-scoped.

Also applies to: 861-867, 970-977

src/include/nodes/execnodes.h (2)

376-385: ProjectionInfo and EState extensions look structurally sound

Appending pi_needsMaterialization and es_rownum at the end of their structs is ABI-safe within the backend, and both will default to 0 via palloc0 when older callers don’t initialize them. The semantics (projection-level “needs materialization” flag and executor‑wide ROWNUM counter) are coherent with the usage in execScan.h and executor code paths.

Also applies to: 752-758

1495-1522: New UNION/ROWNUM tracking flags are consistent with executor state design

The additions of as_is_union to AppendState, ms_is_union to MergeAppendState, and rownum_reset to SubqueryScanState fit naturally into the existing state structures and match how they’re consumed in nodeAppend.c, nodeMergeAppend.c, and nodeSubqueryscan.c. With palloc0-based allocation and explicit initialization in the init functions, there’s no obvious structural or lifecycle issue from this header.

Also applies to: 1539-1553, 1947-1960

src/backend/executor/nodeMergeAppend.c (1)

200-208: ms_is_union wiring and initial per-branch ROWNUM reset are reasonable

Copying node->is_union into mergestate->ms_is_union and, on the first pass, resetting es_rownum to 0 before seeding each valid subplan ensures that UNION‑style MergeAppend branches start from a clean ROWNUM counter even if their startup (e.g., Sort) already consumed input and bumped the counter. Subsequent calls reuse the heap without additional resets, so per-branch sequences remain monotone without extra overhead. This is consistent as long as only true UNION/UNION ALL MergeAppends are tagged with is_union.

Also applies to: 241-263

src/include/executor/execScan.h (1)

183-285: ROWNUM accounting and projection materialization in ExecScanExtended are locally consistent

Within ExecScanExtended:

  • Incrementing es_rownum once per fetched tuple before ExecQual and rolling it back on qual failure ensures only qualifying rows consume ROWNUM values, while expressions evaluated in the qual and projection see the correct, pre‑filter value.
  • The fast path when !qual && !projInfo leaves es_rownum untouched, avoiding overhead for scans that don’t involve predicates or projections.
  • On the projection path, materializing the result slot only when projInfo->pi_needsMaterialization is set allows you to freeze ROWNUM (and other volatile expressions) into a physical tuple in the relatively rare cases where later re-reading the slot in a different context (e.g., subquery ORDER BY) would otherwise re‑expose volatility.

Given the planner’s responsibility to normalize simple ROWNUM <= N filters into LIMITs, this scan-level counter management looks coherent and doesn’t introduce obvious regressions for non‑ROWNUM queries.

src/backend/executor/nodeSubqueryscan.c (1)

46-52: ROWNUM reset in SubqueryNext can miscount for non-blocking subplans

The current logic marks first_call via rownum_reset, sets rownum_reset = true before the first ExecProcNode(node->subplan), and resets node->ss.ps.state->es_rownum = 0 once when first_call is true. This correctly cancels any es_rownum bumps during startup of a blocking subplan (e.g., Sort that drains its input on first call) so the outer SubqueryScan ROWNUM starts from 1.

However, if later calls invoke ExecProcNode(node->subplan) for a non-blocking (streaming) subplan that calls ExecScanExtended internally on each fetch, es_rownum will increment on each inner fetch. The SubqueryScan's own ExecScanExtended then pre-increments again for the same output row, yielding double-counted outer ROWNUM values (e.g., 1,3,5,… instead of 1,2,3,…).

This is a correctness issue only if the planner permits streaming subplans under SubqueryScan in ROWNUM contexts. If all such subplans are guaranteed to be fully blocking (drain on first call, then buffer), the current approach is safe. Verify whether that invariant holds; if not, consider:

  • Snapshot and restore es_rownum around each subplan call:
int64 saved_rownum = node->ss.ps.state->es_rownum;
slot = ExecProcNode(node->subplan);
/* ... */
node->ss.ps.state->es_rownum = saved_rownum;

to isolate inner increments, or

  • Document and enforce (with assertions) that only fully-blocking subplans appear under SubqueryScan in Oracle-mode ROWNUM paths.

Also applies to: 57-72, 133-134, 201-209

src/backend/executor/execExpr.c (3)

380-414: LGTM: Helper functions for ROWNUM detection are well-implemented.

The walker function correctly uses expression_tree_walker for recursion and short-circuits on finding RownumExpr. The targetlist_contains_rownum function properly iterates through target entries using lfirst_node for type-safe access.

557-562: Materialization flag correctly set for ROWNUM expressions.

Setting pi_needsMaterialization ensures ROWNUM values are preserved during projection, preventing re-evaluation that would yield incorrect values in outer queries. The placement after ExecReadyExpr is correct since the flag is independent of expression compilation.

2691-2697: LGTM: T_RownumExpr case follows standard pattern.

The implementation correctly emits the EEOP_ROWNUM step with no additional data required, consistent with similar simple expression types like T_CurrentOfExpr.

src/backend/executor/nodeSubplan.c (4)

69-101: LGTM: Proper save/restore pattern for ROWNUM state in ExecSubPlan.

The save_rownum correctly preserves the caller's ROWNUM state, and the reset to 0 before subplan execution ensures each correlated subquery invocation starts fresh, matching Oracle semantics. The restoration at the end maintains correct ROWNUM values for the outer query.

1122-1148: LGTM: Proper save/restore pattern for ROWNUM state in ExecSetParamPlan.

The InitPlan execution correctly saves and restores es_rownum, ensuring that lazy evaluation of initplan parameters doesn't interfere with the outer query's ROWNUM state. The reset to 0 before execution matches the expected Oracle behavior.

1282-1284: Correct restoration of both scan direction and ROWNUM counter.

The comment update from "restore scan direction" to "restore scan direction and ROWNUM counter" accurately reflects the additional restoration logic added for es_rownum.

274-278: Verify redundancy claim by examining control flow and ExecReScan implementation.

The comment claims the reset at lines 274-278 is redundant because es_rownum is already reset at line 91 before ExecScanSubPlan is called. This claim requires verification of:

  1. Whether ExecReScan (called before line 274-278) modifies es_rownum
  2. Whether there is control flow that could skip the line 91 reset but reach lines 274-278
  3. The purpose of the second reset in relation to the subplan execution lifecycle

If the reset is confirmed redundant, remove it. If intentional, add a clarifying comment explaining why it's necessary after ExecReScan.

src/backend/executor/execExprInterp.c (2)

555-555: LGTM: Dispatch table entry correctly added.

The EEOP_ROWNUM opcode is properly placed in the dispatch table. The StaticAssertDecl at line 613 ensures the table stays synchronized with the ExprEvalOp enum.

1597-1607: LGTM: Case handler follows established patterns.

The EEOP_ROWNUM case correctly delegates to the out-of-line ExecEvalRownum() function and advances with EEO_NEXT(), consistent with similar opcodes like EEOP_NEXTVALUEEXPR.

src/backend/optimizer/plan/planner.c (3)

62-65: New Oracle-mode headers are consistent with usage

Including utils/guc.h and utils/ora_compatible.h here is consistent with the new database_mode/DB_ORACLE usage below; no issues with these additions from a planning-layer perspective.

252-252: Static declaration for transform_rownum_to_limit is appropriate

Declaring transform_rownum_to_limit(Query *parse) as static keeps the helper confined to this translation unit and matches its single use from subquery_planner; interface and placement look fine.

1038-1044: Early invocation of transform_rownum_to_limit in subquery_planner is well-placed

Running transform_rownum_to_limit(parse) right after initializing PlannerInfo and before CTE processing, MERGE-to-join rewriting, join-tree manipulation, and expression preprocessing is appropriate:

  • You see the original jointree->quals before preprocess_expression flattens AND/OR structure and before quals are pushed around, which simplifies detecting a top-level ROWNUM predicate.
  • Later calls to subquery_planner for nested queries will each run this helper once at their own entry, so you don’t rely solely on the manual rtable recursion inside the helper.

Given the conservative guards inside transform_rownum_to_limit, this placement should not affect non-ROWNUM queries or non-Oracle database_mode.

src/backend/optimizer/plan/createplan.c (3)

82-93: Rownum helper declarations and context struct look consistent

The helper prototypes and replace_rownum_context (with rownum_vars and rownum_idx) are minimal and sufficient for the later mutator; no issues here.

1281-1287: Propagating is_union into Append plan node is correct

Copying best_path->is_union into Append ensures executor-side union-specific behavior (e.g., per-branch ROWNUM reset) can rely on plan metadata; the change is straightforward and safe.

1574-1578: Propagating is_union into MergeAppend is consistent with Append

Setting node->is_union = best_path->is_union keeps union metadata available for merge-based unions as well; this is consistent with the Append change and looks good.

rophy added 3 commits December 3, 2025 16:19
@rophy
feat: implement Oracle-compatible ROWNUM pseudocolumn
7705e72 
Add ROWNUM pseudocolumn with Oracle-compatible semantics:
- ROWNUM <= N optimized to LIMIT for simple queries
- Blocks LIMIT optimization for complex queries (ORDER BY, DISTINCT,
  GROUP BY, aggregation) to preserve Oracle semantics
- Handles special Oracle edge cases: ROWNUM > N, ROWNUM >= N, ROWNUM = N
- Supports tautology cases: ROWNUM > 0, ROWNUM >= 1 return all rows
- Marks ROWNUM as volatile to ensure per-row evaluation

Includes comprehensive regression tests covering all edge cases.
@rophy
fix: ROWNUM reset for UNION/UNION ALL branches (Oracle compatibility)
56e7fcc 
In Oracle, each UNION/UNION ALL branch has independent ROWNUM counting.
For example:
  SELECT ROWNUM, id FROM t1 UNION ALL SELECT ROWNUM, id FROM t2
produces rn=1,2,3 from t1 then rn=1,2,3 from t2, not rn=1,2,3,4,5,6.

Implementation:
- Add is_union flag to Append/MergeAppend plan nodes
- Detect UNION operations in planner (generate_union_paths, allpaths.c)
- Reset es_rownum in executor when switching between UNION branches
- MergeAppend resets before each child in initialization (Sort buffers all)
- Append resets when switching to next child during iteration
@rophy
test: add ROWNUM tests for INTERSECT, EXCEPT, LATERAL, DML, empty tables
f66e6ce 
Added test cases documenting ROWNUM behavior in additional scenarios:

- INTERSECT/EXCEPT: Documents that IvorySQL shares ROWNUM counter across
  both sides (differs from Oracle which resets for each side)
- LATERAL joins: Documents that IvorySQL doesn't reset ROWNUM for each
  outer row (differs from Oracle CROSS APPLY behavior)
- DELETE/UPDATE with ROWNUM: Works correctly (matches Oracle)
- Empty tables: Works correctly (returns 0 rows)

Tests include comments explaining Oracle vs IvorySQL behavior differences.
coderabbitai[bot]
coderabbitai bot reviewed Dec 3, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
src/backend/nodes/nodeFuncs.c (1)

2179-2180: RownumExpr as a primitive leaf is correctly wired into walkers/mutators; consider exprLocation support

  • Adding T_RownumExpr to the “primitive node types with no expression subnodes” in expression_tree_walker_impl and to the simple copyObject bucket in expression_tree_mutator_impl matches the struct (no subnodes) and is appropriate.
  • Optional: if you want error messages that call exprLocation() to point at ROWNUM usage, consider adding a case T_RownumExpr: there that returns the location field from RownumExpr. Right now it will quietly return -1.

Also applies to: 3061-3062

src/backend/optimizer/path/allpaths.c (1)

1337-1361: ROWNUM UNION detection looks correct but is a bit ad‑hoc; consider clarifying the intent.

The is_union_all heuristic (“all live_childrels have rtekind == RTE_SUBQUERY”) is a reasonable way to identify set-op appendrels (UNION/UNION ALL) and avoids mis-tagging inheritance/partitionwise-join appendrels, since those children are RTE_RELATION or joinrels. Propagating this via appendpath->is_union on all the main/partial/parallel append paths is exactly what the executor needs for ROWNUM resets.

Two minor suggestions:

  • The name/comment say “UNION ALL appendrels” but this will also be true for plain UNION (which is what you want for Oracle ROWNUM semantics). Renaming the local to is_union and updating the comment would better reflect actual behavior.
  • generate_orderedappend_paths still creates AppendPath/MergeAppendPath instances without touching any is_union flag. If the ROWNUM reset logic depends on that flag on ordered append/mergeappend plans as well, double‑check that those nodes get tagged elsewhere (e.g., in create_merge_append_path or createplan.c); otherwise, ordered UNION queries may not reset ROWNUM per branch.

If you want to cross‑check, a quick grep for is_union on AppendPath/MergeAppendPath plus where the executor inspects it will confirm you’ve covered all relevant plan shapes.

Also applies to: 1547-1556, 1559-1568, 1607-1614, 1659-1665

src/backend/executor/execExpr.c (1)

384-413: LGTM! Helper functions correctly detect ROWNUM in expressions.

The walker pattern is implemented correctly and handles nested ROWNUM expressions properly through expression_tree_walker.

Optional optimization: The target list is scanned twice—once during projection building (lines 441-550) and again here (line 562). You could detect ROWNUM during the first scan by adding a boolean flag that gets set when a RownumExpr is encountered. However, the current implementation is clearer and the performance impact is negligible since this runs at plan initialization, not per-row execution.

Apply this diff if you want to optimize:

 ProjectionInfo *
 ExecBuildProjectionInfo(List *targetList,
 						ExprContext *econtext,
 						TupleTableSlot *slot,
 						PlanState *parent,
 						TupleDesc inputDesc)
 {
 	ProjectionInfo *projInfo = makeNode(ProjectionInfo);
 	ExprState  *state;
 	ExprEvalStep scratch = {0};
 	ListCell   *lc;
+	bool		has_rownum = false;
 
 	projInfo->pi_exprContext = econtext;
 	/* We embed ExprState into ProjectionInfo instead of doing extra palloc */
 	projInfo->pi_state.type = T_ExprState;
 	state = &projInfo->pi_state;
 	state->expr = (Expr *) targetList;
 	state->parent = parent;
 	state->ext_params = NULL;
 
 	state->resultslot = slot;
 
 	/* Insert setup steps as needed */
 	ExecCreateExprSetupSteps(state, (Node *) targetList);
 
 	/* Now compile each tlist column */
 	foreach(lc, targetList)
 	{
 		TargetEntry *tle = lfirst_node(TargetEntry, lc);
+		
+		/* Check if this column contains ROWNUM */
+		if (!has_rownum && expression_contains_rownum_walker((Node *) tle->expr, NULL))
+			has_rownum = true;
 		
 		/* ... rest of the loop ... */
 	}
 
 	scratch.opcode = EEOP_DONE_NO_RETURN;
 	ExprEvalPushStep(state, &scratch);
 
 	ExecReadyExpr(state);
 
-	/*
-	 * Check if the target list contains ROWNUM expressions.
-	 * If so, we need to materialize the result tuple to preserve the
-	 * ROWNUM values and prevent re-evaluation in outer queries.
-	 */
-	projInfo->pi_needsMaterialization = targetlist_contains_rownum(targetList);
+	projInfo->pi_needsMaterialization = has_rownum;
 
 	return projInfo;
 }
src/backend/executor/execExprInterp.c (1)

3353-3359: Redundant null check can be simplified.

The if (planstate) check on line 3357 is unnecessary because planstate is assigned from state->parent, and we're already inside the if (state && state->parent) block guaranteeing it's non-null.

 	/* Safely get the PlanState and EState */
 	if (state && state->parent)
 	{
 		planstate = state->parent;
-		if (planstate)
-			estate = planstate->state;
+		estate = planstate->state;
 	}
src/backend/optimizer/plan/createplan.c (1)

2083-2229: ROWNUM-before-Sort rewrite is logically sound but can be slightly tightened

The transformation to evaluate ROWNUM below Sort/IncrementalSort and then reference it via Vars in the top projection is correct and matches Oracle semantics (ROWNUM assigned before ORDER BY). The flow:

  • Detects ROWNUM in the projection tlist only when a separate Result node is needed.
  • Extends the sort input’s tlist with one RownumExpr per occurrence in the final tlist.
  • Uses change_plan_targetlist so non–projection-capable inputs get a Result node under the Sort.
  • Appends corresponding Vars to the Sort’s own tlist and then replaces all RownumExpr occurrences in the final tlist with those Vars.

This gives per-row, pre-sort ROWNUM values while keeping the top-level node purely a projection over Vars, which is what the executor expects.

A couple of minor, deferrable nits:

  • You recompute count_rownum_exprs for each TargetEntry twice (once when adding RownumExprs, once when building rownum_vars). You could cache the counts in a small side list or struct if you ever care about shaving a tree walk, but it’s probably not worth complicating the code given realistic ROWNUM usage.
  • The extra ROWNUM columns propagated through Sort are internal plumbing; marking those added TargetEntrys as resjunk = true would make their intent clearer, though the separate Result node already hides them from consumers.

Functionally, the approach looks good.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7a0723c and f66e6ce.

⛔ Files ignored due to path filters (2)
  • src/oracle_test/regress/expected/rownum.out is excluded by !**/*.out
  • src/pl/plisql/src/expected/plisql_rownum.out is excluded by !**/*.out
📒 Files selected for processing (29)
  • src/backend/executor/execExpr.c (3 hunks)
  • src/backend/executor/execExprInterp.c (3 hunks)
  • src/backend/executor/execUtils.c (1 hunks)
  • src/backend/executor/nodeAppend.c (2 hunks)
  • src/backend/executor/nodeMergeAppend.c (2 hunks)
  • src/backend/executor/nodeResult.c (1 hunks)
  • src/backend/executor/nodeSubplan.c (6 hunks)
  • src/backend/executor/nodeSubqueryscan.c (3 hunks)
  • src/backend/nodes/nodeFuncs.c (5 hunks)
  • src/backend/optimizer/path/allpaths.c (4 hunks)
  • src/backend/optimizer/plan/createplan.c (5 hunks)
  • src/backend/optimizer/plan/planner.c (4 hunks)
  • src/backend/optimizer/prep/prepunion.c (3 hunks)
  • src/backend/optimizer/util/clauses.c (2 hunks)
  • src/backend/optimizer/util/pathnode.c (2 hunks)
  • src/backend/oracle_parser/ora_gram.y (3 hunks)
  • src/backend/parser/parse_expr.c (1 hunks)
  • src/backend/utils/adt/ruleutils.c (2 hunks)
  • src/include/executor/execExpr.h (2 hunks)
  • src/include/executor/execScan.h (3 hunks)
  • src/include/nodes/execnodes.h (5 hunks)
  • src/include/nodes/pathnodes.h (2 hunks)
  • src/include/nodes/plannodes.h (2 hunks)
  • src/include/nodes/primnodes.h (1 hunks)
  • src/include/oracle_parser/ora_kwlist.h (1 hunks)
  • src/oracle_test/regress/parallel_schedule (1 hunks)
  • src/oracle_test/regress/sql/rownum.sql (1 hunks)
  • src/pl/plisql/src/Makefile (1 hunks)
  • src/pl/plisql/src/sql/plisql_rownum.sql (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
  • src/include/nodes/plannodes.h
  • src/include/nodes/pathnodes.h
  • src/backend/optimizer/prep/prepunion.c
  • src/backend/utils/adt/ruleutils.c
  • src/backend/executor/nodeResult.c
  • src/oracle_test/regress/parallel_schedule
  • src/backend/executor/execUtils.c
  • src/include/nodes/execnodes.h
  • src/backend/parser/parse_expr.c
  • src/pl/plisql/src/Makefile
  • src/backend/executor/nodeAppend.c
  • src/backend/optimizer/util/clauses.c
  • src/backend/oracle_parser/ora_gram.y
  • src/pl/plisql/src/sql/plisql_rownum.sql
🧰 Additional context used
🧬 Code graph analysis (6)
src/backend/optimizer/plan/planner.c (4)
src/include/nodes/pg_list.h (1)
  • list_length (151-155)
src/backend/utils/cache/lsyscache.c (1)
  • get_opname (1455-1472)
src/include/postgres.h (2)
  • DatumGetInt64 (389-397)
  • DatumGetInt32 (206-210)
src/backend/nodes/makefuncs.c (2)
  • makeBoolConst (408-414)
  • makeConst (350-379)
src/include/executor/execExpr.h (1)
src/backend/executor/execExprInterp.c (2)
  • EEOP_ROWNUM (1597-1607)
  • ExecEvalRownum (3346-3371)
src/backend/executor/nodeSubqueryscan.c (1)
src/include/executor/executor.h (1)
  • ExecProcNode (310-317)
src/backend/executor/execExpr.c (1)
src/backend/executor/execExprInterp.c (1)
  • EEOP_ROWNUM (1597-1607)
src/include/executor/execScan.h (2)
src/include/executor/executor.h (1)
  • ExecProject (479-503)
src/include/executor/tuptable.h (1)
  • ExecMaterializeSlot (475-479)
src/backend/optimizer/plan/createplan.c (2)
src/include/nodes/pg_list.h (2)
  • list_length (151-155)
  • list_nth (298-303)
src/backend/nodes/makefuncs.c (2)
  • makeTargetEntry (289-313)
  • makeVar (66-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: oracle_pg_regression (ubuntu-latest)
  • GitHub Check: oracle_regression (ubuntu-latest)
  • GitHub Check: contrib_regression
  • GitHub Check: build (ubuntu-latest)
  • GitHub Check: pg_regression (ubuntu-latest)
  • GitHub Check: meson_build (ubuntu-latest)
🔇 Additional comments (28)
src/backend/executor/nodeSubqueryscan.c (2)

49-52: ROWNUM reset-on-first-call semantics in SubqueryNext

The first-call guard around resetting es_rownum after ExecProcNode(node->subplan) cleanly addresses the “blocking subplan prefetch” issue described in the comment and matches the intended Oracle-compatible behavior for inline views whose inner plans buffer on the first call.

One thing to double‑check: this unconditionally zeros node->ss.ps.state->es_rownum on the first call for this SubqueryScan, regardless of any prior ROWNUM activity in the same EState. That’s correct as long as no already‑materialized, user‑visible tuples for this query block have been assigned ROWNUM before SubqueryNext’s first call. If there are plan shapes where a SubqueryScan starts producing tuples only after other nodes in the same query block have already emitted ROWNUM-bearing rows, this reset would retroactively renumber subsequent rows.

If your planner invariants guarantee that this SubqueryScan’s first tuple is also the first time this query block’s ROWNUM should start counting (for the patterns you’re targeting here, e.g. inline views with blocking children), consider adding a brief note to that effect, or otherwise gating the reset behind a flag that encodes that assumption at plan time.

Also applies to: 59-72

133-134: Initialization and rescan of rownum_reset look consistent

Initializing subquerystate->rownum_reset = false in ExecInitSubqueryScan and resetting it to false in ExecReScanSubqueryScan correctly restores the “first call” behavior on each (re)scan, so the ROWNUM counter is re-zeroed once per scan as intended.

This is aligned with how other per-scan flags are managed in the executor and should play well with parameterized rescans and correlated use cases that drive repeated rescans of the same SubqueryScanState.

Also applies to: 204-209

src/backend/nodes/nodeFuncs.c (1)

294-297: ROWNUM type and collation handling look consistent

  • Mapping T_RownumExpr to INT8OID and treating it as non‑collatable (InvalidOid in exprCollation, assert in exprSetCollation) is internally consistent and matches the intended “numeric pseudocolumn” semantics.

Also applies to: 1079-1082, 1340-1343

src/include/oracle_parser/ora_kwlist.h (1)

461-461: ROWNUM keyword entry is correctly classified and ordered

  • ASCII ordering between "rowid" and "rows" is preserved.
  • UNRESERVED_KEYWORD, BARE_LABEL matches typical treatment of a pseudocolumn that should not be fully reserved.
src/include/nodes/primnodes.h (1)

2150-2163: RownumExpr node definition is minimal and compatible with existing infrastructure

  • Struct layout (just Expr xpr plus ParseLoc location) matches how it’s treated as a primitive, leaf node in walkers/mutators.
  • Doc comment clearly scopes semantics (Oracle ROWNUM, only in Oracle‑compat mode), which is helpful for future readers.
src/backend/optimizer/util/pathnode.c (2)

1363-1363: LGTM!

Default initialization of is_union = false with clear guidance for callers to override for UNION paths. This follows a sensible pattern where the path creation function provides a safe default.

1576-1577: LGTM!

Consistent with the create_append_path initialization pattern. The comment clearly indicates caller responsibility.

src/include/executor/execExpr.h (2)

888-888: LGTM!

The declaration matches the implementation signature shown in the relevant code snippets from execExprInterp.c.

203-203: Verify dispatch_table synchronization in execExprInterp.c

The EEOP_ROWNUM opcode placement in the enum is correct, but per the comment at lines 71-72, the corresponding handler must be registered in the dispatch_table[] array in execExprInterp.c:ExecInterpExpr() at the same index position. Ensure the handler entry is present and properly positioned to maintain synchronization between the enum and dispatch table.

src/backend/executor/nodeMergeAppend.c (2)

205-207: LGTM!

Correctly copies the is_union flag from the plan node to the executor state for runtime ROWNUM handling.

252-258: The ROWNUM reset timing is correct; Sort nodes are guaranteed for UNION branches.

The assumption that each child will buffer all input tuples is sound. The PostgreSQL planner enforces that all MergeAppend children are presorted on the merge key by injecting Sort or Incremental Sort nodes as needed. For UNION branches, the planner always ensures Sort nodes exist on unsorted children to maintain the merge ordering guarantee. This means the ROWNUM reset before ExecProcNode will correctly reset per branch since each branch is backed by a Sort node that buffers its input.

src/include/executor/execScan.h (4)

208-216: LGTM - Oracle-compatible pre-increment semantics.

Pre-incrementing es_rownum before the qual check ensures that conditions like ROWNUM <= 5 see the correct value. The increment is reverted on qual failure (lines 274-275), so only passing rows consume ROWNUM values.

The defensive check if (node->ps.state) is harmless, though ps.state should always be non-NULL for an active scan node.

241-256: LGTM - Materialization preserves ROWNUM values.

Materializing the slot when pi_needsMaterialization is true correctly captures ROWNUM values at projection time, preventing re-evaluation when the tuple is accessed by outer queries. This matches Oracle's semantics where ROWNUM in SELECT lists is captured as a constant.

266-278: LGTM - Correct revert on qual failure.

Decrementing es_rownum when the row fails the qual ensures only qualified rows consume ROWNUM values, matching Oracle's behavior.

167-171: Potential ROWNUM inconsistency on early return path.

When both qual and projInfo are NULL, this path returns without incrementing es_rownum. If ROWNUM is referenced in a higher-level node's projection (e.g., an outer query referencing ROWNUM from a subquery), the counter won't reflect rows passing through this scan.

Consider whether this is intentional. If ROWNUM is only valid when there's a projection containing it at this level, this is fine. But if ROWNUM can be pushed down while projInfo remains NULL here, rows would get stale ROWNUM values.

src/oracle_test/regress/sql/rownum.sql (1)

1-659: ROWNUM regression coverage and cleanup look solid.

The test script exercises ROWNUM across a wide range of constructs (basic predicates, ORDER BY, DISTINCT/GROUP BY/aggregates, correlated/nested subqueries, UNION/UNION ALL, EXISTS/IN, DML, projection-capability edge cases, and empty tables) and explicitly documents current INTERSECT/EXCEPT and LATERAL limitations. Object lifecycle is well managed: every helper table (including rownum_test, dept, test1/2, lat_test, del_test, upd_test, empty_test, etc.) is dropped by the end, so the file is self-contained.

No syntax or obvious semantic issues stand out from a planner/executor perspective.

src/backend/executor/nodeSubplan.c (1)

69-92: ROWNUM save/reset/restore around subplan execution is consistent and correct; minor redundancy is acceptable.

The new logic in ExecSubPlan and ExecSetParamPlan:

  • Saves estate->es_rownum,
  • Forces es_rownum = 0 for the duration of the subplan/initplan execution, and
  • Restores the saved value afterward,

ensures that each correlated subquery or initplan gets its own ROWNUM sequence starting at 0, without perturbing the caller’s ROWNUM (outer query block or enclosing subplan). This matches the intended Oracle-compatible behavior.

ExecScanSubPlan’s extra planstate->state->es_rownum = 0 immediately after ExecReScan(planstate) is technically redundant given ExecSubPlan’s earlier reset, but it does no harm and makes the function self-contained if it were ever reused.

Two small follow-ups to keep behavior uniform:

  • Verify that other subplan-like execution paths (e.g., any special CTE or SetOp execution code that might evaluate subqueries with their own ROWNUM) also follow the same save/reset/restore pattern on es_rownum, as you did here for ExecSubPlan and ExecSetParamPlan.
  • If you prefer to avoid redundancy, you could drop the reset in ExecScanSubPlan and rely solely on ExecSubPlan for the outer save/reset/restore.

Also applies to: 99-102, 274-279, 1122-1123, 1143-1148, 1283-1285

src/backend/executor/execExpr.c (2)

2691-2697: LGTM! T_RownumExpr handling follows the correct pattern.

The implementation correctly follows the existing pattern for pseudocolumns (similar to NextValueExpr at lines 2679-2689). The EEOP_ROWNUM opcode is properly defined and handled in execExprInterp.c as shown in the relevant code snippets.

557-562: Verify that materialization correctly preserves ROWNUM values across query boundaries.

The logic to set pi_needsMaterialization when ROWNUM is present makes sense—ROWNUM values are assigned during execution and must be preserved to prevent re-evaluation in outer query contexts. However, verify:

  1. Does the materialization actually prevent ROWNUM re-evaluation in subqueries accessed by outer queries?
  2. Should resjunk columns containing ROWNUM also trigger materialization? Currently they do, but resjunk columns are discarded and might not need materialization.

Ensure that materialization applies to the entire projection if ANY column contains ROWNUM, and that sufficient test coverage exists for ROWNUM in subquery and outer query contexts.

src/backend/executor/execExprInterp.c (3)

555-555: Dispatch table entry correctly placed.

The EEOP_ROWNUM entry follows the established pattern and is positioned consistently with the ExprEvalOp enum ordering. The static assert at line 613 will catch any ordering issues at compile time.

1597-1607: Handler follows established patterns.

The EEOP_ROWNUM case correctly follows the pattern used by similar opcodes like EEOP_NEXTVALUEEXPR, delegating to an out-of-line helper function and advancing with EEO_NEXT().

3365-3370: ROWNUM evaluation logic looks correct.

The fallback to 1 when es_rownum <= 0 handles edge cases where ROWNUM is evaluated before the counter is initialized. This defensive approach aligns with Oracle semantics where ROWNUM starts at 1.

src/backend/optimizer/plan/planner.c (3)

62-64: New GUC/Oracle-compat includes are appropriate

Including utils/guc.h and utils/ora_compatible.h here is correct for accessing database_mode and DB_ORACLE; no issues spotted.

252-252: Static transform_rownum_to_limit declaration is well-scoped

Declaring transform_rownum_to_limit as static keeps the helper local to this translation unit and matches the surrounding style.

656-717: ROWNUM→LIMIT transformation and predicate folding look semantically sound

The overall transform is careful and matches Oracle semantics for the handled forms:

  • Guarded by database_mode == DB_ORACLE and can_use_limit so only simple SELECT/DML blocks without ORDER BY/DISTINCT/GROUP BY/window/setops/target SRFs are rewritten to LIMIT, avoiding semantic shifts where PostgreSQL’s LIMIT position differs from Oracle’s ROWNUM.
  • RHS type check to INT8OID/INT4OID/INT2OID plus per-type DatumGetInt* prevents unsafe reinterpretation of arbitrary Const Datums.
  • Unsatisfiable/tautological predicates are folded aggressively regardless of can_use_limit:
    • ROWNUM <= 0, ROWNUM < 1 → always FALSE;
    • ROWNUM = N, N != 1 → always FALSE;
    • ROWNUM > N, N >= 1 and ROWNUM >= N, N > 1 → always FALSE;
    • ROWNUM > N, N < 1 and ROWNUM >= N, N <= 1 → tautologies (qual removed).
  • For satisfiable <=, <, and = 1 cases, the chosen limit_value and removal of the corresponding ROWNUM qual from jointree->quals are correct; the AND-list rebuild handles 0/1/many-qual cases safely and relies on later preprocessing to constant-fold e.g. other_qual AND FALSE.
  • Early returns after rewriting unsatisfiable predicates mean you don’t continue mutating a list you’re iterating, which is important given foreach + list_delete_ptr.

Only one LIMIT-producing ROWNUM qual per query block is used (first matching ROWNUM <= N, ROWNUM < N, or ROWNUM = 1), with any remaining ROWNUM predicates evaluated normally at execution. That still preserves correctness, even when combining bounds (e.g., ROWNUM <= 10 AND ROWNUM > 1 becomes LIMIT 10 plus an unsatisfiable runtime ROWNUM > 1 filter), so this is a planner-optimization choice rather than a semantic risk.

Overall, the function is robust and integrates cleanly with later preprocessing.

Also applies to: 718-997, 1000-1031

src/backend/optimizer/plan/createplan.c (3)

82-92: ROWNUM helper declarations and context struct look consistent

The new prototypes and replace_rownum_context struct are self-contained and match the usage patterns later in the file. No issues here.

1280-1288: Propagating is_union from AppendPath to Append is correct

Setting plan->is_union = best_path->is_union; ensures executor state can distinguish UNION-style appends from other Append uses (e.g., partition scans) without changing existing behavior when is_union is false.

1573-1581: MergeAppend now carries is_union as well — consistent with Append

Copying best_path->is_union into node->is_union mirrors the Append behavior and lets the executor apply per-UNION-branch ROWNUM semantics for both Append and MergeAppend uniformly.

src/backend/optimizer/plan/createplan.c
Comment on lines +7575 to +7683
/*
* contain_rownum_expr
* Check whether a node tree contains any ROWNUM expressions.
*
* This is used to detect when we need special handling for Oracle ROWNUM
* pseudocolumn in combination with Sort nodes.
*/
static bool
contain_rownum_expr_walker(Node *node, void *context)
{
if (node == NULL)
return false;

if (IsA(node, RownumExpr))
return true;

return expression_tree_walker(node, contain_rownum_expr_walker, context);
}

static bool
contain_rownum_expr(Node *node)
{
return contain_rownum_expr_walker(node, NULL);
}

/*
* replace_rownum_expr_mutator
* Replace all RownumExpr nodes with corresponding Vars from context.
*
* This handles nested ROWNUM expressions within complex expressions,
* not just top-level RownumExpr in target entries.
*/
static Node *
replace_rownum_expr_mutator(Node *node, replace_rownum_context *context)
{
if (node == NULL)
return NULL;

if (IsA(node, RownumExpr))
{
/* Replace with the next Var from our list */
if (context->rownum_idx < list_length(context->rownum_vars))
{
Var *replacement = (Var *) list_nth(context->rownum_vars,
context->rownum_idx);
context->rownum_idx++;
return (Node *) copyObject(replacement);
}
/* Should not happen if we counted correctly */
elog(ERROR, "ran out of replacement Vars for ROWNUM expressions");
}

return expression_tree_mutator(node, replace_rownum_expr_mutator, context);
}

/*
* count_rownum_exprs_walker
* Count the number of RownumExpr nodes in an expression tree.
*/
static bool
count_rownum_exprs_walker(Node *node, int *count)
{
if (node == NULL)
return false;

if (IsA(node, RownumExpr))
{
(*count)++;
return false; /* Don't recurse into RownumExpr */
}

return expression_tree_walker(node, count_rownum_exprs_walker, count);
}

/*
* count_rownum_exprs
* Count how many RownumExpr nodes are in an expression.
*/
static int
count_rownum_exprs(Node *node)
{
int count = 0;
count_rownum_exprs_walker(node, &count);
return count;
}

/*
* collect_rownum_exprs
* Collect all ROWNUM expressions from a target list.
*
* Returns a list of TargetEntry nodes that contain ROWNUM expressions
* (either top-level or nested).
*/
static List *
collect_rownum_exprs(List *tlist)
{
List *rownum_tles = NIL;
ListCell *lc;

foreach(lc, tlist)
{
TargetEntry *tle = lfirst_node(TargetEntry, lc);

if (contain_rownum_expr((Node *) tle->expr))
rownum_tles = lappend(rownum_tles, tle);
}

return rownum_tles;
}
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix walker callback signature for count_rownum_exprs_walker

The helper walkers are otherwise correct, but count_rownum_exprs_walker has a mismatched callback signature for expression_tree_walker. The walker framework expects bool (*)(Node *node, void *context), but the function is currently declared with int *count as the second parameter. That will at least trigger warnings, and on stricter builds can be a compile error.

You can fix this by switching the parameter type to void *context and casting inside:

-static bool
-count_rownum_exprs_walker(Node *node, int *count)
+static bool
+count_rownum_exprs_walker(Node *node, void *context)
 {
-	if (node == NULL)
-		return false;
-
-	if (IsA(node, RownumExpr))
-	{
-		(*count)++;
-		return false;  /* Don't recurse into RownumExpr */
-	}
-
-	return expression_tree_walker(node, count_rownum_exprs_walker, count);
+	int *count = (int *) context;
+
+	if (node == NULL)
+		return false;
+
+	if (IsA(node, RownumExpr))
+	{
+		(*count)++;
+		return false;	/* Don't recurse into RownumExpr */
+	}
+
+	return expression_tree_walker(node, count_rownum_exprs_walker, context);
 }

The rest of the ROWNUM helpers (contain_rownum_expr*, replace_rownum_expr_mutator, collect_rownum_exprs) are fine and consistent with existing walker/mutator patterns.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/*
* contain_rownum_expr
* Check whether a node tree contains any ROWNUM expressions.
*
* This is used to detect when we need special handling for Oracle ROWNUM
* pseudocolumn in combination with Sort nodes.
*/
static bool
contain_rownum_expr_walker(Node *node, void *context)
{
if (node == NULL)
return false;
if (IsA(node, RownumExpr))
return true;
return expression_tree_walker(node, contain_rownum_expr_walker, context);
}
static bool
contain_rownum_expr(Node *node)
{
return contain_rownum_expr_walker(node, NULL);
}
/*
* replace_rownum_expr_mutator
* Replace all RownumExpr nodes with corresponding Vars from context.
*
* This handles nested ROWNUM expressions within complex expressions,
* not just top-level RownumExpr in target entries.
*/
static Node *
replace_rownum_expr_mutator(Node *node, replace_rownum_context *context)
{
if (node == NULL)
return NULL;
if (IsA(node, RownumExpr))
{
/* Replace with the next Var from our list */
if (context->rownum_idx < list_length(context->rownum_vars))
{
Var *replacement = (Var *) list_nth(context->rownum_vars,
context->rownum_idx);
context->rownum_idx++;
return (Node *) copyObject(replacement);
}
/* Should not happen if we counted correctly */
elog(ERROR, "ran out of replacement Vars for ROWNUM expressions");
}
return expression_tree_mutator(node, replace_rownum_expr_mutator, context);
}
/*
* count_rownum_exprs_walker
* Count the number of RownumExpr nodes in an expression tree.
*/
static bool
count_rownum_exprs_walker(Node *node, int *count)
{
if (node == NULL)
return false;
if (IsA(node, RownumExpr))
{
(*count)++;
return false; /* Don't recurse into RownumExpr */
}
return expression_tree_walker(node, count_rownum_exprs_walker, count);
}
/*
* count_rownum_exprs
* Count how many RownumExpr nodes are in an expression.
*/
static int
count_rownum_exprs(Node *node)
{
int count = 0;
count_rownum_exprs_walker(node, &count);
return count;
}
/*
* collect_rownum_exprs
* Collect all ROWNUM expressions from a target list.
*
* Returns a list of TargetEntry nodes that contain ROWNUM expressions
* (either top-level or nested).
*/
static List *
collect_rownum_exprs(List *tlist)
{
List *rownum_tles = NIL;
ListCell *lc;
foreach(lc, tlist)
{
TargetEntry *tle = lfirst_node(TargetEntry, lc);
if (contain_rownum_expr((Node *) tle->expr))
rownum_tles = lappend(rownum_tles, tle);
}
return rownum_tles;
}
static bool
count_rownum_exprs_walker(Node *node, void *context)
{
int *count = (int *) context;
if (node == NULL)
return false;
if (IsA(node, RownumExpr))
{
(*count)++;
return false; /* Don't recurse into RownumExpr */
}
return expression_tree_walker(node, count_rownum_exprs_walker, context);
}
static int
count_rownum_exprs(Node *node)
{
int count = 0;
count_rownum_exprs_walker(node, &count);
return count;
}
🤖 Prompt for AI Agents 
In src/backend/optimizer/plan/createplan.c around lines 7575-7683, the function
count_rownum_exprs_walker currently uses the wrong second parameter type (int
*count) for expression_tree_walker; change its signature to bool
count_rownum_exprs_walker(Node *node, void *context) and inside cast the context
to int *count = (int *) context, increment *count when encountering a
RownumExpr, and pass the same context through to expression_tree_walker; ensure
count_rownum_exprs continues to call the walker with &count as the context and
return the counted value.

src/backend/optimizer/plan/planner.c
Comment on lines +1002 to +1006
/* Create the LIMIT constant (INT8 is pass-by-value on 64-bit systems) */
parse->limitCount = (Node *) makeConst(INT8OID, -1, InvalidOid,
sizeof(int64),
Int64GetDatum(limit_value),
false, true);
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the actual code in the file
cat -n src/backend/optimizer/plan/planner.c | sed -n '995,1015p'

Repository: IvorySQL/IvorySQL

Length of output: 178

🏁 Script executed:

# Search for FLOAT8PASSBYVAL usage in the codebase
rg "FLOAT8PASSBYVAL" --type c

Repository: IvorySQL/IvorySQL

Length of output: 178

🏁 Script executed:

# Search for other makeConst calls with INT8OID
rg "makeConst.*INT8OID" --type c -A 3 -B 1

Repository: IvorySQL/IvorySQL

Length of output: 178

🏁 Script executed:

# Look for typbyval lookups in the codebase
rg "typbyval" --type c | head -20

Repository: IvorySQL/IvorySQL

Length of output: 178

🏁 Script executed:

# Check if there are configuration headers that define FLOAT8PASSBYVAL
fd "config.h" --type f | head -5

Repository: IvorySQL/IvorySQL

Length of output: 178

🌐 Web query:

PostgreSQL int8 pass-by-value FLOAT8PASSBYVAL Datum

💡 Result:

Short answer: On current PostgreSQL builds, int8 (bigint) is a pass-by-value 8-byte type (it is stored in a Datum and passed/returned by value). The historical compile-time option FLOAT8PASSBYVAL (and USE_FLOAT8_BYVAL) controlled whether 8‑byte types (float8, int8, timestamp) were passed by value; modern Postgres requires Datum to be wide enough and treats built‑in 8‑byte types as pass-by-value, so extensions should assume int8/float8 are passed by value. See the C-language type docs and the notes about FLOAT8/Datum sizing. [1][2][3]

References

  • PostgreSQL C functions / base types (docs). [1]
  • Recent source discussion / pg_config_manual: FLOAT8PASSBYVAL / USE_FLOAT8_BYVAL history. [2]
  • CREATE TYPE / PASSEDBYVALUE note (Datum size requirement). [3]

🌐 Web query:

PostgreSQL makeConst INT8OID constbyval handling

💡 Result:

Short answer: makeConst sets constbyval from the type entry (typeByVal), so an INT8OID Const will have constbyval = true (i.e. passed by value) according to pg_type. See parse_coerce.c where makeConst assigns newcon->constbyval = typeByVal(baseType). [1][2]

Sources

  • makeConst in parse_coerce.c (assigns constbyval = typeByVal(baseType)). [1]
  • pg_type documentation (typbyval meaning; fixed-size 1/2/4/8 bytes can be pass-by-value). [2]

Use typeByVal() to determine INT8OID's constbyval instead of hardcoding true

The call hardcodes true for the constbyval parameter:

parse->limitCount = (Node *) makeConst(INT8OID, -1, InvalidOid,
                                       sizeof(int64),
                                       Int64GetDatum(limit_value),
                                       false, true);

While int8 is guaranteed pass-by-value on all modern PostgreSQL builds (and IvorySQL), the standard pattern is to look up the type's typbyval attribute via typeByVal() rather than hardcoding a literal. This ensures consistency with PostgreSQL conventions and makes the code more maintainable. Consider:

parse->limitCount = (Node *) makeConst(INT8OID, -1, InvalidOid,
                                       sizeof(int64),
                                       Int64GetDatum(limit_value),
                                       false, typeByVal(INT8OID));
🤖 Prompt for AI Agents 
In src/backend/optimizer/plan/planner.c around lines 1002 to 1006, the makeConst
call hardcodes the const-by-value flag as true for INT8OID; replace the literal
with a lookup using typeByVal(INT8OID) so the constbyval is determined from the
type system (i.e., call typeByVal(INT8OID) for that parameter) to follow the
standard pattern and avoid hardcoding.

@rophy rophy mentioned this pull request Dec 4, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@rophy