feat: ROWNUM Implementation #1000

rophy · 2025-12-03T15:56:09Z

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:

ROWNUM is assigned to each row before the WHERE clause is evaluated (for ROWNUM conditions)
ROWNUM is assigned only to rows that pass non-ROWNUM filters
ROWNUM values are sequential starting from 1
Each query block (including UNION branches) has its own independent ROWNUM counter
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:

The planner transforms ROWNUM <= N to a Limit node (optimization)
Each row fetched by the scan increments es_rownum
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:

es_rownum is pre-incremented before qual check
Both conditions are evaluated together
If id > 2 fails, es_rownum is reverted
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:

The is_union flag is set on AppendPath/MergeAppendPath nodes created for UNION operations
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:

ExecAppend: Resets es_rownum when switching to a new subplan (during iteration)
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

Pre-increment/Revert Overhead: Each filtered row requires an increment and a decrement. This is minimal overhead (two integer operations).
Optimizer Transformations: The planner transforms simple ROWNUM conditions to Limit nodes, avoiding the need for runtime ROWNUM checking in many cases.
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

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)
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)
ROWNUM optimization: Additional planner optimizations for complex ROWNUM patterns
Parallel query support: Ensure ROWNUM works correctly with parallel execution

Summary by CodeRabbit

New Features
- Oracle-compatible ROWNUM support: parsing, planner transforms (safe LIMIT conversion), runtime evaluation, projection materialization to preserve ROWNUM across queries/subqueries, and per-branch ROWNUM resets for UNION semantics.
Tests
- Adds extensive SQL and PL/pgSQL regression suites exercising ROWNUM across queries, subqueries, set operations, DML, optimizer interactions, and edge cases.

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

coderabbitai · 2025-12-03T15:56:37Z

📝 Walkthrough

Walkthrough

Adds Oracle ROWNUM end-to-end: new RownumExpr AST node and parser keyword; planner transforms (ROWNUM→LIMIT, projection rewrites); path/plan is_union flags; executor es_rownum, EEOP_ROWNUM/ExecEvalRownum, per-branch/subplan resets, and projection materialization; extensive tests added.

Changes

Cohort / File(s)	Summary
AST / public node types `src/include/nodes/primnodes.h`, `src/include/nodes/plannodes.h`, `src/include/nodes/pathnodes.h`, `src/include/nodes/execnodes.h`, `src/include/nodes/pathnodes.h`, `src/include/oracle_parser/ora_kwlist.h`	Add `RownumExpr` node type; add `is_union` fields to Append/MergeAppend path/plan structs; add `es_rownum`, `pi_needsMaterialization`, `as_is_union`, `ms_is_union`, `sub_rownum` public fields; add `ROWNUM` keyword entry.
Parser / name resolution / aliases `src/backend/parser/parse_expr.c`, `src/backend/parser/parse_relation.c`, `src/backend/parser/parse_target.c`, `src/pl/plisql/src/pl_comp.c`, `src/backend/oracle_parser/ora_gram.y`	Recognize unqualified `rownum` as `RownumExpr` in Oracle mode; reject `ROWNUM`/`ROWID` as table/column aliases in Oracle mode; PL/iSQL name-resolution tweaks; add `ROWNUM` grammar token.
Planner / optimizer rewrites `src/backend/optimizer/plan/planner.c`, `src/backend/optimizer/plan/createplan.c`, `src/backend/optimizer/path/allpaths.c`, `src/backend/optimizer/prep/prepunion.c`, `src/backend/optimizer/util/pathnode.c`	Add `transform_rownum_to_limit()` static helper and invoke during planning; detect/collect/replace ROWNUM in tlists; propagate `is_union` on paths/plans (Append/MergeAppend).
Expression infrastructure `src/backend/nodes/nodeFuncs.c`, `src/backend/optimizer/util/clauses.c`, `src/backend/utils/adt/ruleutils.c`	Register `T_RownumExpr` in type/collation/walkers/mutators; treat as mutable/volatile where relevant; deparse ROWNUM as literal `ROWNUM`.
Executor: eval/interpreter/state `src/include/executor/execExpr.h`, `src/backend/executor/execExprInterp.c`, `src/backend/executor/execExpr.c`, `src/backend/executor/execUtils.c`, `src/include/executor/execScan.h`	Add `EEOP_ROWNUM` opcode and `ExecEvalRownum` declaration/implementation; initialize `es_rownum` in executor state; pre-increment/revert around qual evaluation; propagate projection materialization flag handling.
Executor nodes & subplans `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` from plan to state and reset `es_rownum` per UNION branch; save/restore/reset `es_rownum` around correlated/init/subplan evaluations; add per-subquery `sub_rownum` handling; materialize projection when flagged.
Portal / materialization `src/backend/tcop/pquery.c`	Add plan walker to detect ROWNUM in plans and materialize portal result for scroll cursors when plan contains ROWNUM (private helpers and fill-store flow).
Planner validation / aggregates `src/backend/parser/parse_agg.c`	Treat ROWNUM in grouping substitution: raise grouping error when ROWNUM appears at current level without GROUP BY/aggregate.
Tests & schedules `src/oracle_test/regress/sql/rownum.sql`, `src/pl/plisql/src/sql/plisql_rownum.sql`, `src/oracle_test/regress/parallel_schedule`, `src/pl/plisql/src/Makefile`, `src/test/regress/sql/ora_keywords_pg.sql`, `src/test/regress/parallel_schedule`	Add extensive SQL and PL/iSQL regression tests for ROWNUM; add schedule entries and Makefile test registrations.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Client
    participant Parser
    participant Planner
    participant Executor
    participant EState as EState(es_rownum)

    Client->>Parser: submit SQL (uses ROWNUM)
    Parser->>Parser: emit RownumExpr in parse tree
    Parser->>Planner: pass query tree
    Planner->>Planner: transform ROWNUM (LIMIT or projection rewrites)
    Planner->>Executor: produce plan (may mark is_union / add ROWNUM Vars)

    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 -> EEOP_ROWNUM executed
            Executor->>Executor: if pi_needsMaterialization: ExecMaterializeSlot
            Executor->>Client: return tuple
        else qual fails
            Executor->>EState: es_rownum-- (revert)
        end
    end

sequenceDiagram
    autonumber
    participant OuterExec
    participant Subplan
    participant EState as EState(es_rownum)

    OuterExec->>EState: save outer_rownum
    OuterExec->>EState: reset es_rownum = 0
    OuterExec->>Subplan: execute subplan
    loop subplan tuples
        Subplan->>EState: es_rownum++ / evaluate / emit
    end
    OuterExec->>EState: restore outer_rownum

sequenceDiagram
    autonumber
    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

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Suggested reviewers

bigplaice
NotHimmel
jiaoshuntian

Poem

🐰 I count each hop from row to row,
es_rownum ticks where queries go.
Branches reset and subqueries play,
I tuck the numbers safe away.
A rabbit's tally—done for today.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: ROWNUM Implementation' clearly and concisely summarizes the main change: adding Oracle-compatible ROWNUM pseudocolumn support to the database system.
Docstring Coverage	✅ Passed	Docstring coverage is 85.48% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

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

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:

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.

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:

Whether ExecReScan (called before line 274-278) modifies es_rownum

Whether there is control flow that could skip the line 91 reset but reach lines 274-278

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.

src/backend/optimizer/plan/planner.c

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.

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

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

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:

Does the materialization actually prevent ROWNUM re-evaluation in subqueries accessed by outer queries?

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

src/backend/optimizer/plan/planner.c

- Fix count_rownum_exprs_walker signature to use void* context (matches expression_tree_walker callback convention) - Use get_typbyval(INT8OID) instead of hardcoded true for constbyval (follows PostgreSQL coding conventions) Both changes are stylistic improvements that align with PostgreSQL coding standards. The existing code worked due to macro casts and INT8 being pass-by-value on 64-bit systems, but proper signatures and API usage improve maintainability.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (3)

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

2091-2227: ROWNUM-before-Sort transformation is logically sound

The transformation that:

pushes RownumExpr evaluation down into the Sort input,

materializes one scan-level ROWNUM column per occurrence,

threads them through Sort as extra columns, and

rewrites the projected tlist to refer to those columns

correctly ensures ROWNUM is evaluated before Sort/IncrementalSort while preserving expression structure (including multiple or nested ROWNUM uses). Attribute numbering and use of change_plan_targetlist/OUTER_VAR look consistent with existing planner patterns. Any further cleanup (e.g., extracting this block into a dedicated helper for readability) is purely optional.

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

620-1032: ROWNUM→LIMIT transform is logically solid; consider deeper AND handling and multiple‑predicate tightening

The overall structure is good: Oracle-only guard, early bail if limitCount already present, conservative can_use_limit predicate, strict RHS type checks, and correct handling of Oracle’s special cases (ROWNUM > N, ROWNUM >= N, ROWNUM = N≠1) as unsatisfiable/tautological predicates. Using INT2/4/8 + DatumGetInt* and get_typbyval(INT8OID) for the generated LIMIT Const is also in line with the type system.

A few non-blocking improvements to consider:

You only look at the top-level BoolExpr AND arguments without flattening. For queries like WHERE rownum <= 10 AND a = 1 AND b = 2, the parser produces nested AND trees, so the RownumExpr is buried in a child BoolExpr and this code will never see it. If you want better coverage, either:

call canonicalize_qual / make_ands_implicit on jointree->quals here, or

recursively descend into BoolExpr nodes with boolop == AND_EXPR instead of only treating the first level.

You track a single rownum_qual/limit_value and leave any other ROWNUM conjuncts untouched. That’s semantically safe (remaining ROWNUM quals still gate rows) but can be suboptimal: for multiple upper bounds (e.g. ROWNUM <= 10 AND ROWNUM < 5) you could choose the tightest effective LIMIT and optionally strip all redundant ROWNUM quals once the LIMIT is installed.

Matching comparison operators via get_opname() + strcmp on "<=, "=", "<", ">", ">=" works for built-ins, but if you ever extend ROWNUM to non-integer or cross-type operators it would be more robust to check the operator’s btree strategy (and perhaps opfamily) instead of relying on the textual name.

These are all enhancement/coverage issues rather than correctness problems; the existing logic is safe as-is.

1128-1134: Early call to transform_rownum_to_limit in subquery_planner is well‑placed but redundant with the function’s own recursion

Running transform_rownum_to_limit(parse) right after initializing PlannerInfo (and before CTE processing, join pull‑up, and expression preprocessing) is a sensible place to do the rewrite so that both the current query and its descendants see any LIMITs derived from ROWNUM early.

Note, though, that transform_rownum_to_limit() itself already walks parse->rtable and recurses into RTE_SUBQUERY entries. When each subquery later goes through its own subquery_planner invocation, the transform runs twice on those trees (the second time it quickly bails due to parse->limitCount being set). If you want to simplify control flow and avoid the extra pass, you could:

either keep this top-level subquery_planner call and drop the explicit recursion over rtable inside transform_rownum_to_limit, or

remove the subquery_planner call and rely purely on per-Query invocations (top-level + subqueries) instead of recursing manually.

Not urgent, but it would make the execution order easier to reason about.

📜 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 f66e6ce and 3561a21.

📒 Files selected for processing (2)

src/backend/optimizer/plan/createplan.c (5 hunks)
src/backend/optimizer/plan/planner.c (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

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 (3)

DatumGetInt64 (389-397)

DatumGetInt32 (206-210)

DatumGetInt16 (166-170)

src/backend/nodes/makefuncs.c (2)

makeBoolConst (408-414)

makeConst (350-379)

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: build (ubuntu-latest)
GitHub Check: pg_regression (ubuntu-latest)
GitHub Check: oracle_pg_regression (ubuntu-latest)
GitHub Check: contrib_regression
GitHub Check: oracle_regression (ubuntu-latest)
GitHub Check: meson_build (ubuntu-latest)

🔇 Additional comments (5)

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

82-92: ROWNUM helper declarations are well-scoped and consistent

Static prototypes and replace_rownum_context are appropriately introduced here and keep the ROWNUM machinery internal to createplan.c. No issues with visibility or basic structure.

1286-1287: is_union propagation for Append/MergeAppend looks correct

Copying best_path->is_union into Append and MergeAppend plan nodes is the right place to carry set-op/UNION context down to the executor. Assuming the path and plan node structs both have the new is_union field, this wiring is straightforward and correct.

Also applies to: 1577-1578

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

62-64: New includes for Oracle compatibility mode are appropriate

Bringing in utils/guc.h and utils/ora_compatible.h here makes sense given the new dependency on database_mode/DB_ORACLE in transform_rownum_to_limit(), and utils/lsyscache.h already covers get_opname/get_typbyval. No issues spotted.

252-252: Static declaration placement for transform_rownum_to_limit is fine

Declaring transform_rownum_to_limit(Query *parse) alongside the other file-local planner helpers keeps the interface clear and correctly limits scope to this translation unit.

1002-1006: makeConst for LIMIT uses correct by‑val flag

Constructing limitCount as an INT8OID Const with sizeof(int64), Int64GetDatum(limit_value), and constbyval = get_typbyval(INT8OID) is the right pattern and avoids the previous FLOAT8PASSBYVAL hack. This should interoperate cleanly with preprocess_limit() and limit_needed(), which assume a pass‑by‑value 8‑byte integer Const.

src/backend/optimizer/plan/createplan.c

Change replace_rownum_expr_mutator to use void* context parameter to match expression_tree_mutator callback convention. This is the same fix applied to count_rownum_exprs_walker in the previous commit.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/backend/optimizer/plan/createplan.c (2)
2084-2229: ROWNUM-before-Sort rewrite is logically sound; consider tightening parallel safety and minor details

The special-case block that, when needs_result_node and subplan is Sort/IncrementalSort and the projection tlist contains ROWNUM, does the following:

Pushes one RownumExpr per occurrence into the sort input (sortinput) via change_plan_targetlist, so ROWNUM is evaluated on the unsorted stream.

Builds matching Vars (rownum_vars) pointing at those new scan outputs.

Extends the Sort/IncrementalSort’s own targetlist with TLEs referencing those Vars, so the ROWNUM values flow through the sort.

Rewrites the final projection tlist by replacing every RownumExpr with the corresponding Var using replace_rownum_expr_mutator.

That preserves Oracle semantics (“assign ROWNUM before ORDER BY”) while still letting the projection happen in a Result node above the sort, and handles multiple and nested ROWNUM occurrences correctly. The count_rownum_exprs + rownum_idx mapping is consistent and safe (and all per-rownum columns have the same value anyway).

Two non-blocking points you may want to tighten:
Parallel safety propagation: the call
sortinput = change_plan_targetlist(sortinput, new_input_tlist,
                                   sortinput->parallel_safe);
reuses the existing parallel_safe flag as the tlist_parallel_safe argument. If RownumExpr is treated as parallel-unsafe, it would be safer to either:

pass false here when you’re injecting ROWNUM, or

compute tlist_parallel_safe with is_parallel_safe(root, (Node *) new_input_tlist) (or at least for the added RownumExprs).
Otherwise there’s a risk of marking this subtree parallel‑safe even though we just introduced a parallel‑unsafe expression. This may already be guarded elsewhere in the ROWNUM plumbing, but it’s worth confirming.
Minor optimization/nit: inside replace_rownum_expr_mutator, you repeatedly call list_length(ctx->rownum_vars) on every RownumExpr visit. If you care about micro‑efficiency with many occurrences, caching that length in replace_rownum_context (e.g., int rownum_count) avoids repeated O(n) walks; not critical given typical usage.
Functionally, though, the transformation looks correct and in line with the rest of the plan‑building patterns in this file.

7575-7687: ROWNUM helper implementations match walker/mutator contracts and handle nested usage correctly

contain_rownum_expr_walker / contain_rownum_expr correctly use expression_tree_walker and short‑circuit on RownumExpr.

replace_rownum_expr_mutator now has the proper void *context signature, casts to replace_rownum_context *, and walks with expression_tree_mutator, replacing each RownumExpr with the next Var from rownum_vars, with a defensive elog(ERROR) if the count underflows.

count_rownum_exprs_walker / count_rownum_exprs correctly count all RownumExpr nodes using the walker pattern.

collect_rownum_exprs cleanly gathers TargetEntrys whose expressions contain ROWNUM, covering nested occurrences.

These are idiomatic uses of the nodeFuncs walker/mutator APIs and line up with the prototypes at the top of the file. The only tiny polish you might consider (purely optional) is storing the list length in replace_rownum_context instead of calling list_length in each replacement.

📜 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 3561a21 and a8171fa.

📒 Files selected for processing (1)

src/backend/optimizer/plan/createplan.c (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

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: pg_regression (ubuntu-latest)
GitHub Check: build (ubuntu-latest)
GitHub Check: oracle_regression (ubuntu-latest)
GitHub Check: oracle_pg_regression (ubuntu-latest)
GitHub Check: meson_build (ubuntu-latest)
GitHub Check: contrib_regression

🔇 Additional comments (3)

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

82-93: ROWNUM helper declarations and context wiring are consistent

The forward declarations for contain_rownum_expr, count_rownum_exprs, collect_rownum_exprs, and replace_rownum_expr_mutator, plus replace_rownum_context, all line up with the implementations later in the file and with the walker/mutator API expectations. No issues here.

1280-1287: Propagating is_union from AppendPath to Append looks correct – just ensure executor side is wired

Setting plan->is_union = best_path->is_union; is the right place to carry the planner flag into the plan node for UNION‑specific ROWNUM behavior. Locally this is self‑contained; just make sure corresponding fields and logic exist in the Append node struct, copyfuncs/outfuncs, and executor (e.g., ExecInitAppend/ExecAppend) to actually honor it.

1574-1578: MergeAppend is_union propagation mirrors Append and is fine

node->is_union = best_path->is_union; correctly propagates the UNION flag for MergeAppend branches, which is needed for ROWNUM reset semantics when inputs are merged via a sorted append. As with Append, just confirm the MergeAppend struct, serialization, and executor logic are updated to use this flag.

bigplaice · 2026-01-06T01:49:12Z

@rophy

Hi, Rophy,

Your implementation uses a shared es_rownum counter in the EState structure, combined with the timing of resets in SubqueryNext for nested subqueries. The reset (es_rownum = 0) occurs after fetching the first tuple from the subplan, which works for simple cases but may have problem in multiple nested scenarios.

I tested this with the following case:

CREATE TABLE eemp(
empno NUMBER(4) NOT NULL CONSTRAINT emp_pk PRIMARY KEY,
ename VARCHAR2(10),
job VARCHAR2(9),
mgr NUMBER(4),
hiredate DATE,
sal NUMBER(7,2) CONSTRAINT emp_sal_ck CHECK (sal > 0),
comm NUMBER(7,2),
deptno NUMBER(2)
);

INSERT INTO eemp VALUES (7369,'SMITH','CLERK',7902, to_date('17-10-1980','dd-mm-yyyy'),800,NULL,20);
INSERT INTO eemp VALUES (7499,'ALLEN','SALESMAN',7698,to_date('20-02-1981','dd-mm-yyyy'),1600,300,30);
INSERT INTO eemp VALUES (7521,'WARD','SALESMAN',7698,to_date('22-02-1981','dd-mm-yyyy'),1250,500,30);
INSERT INTO eemp VALUES (7566,'JONES','MANAGER',7839,to_date('02-04-1981','dd-mm-yyyy'),2975,NULL,20);
INSERT INTO eemp VALUES (7654,'MARTIN','SALESMAN',7698,to_date('28-09-1981','dd-mm-yyyy'),1250,1400,30);
INSERT INTO eemp VALUES (7698,'BLAKE','MANAGER',7839,to_date('01-05-1981','dd-mm-yyyy'),2850,NULL,30);
INSERT INTO eemp VALUES (7782,'CLARK','MANAGER',7839,to_date('09-06-1981','dd-mm-yyyy'),2450,NULL,10);
INSERT INTO eemp VALUES (7788,'SCOTT','ANALYST',7566,to_date('17-04-1987','dd-mm-yyyy'),3000,NULL,20);
INSERT INTO eemp VALUES (7839,'KING','PRESIDENT',NULL,to_date('17-11-1981','dd-mm-yyyy'),5000,NULL,10);
INSERT INTO eemp VALUES (7844,'TURNER','SALESMAN',7698,to_date('08-09-1981','dd-mm-yyyy'),1500,0,30);
INSERT INTO eemp VALUES (7876,'ADAMS','CLERK',7788,to_date('23-05-1987','dd-mm-yyyy'),1100,NULL,20);
INSERT INTO eemp VALUES (7900,'JAMES','CLERK',7698,to_date('03-10-1981','dd-mm-yyyy'),950,NULL,30);
INSERT INTO eemp VALUES (7902,'FORD','ANALYST',7566,to_date('17-10-1980','dd-mm-yyyy'),3000,NULL,20);
INSERT INTO eemp VALUES (7934,'MILLER','CLERK',7782,to_date('23-02-1982','dd-mm-yyyy'),1300,NULL,10);

-- Pagination with ROWNUM
select *
from
(select a.*, rownum rnum
from
(select empno, sal
from eemp
order by sal) a
where rownum <= 12
)
where rnum >= 8;

Expected result:
empno | sal | rnum
-------+---------+------
7499 | 1600.00 | 8
7782 | 2450.00 | 9
7698 | 2850.00 | 10
7566 | 2975.00 | 11
7902 | 3000.00 | 12
(5 rows)

Actually got：
empno | sal | rnum
-------+---------+------
7782 | 2450.00 | 8
7698 | 2850.00 | 10
7566 | 2975.00 | 12
7902 | 3000.00 | 14
(4 rows)

The result has one less than correct result and skips to even values like 8, 10, 12, 14.

I made a fix for this issue as attached. Please let me know if it is appropriate.

fix_multiple_nested_subquery.patch

Thanks,
Steven

bigplaice · 2026-01-06T06:06:54Z

With regard to the database_mode and compatible_mode, they are different in IvorySQL.
If database_mode is set to be DB_ORACLE, then initdb tool will load plisql parser and create ivorysql_ora extension.
While the compatible_mode is in responsible to determine which parser is used, PG native parser or IvorySQL parser.

I believe the following code should uniformly use compatible_mode.

transform_rownum_to_limit()

/* Only apply in Oracle compatibility mode */
if (database_mode != DB_ORACLE)
```
  return;
```

transformColumnRefInternal()
if (database_mode == DB_ORACLE && pg_strcasecmp(colname, "rownum") == 0)

struct EState

/*
- Oracle ROWNUM support: current row number counter.
- This is incremented for each row emitted during query execution.
- Only used when database_mode == DB_ORACLE.
*/

bigplaice · 2026-01-06T08:50:38Z

Using the same test data in my first comment, another issue is found.

SQL:
select rownum from eemp group by deptno; -- error

Expected result:
ERROR: column "ROWNUM" must appear in the GROUP BY clause or be used in an aggregate function

Actually got:
rownum

  1
  1
  1

(3 rows)

bigplaice · 2026-01-06T09:08:42Z

And apparently this implementation does not support aggregate functions yet.

Add test case for Oracle pagination pattern with nested subqueries. This exposes a bug where shared es_rownum counter causes skipped values (8,10,12,14 instead of 8,9,10,11,12) in nested subquery scans.

Each SubqueryScan now maintains its own sub_rownum counter instead of sharing es_rownum globally. On each tuple fetch, the outer query's counter is saved, this subquery's counter is swapped in, the subplan executes, then the outer counter is restored. This fixes the Oracle pagination pattern where nested subqueries caused skipped ROWNUM values (8,10,12,14 instead of 8,9,10,11,12). Co-authored-by: Steven <bigplaice@users.noreply.github.com>

Change ROWNUM checks from database_mode == DB_ORACLE to compatible_db == ORA_PARSER. This allows ROWNUM to work based on the session's compatible_mode setting rather than the cluster's database_mode. - database_mode: set at initdb, cannot change at runtime - compatible_mode: can be changed per-session Co-authored-by: Steven <bigplaice@users.noreply.github.com>

ROWNUM must appear in the GROUP BY clause or be used in an aggregate function, matching Oracle's behavior. Without this check, queries like "SELECT rownum FROM t GROUP BY category" would incorrectly return results instead of raising an error. Co-authored-by: Steven <bigplaice@users.noreply.github.com>

When ROWNUM is used inside aggregate functions like MIN(rownum), MAX(rownum), SUM(rownum), AVG(rownum), the planner now adds ROWNUM to the subplan's target list. This ensures ROWNUM is evaluated during the scan (where it increments for each row) rather than during aggregation (where it would return the same value). The fix modifies create_agg_plan() to detect ROWNUM in aggregate arguments and add a RownumExpr to the scan's target list. The setrefs.c code then matches the RownumExpr in aggregate arguments with the one in the subplan and creates appropriate Var references. Before: SELECT MIN(rownum), MAX(rownum), SUM(rownum) FROM t; -> 1, 1, 6 (for 6 rows - ROWNUM always evaluated as 1) After: SELECT MIN(rownum), MAX(rownum), SUM(rownum) FROM t; -> 1, 6, 21 (correct values for 6 rows)

Adds test cases for: - Subquery with aggregate containing ROWNUM - Aggregate of subquery with ROWNUM in SELECT - Nested subquery aggregates with GROUP BY - Multiple levels of aggregation

When using ROWNUM inside aggregates with GROUP BY and ORDER BY, queries would fail with "Aggref found in non-Agg plan node" if the ROWNUM-containing aggregate wasn't the last column in the SELECT list. The fix skips special ROWNUM handling when ROWNUM only appears inside aggregate function arguments, since the Agg node correctly evaluates ROWNUM from its input. Also removes broken targetlist reordering logic that attempted to reorder Sort output (Sort is non-projection-capable). Added tests for: - Aggregate + GROUP BY + ORDER BY with various column orders - Multi-layer nested aggregation with ROWNUM (verified against Oracle)

rophy · 2026-01-07T02:27:18Z

I made a fix for this issue as attached. Please let me know if it is appropriate.

Added a test case in commit 3a9f276 to cover the issue, and applied your patch in commit 11a8ed6, works great.

I believe the following code should uniformly use compatible_mode.

Fixed in commit 2ecbfb7

Using the same test data in my first comment, another issue is found.

Fixed in commit 5e4b64b

And apparently this implementation does not support aggregate functions yet.

Commit 91730a3 and commit 0455343 attempted to support aggregate functions, group by / order by and nested.
Added tests to cover various cases.
Hope current version is more close to feature ready.

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @src/backend/optimizer/plan/planner.c:
- Around line 719-997: The loop over ROWNUM clauses in the foreach scanning
andlist exits early because the branches handling opname "<=", "=", and "<" use
break, which leaves later ROWNUM predicates (e.g., ones that make the query
always false) unexamined; change those break statements to continue so the
foreach loop keeps scanning all quals (ensure you preserve setting limit_value
and rownum_qual, and that opname is still pfree'd on every path), i.e., in the
branches that currently do "if (can_use_limit) { limit_value = n; rownum_qual =
qual; } pfree(opname); break;" (and the "<" variant using n-1) replace the final
break with continue so the loop examines remaining quals in andlist.

🧹 Nitpick comments (5)

src/backend/executor/nodeSubqueryscan.c (1)
200-204: Minor: Comment could be more precise.

The comment states "Each rescan starts with ROWNUM = 1" but the counter is reset to 0. While functionally correct (the increment happens during scanning), the comment could better reflect the actual reset value.
Suggested clarification
 	/*
 	 * Reset local ROWNUM counter for Oracle compatibility.
-	 * Each rescan starts with ROWNUM = 1.
+	 * Reset to 0 so the first emitted row will have ROWNUM = 1 after increment.
 	 */
 	node->sub_rownum = 0;
src/backend/optimizer/plan/planner.c (1)

1128-1134: Good placement of transform_rownum_to_limit in subquery_planner, with minor coverage note

Calling transform_rownum_to_limit(parse) very early in subquery_planner (before CTE processing, MERGE rewrite, jointree normalization, and expression preprocessing) is a sensible choice:

It sees the original jointree->quals shape before canonicalization and constant‑folding.

The recursive walk over parse->rtable ensures ROWNUM predicates in subquery RTEs are handled uniformly.

Running before preprocess_expression and preprocess_qual_conditions avoids fighting with later rewrites.

One thing to be aware of (not necessarily for this PR) is that this hook does not currently touch:

ROWNUM predicates inside SubLinks (Subquery expressions in quals/targets), or

ROWNUM inside CTE query trees (cteList), which are expanded by SS_process_ctes after this call.

Those cases will still get correct runtime semantics from the executor, but they won’t benefit from the LIMIT/unsatisfiable rewrites. If you ever need planner‑level ROWNUM behavior there too, you’d want an expression‑tree walker or a second entry point that runs over SubLink/CTE Query nodes.
src/backend/optimizer/plan/createplan.c (3)
2086-2243: ROWNUM-before-Sort rewrite is correct but dense; consider small clarifications

The ROWNUM+Sort/IncrementalSort special-case in create_projection_plan looks logically sound:

It only triggers when we actually need a separate Result node and the immediate subplan is Sort or IncrementalSort.

It skips cases where ROWNUM occurs only inside aggregate arguments (aggref_args_contain_rownum), which are handled via create_agg_plan.

It:

Adds one RownumExpr per occurrence to the sort input's tlist via change_plan_targetlist, so ROWNUM is evaluated in the scan/producer.

Builds a parallel list of OUTER_VAR Vars (rownum_vars) that point at those new columns.

Extends the Sort’s own targetlist with Vars so the ROWNUM values propagate through the Sort.

Replaces all RownumExpr nodes in the final projection tlist with those Vars, ensuring ROWNUM is computed before the sort but only referenced above it.

Two things worth tightening or at least documenting:
Mixed aggregate and non-aggregate ROWNUM with Sort

When the same SELECT list contains both aggregates over ROWNUM and bare ROWNUM, aggref_args_contain_rownum(tlist) will be true, so this block is skipped and the non-aggregate ROWNUM will again be evaluated above Sort. If such queries are meant to be valid under your “ROWNUM must be grouped or aggregated” rule (e.g., ROWNUM appears in both GROUP BY and an aggregate), you may want to:

Either explicitly reject this pattern in parse analysis, or

Relax the !aggref_args_contain_rownum(tlist) guard and handle both cases in a unified way.
Readability / maintenance

The ROWNUM rewrite is a sizeable chunk inside create_projection_plan. Extracting it into a helper like:
static void
pushdown_rownum_through_sort(Plan *sort_plan, List **ptlist);
would make create_projection_plan easier to follow and reduce the chance of regressions if Sort handling changes later.
Functionally this all looks correct; these are mostly about clarifying semantics and future-proofing.

It’d be useful to add or confirm tests for cases like:

SELECT ROWNUM, SUM(ROWNUM) FROM t GROUP BY ROWNUM ORDER BY some_col;

SELECT ROWNUM FROM (SELECT ... ORDER BY ...) ORDER BY ...;

to ensure mixed aggregate/non-aggregate ROWNUM with ORDER BY behaves as intended with this guard in place.

2496-2526: Agg plan ROWNUM support is good; consider extending detection beyond SELECT tlist

Adding a single RownumExpr to the Agg subplan’s targetlist when aggref_args_contain_rownum(tlist) is true is a clean way to make ROWNUM visible to Agg (and to setrefs.c for Var substitution).

A couple of edge cases to double-check:

ROWNUM aggregates not in the SELECT list
aggref_args_contain_rownum(tlist) only inspects expressions reachable from the Agg’s output tlist. If you allow constructs like HAVING max(ROWNUM) < 10 or ORDER BY max(ROWNUM) where the aggregate over ROWNUM appears only in best_path->qual or sort clauses, those won’t currently trigger this block. In that case, you may want to:

Either extend the detection to also walk best_path->qual (and possibly relevant ORDER BY expressions), or

Formally disallow such uses and enforce that aggregates over ROWNUM must appear in the SELECT list.

GROUPING SETS / ROLLUP / CUBE
create_groupingsets_plan builds Agg nodes separately and doesn’t currently call aggref_args_contain_rownum, so ROWNUM inside aggregates in grouping sets queries may not get the same subplan-tlist ROWNUM column. If you expect Oracle-style ROWNUM-in-aggregate semantics to work there too, you probably need a similar injection in create_groupingsets_plan.

None of this blocks the current patch, but clarifying the intended support surface (simple Agg only vs. also HAVING and GROUPING SETS) and adding a few regression tests around those cases would be valuable.

7623-7794: ROWNUM walker/mutator helpers are correct; only minor efficiency nits

The new helpers:

contain_rownum_expr[_walker]

replace_rownum_expr_mutator

count_rownum_exprs[_walker]

collect_rownum_exprs

aggref_args_contain_rownum[_walker]

all have the right bool (Node *, void *)/Node *(Node *, void *) signatures and use expression_tree_walker/mutator idiomatically; they avoid recursing into RownumExpr where appropriate (for counting) and correctly limit aggref_args_contain_rownum to ROWNUM occurrences inside Aggref arguments/direct args/filters.

Two small, optional cleanups:

aggref_args_contain_rownum_walker currently calls contain_rownum_expr on Aggref args and then also invokes expression_tree_walker on the node, which re-walks the same subtree in the no-ROWNUM case. Given these are planner-time only and trees are small, it’s fine, but you could drop the extra expression_tree_walker for Aggref branches (returning false instead) to avoid double work.

collect_rownum_exprs followed by count_rownum_exprs means each TLE containing ROWNUM is traversed twice. That’s also harmless, but if this ever shows up in hot planner paths you might fold counting into collect_rownum_exprs to track per-TLE counts in one walk.

As written, the helpers are correct and maintainable; the above is purely micro-optimization.

📜 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 a8171fa and 91730a3.

⛔ Files ignored due to path filters (1)

src/oracle_test/regress/expected/rownum.out is excluded by !**/*.out

📒 Files selected for processing (7)

src/backend/executor/nodeSubqueryscan.c
src/backend/optimizer/plan/createplan.c
src/backend/optimizer/plan/planner.c
src/backend/parser/parse_agg.c
src/backend/parser/parse_expr.c
src/include/nodes/execnodes.h
src/oracle_test/regress/sql/rownum.sql

🚧 Files skipped from review as they are similar to previous changes (2)

src/backend/parser/parse_expr.c
src/oracle_test/regress/sql/rownum.sql

🧰 Additional context used

🧬 Code graph analysis (3)

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

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

ExecProcNode (310-317)

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

src/backend/utils/error/elog.c (2)

errcode (853-864)

errmsg (1070-1086)

src/backend/parser/parse_node.c (1)

parser_errposition (108-123)

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)

🔇 Additional comments (11)

src/backend/parser/parse_agg.c (1)

1417-1433: LGTM — Correct implementation of ROWNUM validation in GROUP BY context.

The logic correctly mirrors the existing Var handling pattern:

At the current query level (sublevels_up == 0), ROWNUM must appear in GROUP BY or be aggregated

For outer query references, the expression is returned as-is (not our problem to validate)

The error message and error code are appropriate for Oracle compatibility.

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

49-67: LGTM — Correct save/restore pattern for nested subquery ROWNUM independence.

The implementation correctly addresses the nested subquery pagination bug mentioned in the PR comments:

Saves outer query's es_rownum

Swaps in subquery's local sub_rownum

Executes subplan (which may increment es_rownum)

Captures updated counter back to sub_rownum

Restores outer query's counter

This ensures each subquery maintains an independent ROWNUM sequence.

129-129: LGTM — Proper initialization of the local ROWNUM counter.

Initializing to 0 is correct since the counter is pre-incremented during scan execution, making the first emitted row have ROWNUM=1.

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

383-384: LGTM — Appropriate field for tracking materialization needs.

The pi_needsMaterialization flag is well-placed and the comment correctly indicates its purpose for volatile expressions like ROWNUM.

752-757: LGTM — Correct type and documentation for ROWNUM counter.

Using int64 for es_rownum is appropriate to handle large row counts. The comment correctly documents the Oracle compatibility context and when this field is active.

1521-1521: LGTM — Union flag for Append ROWNUM handling.

The as_is_union flag enables proper ROWNUM reset on UNION branch switches, consistent with Oracle semantics where each UNION branch has independent ROWNUM sequences.

1551-1551: LGTM — Consistent union flag for MergeAppend.

The ms_is_union flag mirrors as_is_union in AppendState, maintaining consistency for ROWNUM handling across both append node types.

1951-1959: LGTM — Well-documented local ROWNUM counter for subqueries.

The sub_rownum field and accompanying documentation clearly explain the purpose: enabling independent ROWNUM sequences for nested subqueries, which is essential for Oracle-compatible behavior.

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

1000-1006: makeConst for LIMIT uses correct by‑val flag

Using get_typbyval(INT8OID) as the constbyval argument in the synthesized LIMIT Const is the right direction and avoids hard‑coding assumptions like FLOAT8PASSBYVAL for an INT8OID value. This keeps the code aligned with the type system and previous static‑analysis feedback.

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

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

The new ROWNUM-related prototypes and replace_rownum_context struct are well-aligned with the implementations below and follow the usual nodeFuncs walker/mutator patterns. No issues here.

1288-1288: Propagation of is_union from paths to Append/MergeAppend plans

Copying best_path->is_union into Append.plan->is_union and MergeAppend.node->is_union is straightforward and matches the described executor-side ROWNUM reset behavior for UNION branches. Given makeNode(Append/MergeAppend) zero-initializes memory, non-UNION cases will safely see false here.

Please double-check that all UNION-style Append/MergeAppend paths that should drive per-branch ROWNUM counters are going through these create_*_plan paths (and not only via make_append), and that no non-UNION callers expect is_union to ever be non-zero. A quick grep over Append/MergeAppend constructors plus existing ROWNUM tests should be enough.

Also applies to: 1579-1579

coderabbitai · 2026-01-07T02:32:49Z

src/backend/optimizer/plan/planner.c

+	foreach(lc, andlist)
+	{
+		Node	   *qual = (Node *) lfirst(lc);
+		OpExpr	   *opexpr;
+		Node	   *leftop;
+		Node	   *rightop;
+		char	   *opname;
+		Const	   *constval;
+		int64		n;
+
+		/* We're looking for OpExpr nodes (comparison operators) */
+		if (!IsA(qual, OpExpr))
+			continue;
+
+		opexpr = (OpExpr *) qual;
+
+		/* Need exactly 2 arguments */
+		if (list_length(opexpr->args) != 2)
+			continue;
+
+		leftop = (Node *) linitial(opexpr->args);
+		rightop = (Node *) lsecond(opexpr->args);
+
+		/* Check if left operand is ROWNUM */
+		if (!IsA(leftop, RownumExpr))
+			continue;
+
+		/* Right operand must be a constant */
+		if (!IsA(rightop, Const))
+			continue;
+
+		/* Get the operator name */
+		opname = get_opname(opexpr->opno);
+		if (opname == NULL)
+			continue;
+
+		/* Now handle different operators */
+		constval = (Const *) rightop;
+
+		if (constval->constisnull)
+		{
+			pfree(opname);
+			continue;
+		}
+
+		/*
+		 * Validate that the constant is a numeric type we can safely convert
+		 * to int64. This prevents undefined behavior from unexpected types.
+		 */
+		if (constval->consttype != INT8OID &&
+			constval->consttype != INT4OID &&
+			constval->consttype != INT2OID)
+		{
+			pfree(opname);
+			continue;
+		}
+
+		/* Extract the integer value based on type */
+		switch (constval->consttype)
+		{
+			case INT8OID:
+				n = DatumGetInt64(constval->constvalue);
+				break;
+			case INT4OID:
+				n = (int64) DatumGetInt32(constval->constvalue);
+				break;
+			case INT2OID:
+				n = (int64) DatumGetInt16(constval->constvalue);
+				break;
+			default:
+				/* Should not reach here due to check above */
+				pfree(opname);
+				continue;
+		}
+
+		if (strcmp(opname, "<=") == 0)
+		{
+			/*
+			 * ROWNUM <= N:
+			 *   N <= 0: always false (ROWNUM starts at 1)
+			 *   N > 0: can be optimized to LIMIT N (only for simple queries)
+			 */
+			if (n <= 0)
+			{
+				/* Always false - rewrite as FALSE constant */
+				BoolExpr   *newand;
+				Const	   *falseconst = (Const *) makeBoolConst(false, false);
+
+				andlist = list_delete_ptr(andlist, qual);
+				andlist = lappend(andlist, falseconst);
+
+				/* Rebuild WHERE clause */
+				if (list_length(andlist) == 0)
+					jointree->quals = NULL;
+				else if (list_length(andlist) == 1)
+					jointree->quals = (Node *) linitial(andlist);
+				else
+				{
+					newand = makeNode(BoolExpr);
+					newand->boolop = AND_EXPR;
+					newand->args = andlist;
+					newand->location = -1;
+					jointree->quals = (Node *) newand;
+				}
+
+				pfree(opname);
+				return;
+			}
+			else if (can_use_limit)
+			{
+				limit_value = n;
+				rownum_qual = qual;
+			}
+			pfree(opname);
+			break;
+		}
+		else if (strcmp(opname, "=") == 0)
+		{
+			/*
+			 * ROWNUM = 1 can be optimized to LIMIT 1 (only for simple queries).
+			 * ROWNUM = N where N != 1 is always false (Oracle semantics) - always process.
+			 */
+			if (n == 1 && can_use_limit)
+			{
+				limit_value = n;
+				rownum_qual = qual;
+			}
+			else if (n != 1)
+			{
+				/* ROWNUM = N where N != 1 is always false */
+				BoolExpr   *newand;
+				Const	   *falseconst;
+
+				falseconst = (Const *) makeBoolConst(false, false);
+
+				/* Replace this qual with FALSE in the AND list */
+				andlist = list_delete_ptr(andlist, qual);
+				andlist = lappend(andlist, falseconst);
+
+				/* Rebuild the WHERE clause */
+				if (list_length(andlist) == 0)
+				{
+					jointree->quals = NULL;
+				}
+				else if (list_length(andlist) == 1)
+				{
+					jointree->quals = (Node *) linitial(andlist);
+				}
+				else
+				{
+					newand = makeNode(BoolExpr);
+					newand->boolop = AND_EXPR;
+					newand->args = andlist;
+					newand->location = -1;
+					jointree->quals = (Node *) newand;
+				}
+
+				pfree(opname);
+				return;
+			}
+			pfree(opname);
+			break;
+		}
+		else if (strcmp(opname, "<") == 0)
+		{
+			/*
+			 * ROWNUM < N:
+			 *   N <= 1: always false (ROWNUM starts at 1, so < 1 is impossible)
+			 *   N > 1: can be optimized to LIMIT N-1 (only for simple queries)
+			 */
+			if (n <= 1)
+			{
+				/* Always false - rewrite as FALSE constant */
+				BoolExpr   *newand;
+				Const	   *falseconst = (Const *) makeBoolConst(false, false);
+
+				andlist = list_delete_ptr(andlist, qual);
+				andlist = lappend(andlist, falseconst);
+
+				/* Rebuild WHERE clause */
+				if (list_length(andlist) == 0)
+					jointree->quals = NULL;
+				else if (list_length(andlist) == 1)
+					jointree->quals = (Node *) linitial(andlist);
+				else
+				{
+					newand = makeNode(BoolExpr);
+					newand->boolop = AND_EXPR;
+					newand->args = andlist;
+					newand->location = -1;
+					jointree->quals = (Node *) newand;
+				}
+
+				pfree(opname);
+				return;
+			}
+			else if (can_use_limit)
+			{
+				limit_value = n - 1;
+				rownum_qual = qual;
+			}
+			pfree(opname);
+			break;
+		}
+		else if (strcmp(opname, ">") == 0)
+		{
+			/*
+			 * ROWNUM > N:
+			 *   N >= 1: always false
+			 *   N < 1: tautology, remove qual
+			 */
+			BoolExpr   *newand;
+
+			andlist = list_delete_ptr(andlist, qual);
+
+			if (n >= 1)
+			{
+				/* Always false - add FALSE constant */
+				Const *falseconst = (Const *) makeBoolConst(false, false);
+				andlist = lappend(andlist, falseconst);
+			}
+			/* else: tautology, just remove qual */
+
+			/* Rebuild WHERE clause */
+			if (list_length(andlist) == 0)
+				jointree->quals = NULL;
+			else if (list_length(andlist) == 1)
+				jointree->quals = (Node *) linitial(andlist);
+			else
+			{
+				newand = makeNode(BoolExpr);
+				newand->boolop = AND_EXPR;
+				newand->args = andlist;
+				newand->location = -1;
+				jointree->quals = (Node *) newand;
+			}
+
+			pfree(opname);
+			return;
+		}
+		else if (strcmp(opname, ">=") == 0)
+		{
+			/*
+			 * ROWNUM >= N:
+			 *   N > 1: always false
+			 *   N <= 1: tautology, remove qual
+			 */
+			BoolExpr   *newand;
+
+			andlist = list_delete_ptr(andlist, qual);
+
+			if (n > 1)
+			{
+				/* Always false - add FALSE constant */
+				Const *falseconst = (Const *) makeBoolConst(false, false);
+				andlist = lappend(andlist, falseconst);
+			}
+			/* else: tautology, just remove qual */
+
+			/* Rebuild WHERE clause */
+			if (list_length(andlist) == 0)
+				jointree->quals = NULL;
+			else if (list_length(andlist) == 1)
+				jointree->quals = (Node *) linitial(andlist);
+			else
+			{
+				newand = makeNode(BoolExpr);
+				newand->boolop = AND_EXPR;
+				newand->args = andlist;
+				newand->location = -1;
+				jointree->quals = (Node *) newand;
+			}
+
+			pfree(opname);
+			return;
+		}
+
+		pfree(opname);
+	}


⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's verify the file exists and read the relevant lines wc -l src/backend/optimizer/plan/planner.c

Repository: IvorySQL/IvorySQL

Length of output: 102

🏁 Script executed:

# Read the specific lines mentioned in the review sed -n '719,997p' src/backend/optimizer/plan/planner.c | head -100

Repository: IvorySQL/IvorySQL

Length of output: 2389

🏁 Script executed:

# Let's see more context around get_opname call sed -n '719,850p' src/backend/optimizer/plan/planner.c

Repository: IvorySQL/IvorySQL

Length of output: 3064

🏁 Script executed:

# Check the complete structure of the foreach loop to understand break statements sed -n '719,997p' src/backend/optimizer/plan/planner.c

Repository: IvorySQL/IvorySQL

Length of output: 6390

🏁 Script executed:

# Verify the exact line locations of break statements to confirm loop exit behavior sed -n '780,850p' src/backend/optimizer/plan/planner.c | grep -n "break\|return" | head -20

Repository: IvorySQL/IvorySQL

Length of output: 127

🏁 Script executed:

# Check if andlist is modified during the loop iterations that have break sed -n '780,850p' src/backend/optimizer/plan/planner.c | grep -n "andlist"

Repository: IvorySQL/IvorySQL

Length of output: 323

🏁 Script executed:

# Double-check the exact sequence: when is get_opname called vs when Const is validated? sed -n '745,770p' src/backend/optimizer/plan/planner.c | cat -n

Repository: IvorySQL/IvorySQL

Length of output: 836

🏁 Script executed:

# Verify all paths that call get_opname also free it before exiting sed -n '719,997p' src/backend/optimizer/plan/planner.c | grep -B5 -A5 "pfree(opname)"

Repository: IvorySQL/IvorySQL

Length of output: 1915

🏁 Script executed:

# Trace the exact control flow: get_opname is called when? sed -n '745,760p' src/backend/optimizer/plan/planner.c

Repository: IvorySQL/IvorySQL

Length of output: 370

🏁 Script executed:

# Count how many places pfree(opname) appears and in what contexts grep -n "pfree(opname)" src/backend/optimizer/plan/planner.c | head -20

Repository: IvorySQL/IvorySQL

Length of output: 327

Address the early loop exit that can miss subsequent ROWNUM predicates

The loop structure has one legitimate issue: the break statements in the <=, =, and < branches (after recording a transformable ROWNUM predicate) exit the foreach loop prematurely. This means that if a later ROWNUM predicate in the same AND-list requires an Oracle-semantic "always false" rewrite (like ROWNUM > 1 or ROWNUM = 2), it won't be examined.

For example, if the andlist contains [ROWNUM <= 10, ROWNUM > 1]:

The first predicate matches the <= branch, sets limit_value=10, and executes break.

The second predicate is never seen, so the > branch never executes to fold it to FALSE.

The query incorrectly proceeds with both conditions instead of being marked unsatisfiable.

Replace the break statements in the <=, =, and < branches with continue so the loop scans all remaining ROWNUM predicates in the AND-list.

Note: The get_opname() function is called after validating the right operand is a non-null integer Const, and all code paths properly free it before continuing or returning, so there is no memory leak.

🤖 Prompt for AI Agents

In @src/backend/optimizer/plan/planner.c around lines 719 - 997, The loop over ROWNUM clauses in the foreach scanning andlist exits early because the branches handling opname "<=", "=", and "<" use break, which leaves later ROWNUM predicates (e.g., ones that make the query always false) unexamined; change those break statements to continue so the foreach loop keeps scanning all quals (ensure you preserve setting limit_value and rownum_qual, and that opname is still pfree'd on every path), i.e., in the branches that currently do "if (can_use_limit) { limit_value = n; rownum_qual = qual; } pfree(opname); break;" (and the "<" variant using n-1) replace the final break with continue so the loop examines remaining quals in andlist.

bigplaice · 2026-01-07T07:32:00Z

@rophy

Thanks for so quick fix!
Here is another finding in cursor scenario.

This is the procedure to reproduce problem:

create base table.
CREATE TABLE test_cursor (c1 serial, c2 int);
prepare the test data
create or replace procedure prepare_test_cursor()
as
i int;
begin
for i in 1..60 loop
insert into test_cursor(c2) values (i);
end loop;
end;
/

call prepare_test_cursor();

create cursor
BEGIN;
DECLARE C1 SCROLL CURSOR FOR
SELECT *
FROM (SELECT *, rownum rn FROM test_cursor WHERE rownum <= 25) AS foo
WHERE rn > 5 LIMIT 10 OFFSET 5;
ok to retrieve all data
FETCH 50 IN C1;
c1 | c2 | rn
----+----+----
11 | 11 | 11
12 | 12 | 12
13 | 13 | 13
14 | 14 | 14
15 | 15 | 15
16 | 16 | 16
17 | 17 | 17
18 | 18 | 18
19 | 19 | 19
20 | 20 | 20
(10 rows)
'first' & 'last' work well too
FETCH FIRST IN C1;
c1 | c2 | rn
----+----+----
11 | 11 | 11
(1 row)

FETCH LAST IN C1;
c1 | c2 | rn
----+----+----
20 | 20 | 20
(1 row)

'prior' and 'next' have problem as the rn always gets incremented by1
FETCH PRIOR IN C1;
c1 | c2 | rn
----+----+----
19 | 19 | 21
(1 row)

FETCH NEXT IN C1;
c1 | c2 | rn
----+----+----
20 | 20 | 22
(1 row)

bigplaice · 2026-01-07T07:45:03Z

A general comment:
Since the 'rownum' is listed as a keyword in ora_gram.y, it is not allowed to use it as function argument name. While Oracle does allow.

For example,

create or replace function test_f(rownum in out varchar2) return integer is
begin
raise notice '%', rownum;
return 23;
end;

declare
rownum varchar2(256);
ret integer;
begin
ret := test_f(rownum);
end;
/
ERROR: function test_inquote(Rownum => pg_catalog.int8) does not exist
LINE 1: ret := test_inquote("Rownum"=>rownum)
^
HINT: No function matches the given name and argument types. You might need to add explicit type casts.
QUERY: ret := test_inquote("Rownum"=>rownum)
CONTEXT: PL/iSQL function inline_code_block line 5 at assignment

Another effect of specifying 'rownum' as keyword may be:
CREATE TABLE eemp(
empno NUMBER(4) NOT NULL CONSTRAINT emp_pk PRIMARY KEY,
ename VARCHAR2(10),
job VARCHAR2(9),
mgr NUMBER(4),
hiredate DATE,
sal NUMBER(7,2) CONSTRAINT emp_sal_ck CHECK (sal > 0),
comm NUMBER(7,2),
deptno NUMBER(2)
);

Documents bug where FETCH PRIOR/NEXT re-evaluates ROWNUM instead of using materialized values from original query. Ref: IvorySQL#1000 (comment)

For scroll cursors containing ROWNUM expressions, materialize all results into a tuplestore on first execution. This ensures FETCH PRIOR/NEXT/RELATIVE return the same ROWNUM values computed during initial scan, rather than re-evaluating (which caused incorrect incrementing values). Added plan_contains_rownum() to detect ROWNUM in plan trees, handling SubqueryScan, Append, and MergeAppend nodes. Fixes: IvorySQL#1000 (comment)

Documents bug where FETCH PRIOR/NEXT re-evaluates ROWNUM instead of using materialized values from original query. Ref: IvorySQL#1000 (comment)

For scroll cursors containing ROWNUM expressions, materialize all results into a tuplestore on first execution. This ensures FETCH PRIOR/NEXT/RELATIVE return the same ROWNUM values computed during initial scan, rather than re-evaluating (which caused incorrect incrementing values). Added plan_contains_rownum() to detect ROWNUM in plan trees, handling SubqueryScan, Append, and MergeAppend nodes. Fixes: IvorySQL#1000 (comment)

Tests that FETCH PRIOR/NEXT preserve both outer_rn and inner_rn values when using scroll cursors with multiple ROWNUM nesting levels.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/oracle_test/regress/sql/rownum.sql (1)
577-612: Consider adding explicit test markers for known limitations.

The INTERSECT/EXCEPT (lines 577-594) and LATERAL (lines 597-612) sections document known limitations where IvorySQL behavior differs from Oracle. While the comments are clear, consider adding visual markers like -- KNOWN LIMITATION TEST at the start of each section to make it obvious these tests document current (non-Oracle) behavior rather than validate correct Oracle semantics.
📝 Suggested enhancement for clarity
 --
+-- KNOWN LIMITATION TEST
 -- INTERSECT and EXCEPT with ROWNUM
 -- NOTE: Oracle resets ROWNUM for each side of INTERSECT/EXCEPT independently.
 --
+-- KNOWN LIMITATION TEST  
 -- LATERAL join with ROWNUM
 -- NOTE: Oracle resets ROWNUM for each outer row in LATERAL/CROSS APPLY.

📜 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 91730a3 and fc279e0.

⛔ Files ignored due to path filters (1)

src/oracle_test/regress/expected/rownum.out is excluded by !**/*.out

📒 Files selected for processing (2)

src/backend/tcop/pquery.c
src/oracle_test/regress/sql/rownum.sql

🧰 Additional context used

🧬 Code graph analysis (1)

src/backend/tcop/pquery.c (2)

src/backend/utils/mmgr/portalmem.c (1)

PortalCreateHoldStore (330-361)

src/backend/utils/sort/tuplestore.c (1)

tuplestore_rescan (1284-1314)

🔇 Additional comments (16)

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

1-40: LGTM! Clean test setup and basic ROWNUM coverage.

The initial setup and basic ROWNUM tests provide a solid foundation. The test patterns correctly cover the simple cases that should trigger LIMIT optimization (ROWNUM <= N, = 1, < N).

659-663: LGTM! Oracle type usage is appropriate for compatibility testing.

Using Oracle-specific types (NUMBER, VARCHAR2) in the oracle_test directory correctly validates IvorySQL's Oracle type compatibility.

680-712: LGTM! Critical pagination pattern test.

This test validates the nested-subquery pagination fix mentioned in PR objectives (bigplaice's reported bug). The pattern and expected results are well-documented, making it an excellent regression test.

882-955: LGTM! Comprehensive scroll cursor regression tests.

These tests validate the scroll cursor materialization fix (preserving ROWNUM values across FETCH PRIOR/NEXT/RELATIVE operations) mentioned in the PR objectives. Coverage is excellent with both simple and deeply nested ROWNUM scenarios.

790-879: LGTM! Thorough aggregate function testing.

Excellent coverage of aggregate functions with ROWNUM, GROUP BY, and ORDER BY. These tests validate fixes for the reported aggregate issues (lines 799-817 test column order sensitivity, lines 825-879 test multi-layer aggregation). The variety of aggregate functions and nesting levels provides strong regression protection.

512-570: LGTM! Essential UNION ROWNUM reset tests.

These tests validate the is_union flag functionality mentioned in PR objectives, ensuring each UNION branch maintains independent ROWNUM counters (Oracle behavior). Good coverage of both UNION ALL (Append) and UNION (MergeAppend + Unique) cases, plus multiple-branch scenarios.

321-381: LGTM! Comprehensive correlated subquery tests.

Excellent validation of ROWNUM reset behavior in correlated subqueries (Issue #14 mentioned in PR objectives). Tests cover all critical scenarios: single, multiple, nested, and JOIN cases. This ensures the ExecSubPlan save/restore logic works correctly.

199-246: LGTM! Critical optimizer transformation verification.

These EXPLAIN tests validate the planner's ROWNUM→LIMIT transformations mentioned in PR objectives. Particularly important are the Issue #12 tests (lines 237-245) ensuring the optimizer correctly avoids LIMIT transformation when same-level ORDER BY/DISTINCT/aggregation would change semantics.

615-641: LGTM! DML operations with ROWNUM validated.

Tests cover DELETE and UPDATE with ROWNUM as mentioned in PR objectives. Coverage is sufficient for regression detection, with verification queries confirming correct behavior.

116-150: LGTM! Comprehensive edge case coverage.

Excellent testing of Oracle ROWNUM semantics edge cases: tautologies (ROWNUM > 0), impossible conditions (ROWNUM = 2), negative values, and complex predicates. These validate both runtime behavior and optimizer handling.

1-962: Excellent comprehensive test suite for ROWNUM implementation.

This test file provides outstanding coverage of the ROWNUM feature:

✅ Core functionality: Basic queries, ORDER BY, JOINs, subqueries
✅ Critical bug fixes: Pagination (bigplaice's nested-subquery bug), scroll cursors, aggregates, GROUP BY
✅ Oracle semantics: Edge cases, tautologies, impossible conditions
✅ PR objectives: UNION resets, correlated subquery resets, DML operations, optimizer transformations
✅ Known limitations: INTERSECT/EXCEPT and LATERAL documented with tests
✅ Verification: EXPLAIN tests confirm optimizer transformations

The test organization is logical, well-commented, and includes verification queries. This will provide strong regression protection for the ROWNUM feature.

723-724: No action needed. The error case is already properly validated through the expected output file (rownum.out line 1232), which documents the expected ERROR message. PostgreSQL regression tests validate by comparing actual output against expected output, so the presence of the ERROR message in the expected file ensures the test will fail if the error doesn't occur.

Likely an incorrect or invalid review comment.

src/backend/tcop/pquery.c (4)

25-26: LGTM!

The added includes are necessary for the new ROWNUM detection and materialization logic:

nodes/nodeFuncs.h provides expression_tree_walker

nodes/plannodes.h provides Plan node type definitions

152-188: LGTM - Materialization trade-off is necessary for correctness.

The function correctly materializes scroll cursor results by:

Running the query to completion

Storing all tuples in a tuplestore

Resetting the store for subsequent fetches

Properly managing snapshots and receivers

This ensures ROWNUM values remain stable across FETCH PRIOR/NEXT operations. While full materialization can be expensive for large result sets, the tuplestore uses work_mem limits and can spill to disk, and this approach is necessary to preserve ROWNUM semantics for scroll cursors.

1021-1034: LGTM - Conditional materialization correctly integrated.

The integration logic correctly detects scroll cursors with ROWNUM and triggers materialization on first execution:

Checks are appropriately ordered with short-circuit on holdStore == NULL

plan_contains_rownum is only called once per cursor (before materialization)

Non-scroll cursors are unaffected, as forward-only fetches don't need materialization

Subsequent fetches automatically use RunFromStore (line 1070)

The comment clearly explains the rationale for this approach.

69-150: No concerns identified; separate executor mechanisms correctly handle ROWNUM in InitPlan/SubPlan contexts.

The plan_contains_rownum function is correctly scoped to detect ROWNUM in the main plan's targetlist and child plans. ROWNUM in InitPlan/SubPlan lists is handled separately at execution time via the es_rownum save/restore mechanism (nodeSubplan.c lines 69, 101, 1122, 1284; nodeSubqueryscan.c lines 50, 58, 66, 67). Since InitPlan/SubPlan don't appear in targetlists, they don't affect the scroll cursor materialization decision made by this planner-time check.

Documents current IvorySQL behavior vs Oracle for ROWNUM: 1. ROWNUM as column/table alias (rownum.sql): - Oracle: Does NOT allow ROWNUM as alias (ERROR) - IvorySQL: Allows it (differs from Oracle) 2. ROWNUM as PL/iSQL parameter/variable (ora_plisql.sql): - Oracle: Allows, treats as regular identifier - IvorySQL: Creates function but treats 'rownum' as pseudocolumn Ref: IvorySQL#1000 (comment)

Add semantic validation to reject ROWNUM and ROWID as aliases in Oracle compatibility mode, matching Oracle's behavior: - Column alias: SELECT 1 rownum → ERROR - Column alias with AS: SELECT 1 AS rownum → ERROR - Table alias: SELECT * FROM t rownum → ERROR - Subquery alias: SELECT * FROM (SELECT 1) rownum → ERROR Validation added in: - parse_target.c:transformTargetEntry() for column aliases - parse_relation.c:addRangeTableEntry() for table aliases - parse_relation.c:addRangeTableEntryForSubquery() for subquery aliases

In PL/iSQL's pre-columnref hook, check if ROWNUM or ROWID identifiers resolve to declared variables. This prevents the SQL parser from converting them to pseudocolumn references, matching Oracle behavior where these can be used as variable/parameter names in PL/SQL. The fix adds special handling in plisql_pre_column_ref() to check ROWNUM/ROWID names even when resolve_option is not PLISQL_RESOLVE_VARIABLE, since the SQL parser would otherwise intercept them first.

Verify that Oracle-specific keywords (ROWNUM, ROWID, etc.) have no special meaning in PostgreSQL mode: - Can be used as column names - Can be used as column/table aliases - Can be used as function parameters and variables - Can be used as CTE names This ensures Oracle-specific functionality doesn't leak into PostgreSQL mode.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/oracle_test/regress/sql/rownum.sql (1)
980-985: Consider using CASCADE for dept table cleanup.

The dept table is used in JOIN tests earlier in the file. Using CASCADE ensures cleanup succeeds even if there are dependencies, matching the pattern used for rownum_test.
Minor cleanup improvement
 DROP TABLE rownum_test CASCADE;
-DROP TABLE dept CASCADE;
+DROP TABLE dept;
Actually, looking again, both already use CASCADE which is correct. No change needed.
The cleanup is already correct with CASCADE on both tables.

📜 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 fc279e0 and 7412485.

⛔ Files ignored due to path filters (3)

src/oracle_test/regress/expected/ora_plisql.out is excluded by !**/*.out
src/oracle_test/regress/expected/rownum.out is excluded by !**/*.out
src/test/regress/expected/ora_keywords_pg.out is excluded by !**/*.out

📒 Files selected for processing (7)

src/backend/parser/parse_relation.c
src/backend/parser/parse_target.c
src/oracle_test/regress/sql/ora_plisql.sql
src/oracle_test/regress/sql/rownum.sql
src/pl/plisql/src/pl_comp.c
src/test/regress/parallel_schedule
src/test/regress/sql/ora_keywords_pg.sql

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-10-24T08:19:29.453Z

Learnt from: OreoYang
Repo: IvorySQL/IvorySQL PR: 931
File: src/backend/oracle_parser/ora_gram.y:0-0
Timestamp: 2025-10-24T08:19:29.453Z
Learning: In IvorySQL’s parser (src/backend/oracle_parser/ora_gram.y and related headers), downcase_identifier has the 4-argument signature:
extern char *downcase_identifier(const char *ident, int len, bool warn, bool truncate);

Applied to files:

src/backend/parser/parse_target.c

🧬 Code graph analysis (3)

src/backend/parser/parse_target.c (3)

src/backend/utils/error/elog.c (2)

errcode (853-864)

errmsg (1070-1086)

src/backend/parser/parse_node.c (1)

parser_errposition (108-123)

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

exprLocation (1422-1857)

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

src/backend/utils/error/elog.c (2)

errcode (853-864)

errmsg (1070-1086)

src/backend/parser/parse_node.c (1)

parser_errposition (108-123)

src/pl/plisql/src/pl_comp.c (1)

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

list_length (151-155)

🔇 Additional comments (13)

src/pl/plisql/src/pl_comp.c (2)

37-37: LGTM: Include added for Oracle compatibility checks.

The include of utils/ora_compatible.h is necessary for the compatible_db and ORA_PARSER checks added below.

614-642: LGTM: Correct handling of ROWNUM/ROWID as PL/iSQL variable names.

The logic correctly allows ROWNUM and ROWID to be used as PL/iSQL variable/parameter names in Oracle compatibility mode by:

Intercepting in the pre-columnref hook before the SQL parser converts them to pseudocolumns

Attempting to resolve them as PL/iSQL variables first

Only applying this special handling for unqualified single-field references in ORA_PARSER mode

Falling back to normal parsing (pseudocolumn creation) if no variable is found

This matches Oracle's PL/SQL behavior where ROWNUM/ROWID can be used as variable names.

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

32-34: LGTM: Includes added for Oracle compatibility checks.

The includes are necessary for the compatible_db and ORA_PARSER checks added below.

99-116: Table alias validation for ROWNUM/ROWID in Oracle mode is already properly implemented in src/backend/parser/parse_relation.c (lines 1521-1533 for table aliases and lines 1746-1756 for subquery aliases), with comprehensive tests in src/oracle_test/regress/sql/rownum.sql (lines 958-977) and src/test/regress/sql/ora_keywords_pg.sql (lines 29-33). The implementation is complete across all contexts required by the PR objectives.

src/test/regress/sql/ora_keywords_pg.sql (1)

1-83: LGTM: Comprehensive PostgreSQL-mode keyword tests.

This test file thoroughly validates that Oracle-specific keywords (ROWNUM, ROWID) have no special meaning in PostgreSQL mode and are treated as regular identifiers. The coverage includes:

Column names and aliases (with and without AS)

Table and subquery aliases

Expressions and function parameters

PL/pgSQL variables

CTEs

Proper cleanup

The tests complement the Oracle-mode tests by ensuring Oracle functionality doesn't leak into PostgreSQL mode.

src/test/regress/parallel_schedule (1)

144-145: LGTM: Test added to parallel schedule.

The new test is appropriately placed at the end of the schedule with a clear comment describing its purpose.

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

1516-1533: LGTM with a minor suggestion: consider using alias location for more precise error positioning.

The error position uses relation->location which points to the relation name, not the alias. Using the alias's location (if available) would provide more accurate error positioning for the user. However, since Alias doesn't have a location field, relation->location is a reasonable fallback.

1741-1756: Missing parser_errposition in subquery alias validation, but proposed fix references non-existent field.

This block omits parser_errposition() in the error reporting, making it inconsistent with the similar check in addRangeTableEntry (lines 1524-1532). However, the proposed fix of using parser_errposition(pstate, alias->location) is not feasible—the Alias struct does not have a location field (it only contains type, aliasname, and colnames).

To address this, either:

Add a location field to the Alias struct, or

Modify addRangeTableEntryForSubquery to accept and use a location parameter passed from transformRangeSubselect

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

1460-1497: Well-structured tests for ROWNUM as PL/iSQL identifier.

The tests comprehensively cover:

Function parameter named rownum

Local variable named rownum

Named parameter syntax (rownum => 'value')

Good defensive cleanup with DROP FUNCTION IF EXISTS. The GitHub issue reference provides helpful context.

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

1-24: LGTM: Test setup is clean and comprehensive.

Good foundational test data with varied values for id, name, and value columns to support diverse test scenarios.

571-612: Excellent documentation of known limitations.

The comments clearly explain the behavioral differences between Oracle and IvorySQL for INTERSECT/EXCEPT and LATERAL joins. This helps users understand expected behavior and provides a roadmap for future improvements. The "NOTE" sections effectively communicate that these are known limitations, not bugs.

880-955: Thorough scroll cursor testing for ROWNUM materialization.

These tests verify that ROWNUM values are materialized and preserved across cursor movements (FETCH PRIOR/NEXT), which is critical for correct Oracle-compatible behavior. The nested ROWNUM cursor test adds extra coverage for multi-level scenarios.

956-978: Alias restriction tests align with parse_relation.c changes.

These tests validate the new Oracle compatibility checks that reject ROWNUM/ROWID as column and table aliases. The error comments reference Oracle's actual error messages, which is helpful for verification.

rophy · 2026-01-09T10:03:05Z

Cursor:

tests in commit d6d8e08 and commit fc279e0
fixed in commit 5c54046
AI says materializing costs memory, but is reasonable because we'll need to transverse the results anyway. Is that right?

When to recognize as keywords or variables - looks complicated:

tests in commit ede7906
fixes in commit 42cb0d3 and commit 61d2053
commit 7412485 added tests to postgres regression to make sure those keywords doesn't "pollute" postgres

coderabbitai bot reviewed Dec 3, 2025

View reviewed changes

src/backend/optimizer/plan/planner.c Show resolved Hide resolved

rophy added 3 commits December 3, 2025 16:19

rophy force-pushed the feat/rownum branch from 7a0723c to f66e6ce Compare December 3, 2025 16:20

coderabbitai bot reviewed Dec 3, 2025

View reviewed changes

src/backend/optimizer/plan/createplan.c Show resolved Hide resolved

src/backend/optimizer/plan/planner.c Outdated Show resolved Hide resolved

rophy mentioned this pull request Dec 4, 2025

Oracle Compatibility Gap Analysis rophy/IvorySQL#4

Open

coderabbitai bot reviewed Dec 8, 2025

View reviewed changes

src/backend/optimizer/plan/createplan.c Outdated Show resolved Hide resolved

refactor: fix replace_rownum_expr_mutator signature

a8171fa

Change replace_rownum_expr_mutator to use void* context parameter to match expression_tree_mutator callback convention. This is the same fix applied to count_rownum_exprs_walker in the previous commit.

coderabbitai bot reviewed Dec 8, 2025

View reviewed changes

gaoxueyu mentioned this pull request Jan 3, 2026

oracle compatibility feature: Oracle ROWNUM #1070

Open

rophy and others added 7 commits January 7, 2026 02:16

test: add ROWNUM pagination test for nested subqueries

3a9f276

Add test case for Oracle pagination pattern with nested subqueries. This exposes a bug where shared es_rownum counter causes skipped values (8,10,12,14 instead of 8,9,10,11,12) in nested subquery scans.

test: add nested aggregate function tests for ROWNUM

0455343

Adds test cases for: - Subquery with aggregate containing ROWNUM - Aggregate of subquery with ROWNUM in SELECT - Nested subquery aggregates with GROUP BY - Multiple levels of aggregation

coderabbitai bot reviewed Jan 7, 2026

View reviewed changes

rophy added a commit to rophy/IvorySQL that referenced this pull request Jan 8, 2026

test: add scroll cursor ROWNUM bug regression test

387dfae

Documents bug where FETCH PRIOR/NEXT re-evaluates ROWNUM instead of using materialized values from original query. Ref: IvorySQL#1000 (comment)

rophy added 3 commits January 8, 2026 14:06

test: add scroll cursor ROWNUM bug regression test

d6d8e08

Documents bug where FETCH PRIOR/NEXT re-evaluates ROWNUM instead of using materialized values from original query. Ref: IvorySQL#1000 (comment)

test: add deeply nested ROWNUM scroll cursor test

fc279e0

Tests that FETCH PRIOR/NEXT preserve both outer_rn and inner_rn values when using scroll cursors with multiple ROWNUM nesting levels.

coderabbitai bot reviewed Jan 8, 2026

View reviewed changes

rophy added 4 commits January 8, 2026 15:40

coderabbitai bot reviewed Jan 8, 2026

View reviewed changes

feat: ROWNUM Implementation #1000

Are you sure you want to change the base?

feat: ROWNUM Implementation #1000

Uh oh!

Conversation

rophy commented Dec 3, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Oracle ROWNUM Semantics

Key Oracle Behaviors

IvorySQL Implementation

Architecture

Key Source Files

Execution Flow

Basic ROWNUM in WHERE Clause

ROWNUM with Non-ROWNUM Filters

ROWNUM with ROWNUM Conditions

SubqueryScan with ORDER BY

Correlated Subqueries

Code Changes Summary

execScan.h - Pre-increment with Revert

nodeSubqueryscan.c - Reset on First Tuple

nodeSubplan.c - Save/Restore for Correlated Subqueries

Oracle Compatibility Test Results

Passing Tests (Match Oracle)

UNION/UNION ALL ROWNUM Support

Implementation

MergeAppend Specifics

Performance Considerations

Testing

DML with ROWNUM

Known Limitations

INTERSECT/EXCEPT ROWNUM Reset

LATERAL Subquery ROWNUM Reset

Future Work

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

bigplaice commented Jan 6, 2026

Uh oh!

bigplaice commented Jan 6, 2026

Uh oh!

bigplaice commented Jan 6, 2026

Actually got: rownum

Uh oh!

bigplaice commented Jan 6, 2026

Uh oh!

rophy commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

rophy commented Dec 3, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 3, 2025 •

edited

Loading

Actually got:
rownum

rophy commented Jan 7, 2026 •

edited

Loading

bigplaice commented Jan 7, 2026 •

edited

Loading