cockroachdb / cockroachdb/cockroach

sql/plpgsql: out-of-range placeholder in a lazily built routine body reads the caller's placeholder value

Open
#172,995 1 comment 0 reactions 0 assignees View on GitHub
A-sql-optimizer A-sql-plpgsql A-sql-routine branch-master C-bug O-agent T-sql-foundations
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

An out-of-range ordinal parameter reference in a routine body (e.g. `$1` in a
routine with zero parameters) is normally rejected with
`42P02 no value provided for placeholder`, because the body is built when the
routine is created.

When a body statement is instead built *lazily*, that check does not happen. The
placeholder is resolved against whatever `evalCtx` is active at build time —
which, for a lazily built body, is the enclosing query's. If the caller is a
prepared statement with placeholders, the routine silently reads the *caller's*
placeholder value instead of erroring.

The trigger is a deferred body seam, not any single feature. Two independent
things create one:

- A DDL statement earlier in the body, which defers everything after it. This
requires `sql.procedures.plpgsql.late_binding.enabled = true`.
- Dynamic `EXECUTE`, whose command string is only known at execution time. This
is always deferred and needs no cluster setting.

Observed behavior across the combinations:

| Body | Late binding | Result |
|---|---|---|
| Static SQL | off | `42P02` at `CREATE PROCEDURE` |
| Static SQL, no DDL | on | `42P02` when the body is first built |
| Static SQL, DDL earlier in body | on | **reads caller's `$1`** |
| Dynamic `EXECUTE` | n/a | **reads caller's `$1`** |

Note that late binding alone is not sufficient — it only changes *when* the body
is built, and the reference is still caught as long as something builds it
eagerly.

**To Reproduce**

Static SQL, deferred by a preceding DDL:

```sql
SET CLUSTER SETTING sql.procedures.plpgsql.late_binding.enabled = true;

CREATE PROCEDURE p() LANGUAGE PLpgSQL AS $$
BEGIN
CREATE TABLE d (x INT); -- forces lazy building of the following statement
INSERT INTO d VALUES ($1); -- p has zero parameters, so $1 is out of range
END $$;

CREATE FUNCTION f() RETURNS INT LANGUAGE PLpgSQL AS $$
BEGIN CALL p(); RETURN 0; END $$;

PREPARE q AS SELECT f(), $1::INT;
EXECUTE q(99);

SELECT x FROM d;
```

```
x
-----
99 <- the caller's $1
```

Dynamic `EXECUTE`, with late binding off:

```sql
CREATE TABLE d_exec (x INT);

CREATE PROCEDURE p_exec() LANGUAGE PLpgSQL AS $$
BEGIN
EXECUTE 'INSERT INTO d_exec VALUES ($1)';
END $$;

CREATE FUNCTION f_exec() RETURNS INT LANGUAGE PLpgSQL AS $$
BEGIN CALL p_exec(); RETURN 0; END $$;

PREPARE q_exec AS SELECT f_exec(), $1::INT;
EXECUTE q_exec(77);

SELECT x FROM d_exec;
```

```
x
-----
77 <- the caller's $1
```

Both reproduce on `master`.

**Expected behavior**

An out-of-range `$N` in a routine body should be rejected the same way
regardless of when the body is built — `42P02 no value provided for
placeholder: $N`. A routine body should never resolve a placeholder against the
calling query's parameters.

**Additional data / schema**

Root cause is two fall-throughs in the optbuilder:

- [scope.go:1138](https://github.com/cockroachlabs/cockroach/blob/1f38a33e9a79375d9a9225463fb584b02eb5954d/pkg/sql/opt/optbuilder/scope.go#L1138) —
`case *tree.Placeholder` replaces the placeholder with a routine-argument
column via `findFuncArgCol(t.Idx)`. When the ordinal is out of range that
returns nil and the case falls through, leaving the placeholder in place
rather than raising an error.
- [scalar.go:411](https://github.com/cockroachlabs/cockroach/blob/1f38a33e9a79375d9a9225463fb584b02eb5954d/pkg/sql/opt/optbuilder/scalar.go#L411) —
the surviving placeholder then hits `case *tree.Placeholder`, and because
`b.evalCtx.HasPlaceholders()` is true for a body built during execution of an
outer prepared statement, it is folded to a constant via `eval.Expr` using the
caller's `evalCtx.Placeholders`.

When the body is built eagerly, `evalCtx` has no placeholders, so
`ConstructPlaceholder` is built instead and execution later fails with 42P02 —
hence the divergence.

The `NOTE` already on scope.go:1141 anticipates a related problem (placeholders
belonging to a prepared statement rather than to routine arguments).

**Additional context**

Impact is silent wrong results and unintended writes rather than a crash. The
values read belong to the caller's own prepared statement, so this is not
cross-session data access; the notable case is a routine owned by another user,
whose body can observe parameters the caller passed to the surrounding query.

Likely fix direction: reject an out-of-range ordinal at scope.go:1138 instead of
falling through, so the error is raised wherever the body is built. That path is
shared with SQL-language routines, so it needs checking against
`KeepPlaceholders` and the prepared-statement cases the existing NOTE calls out.

Found during review of the dynamic `EXECUTE` work (cockroachlabs/cockroach#2291,
thread on `plpgsql.go:3575`). Not introduced by that PR — it predates it and
affects any lazily built routine body.

Jira issue: CRDB-66294

Epic CRDB-65937

Contributor guide

Open the contributing guide

Research direction

Start with the placeholder handling at scope.go:1138 and scalar.go:411, then reproduce both cases using the SQL examples in the issue. Check how the proposed rejection interacts with KeepPlaceholders and the prepared-statement cases mentioned in the existing NOTE. Done means out-of-range routine placeholders consistently raise 42P02 without regressing those cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.