Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 108 additions & 0 deletions dev/modules/dbix_simple.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
# jcpan DBIx::Simple stabilization

## Overview

`./jcpan -t DBIx::Simple` exercises the vendored OO DBI wrapper. The **source of truth
at runtime** is the copy packed into the fat JAR as **`jar:PERL5LIB`** (built from
[`src/main/perl/lib/DBIx/Simple.pm`](../../src/main/perl/lib/DBIx/Simple.pm) when you **`make`).
GlobalContext **`@INC`** prefers **`PERL5LIB` / `-I`**, then **`~/.perlonjava/lib`**,
then the JAR — so a **leftover** `DBIx/Simple.pm` under **`~/.perlonjava/lib`**
(can happen after older jcpan installs) **shadows** the JDBC-aware bundled copy until
removed or superseded by a reinstall that leaves that path unused.

## Symptoms

| Layer | When it bites | Symptoms |
| ----- | ------------- | ------- |
| A. Stale jcpan-installed `Simple.pm` | **`~/.perlonjava/lib/DBIx/Simple.pm`** appears **before** `jar:PERL5LIB` in **`@INC`** | `keep_statements` stays **16**, `old_statements` recycling clashes with JDBC `finish`/`execute_result` semantics → **`execute` on undef** after chained queries, bogus row counts |
| B. JVM method-chain lifetime | Bundled Simple (`keep_statements == 0` on JDBC) | **`scalar $db->query($sql)->arrays`** returns **too few rows** vs **`my $r = $db->query($sql); scalar $r->arrays`** for the same SQL |
| C. JDBC `fetchrow_hashref` | Upstream **`t/sqlite.t`** with `lc_columns` (default hash keys **`foo`** not **`FOO`**) | **`fetchrow_hashref`** ignored **`NAME_lc`** / **`'NAME_lc'`** argument — hashes missing expected keys |

Layer B points at PerlOnJava **`scalar` / `->` / mortal or refcount boundaries**, not DBIx `wantarray` in `arrays` alone.

## Reproduction

Default (uses whatever **`@INC`** resolves — often JAR unless **`~/.perlonjava`** shadows):

```bash
timeout 120 ./jperl dev/tools/dbix_simple_chain_repro.pl
```

Minimal inline check (**`keep_statements` must be `0` on JDBC** when the **bundled**
`Simple.pm` is the one loaded):

```bash
./jperl -e 'use DBIx::Simple; use DBI; \
my $db=DBIx::Simple->connect(q{dbi:SQLite:dbname=:memory:},"","",{RaiseError=>1}); \
print "keep=".$db->keep_statements."\n"'
```

If that prints **`16`**, you are almost certainly loading a **non-JAR** copy (typically
remove **`$HOME/.perlonjava/lib/DBIx/Simple.pm`** — and parent dirs if empty — then retry).

Optional: exercise an **editable workspace** `.pm` **before rebuilding the JAR** (not how
releases behave):

```bash
./jperl -I"$PWD/src/main/perl/lib" dev/tools/dbix_simple_chain_repro.pl
```

Full module test harness (agents: **wrap jcpan** in `timeout`; capture TAP to a file).
Bundled **`DBIx/Simple.pm`** skips upstream **`t/`** unless you force it:

```bash
JCPAN_RUN_BUNDLED_TESTS=1 timeout 3600 ./jcpan -t DBIx::Simple > /tmp/jcpan_dbix_simple.txt 2>&1
echo EXIT:$? >> /tmp/jcpan_dbix_simple.txt
```

## JDBC /statement notes (bundled fork)

SQLite on PerlOnJava uses JDBC. Recycling finished statement wrappers while the
same `PreparedStatement` is reused is unsafe; bundled `Simple.pm` forces
`keep_statements = 0` when [`DBI::_is_jdbc_handle`](../../src/main/perl/lib/DBI.pm)
returns true (`connection`/`statement`/`ImplementorClass` heuristics).

## Resolution log

| Date | Change |
| ---- | ------ |
| 2026-05-16 | Added this doc + `dev/tools/dbix_simple_chain_repro.pl`. **Layer B**: blessed method-call invocant **refcount hold** across `dispatchPerlMethodAfterSelfInjected` / `callCachedInner` (`RuntimeCode.java`). **Layer C**: JDBC **`DBI.fetchrow_hashref`** honors the optional **second argument** (`NAME_lc`) like `DBI.pm`, fixing **`t/sqlite.t`** hash key expectations under bundled tests. |

## `RuntimeCode.coerceScalarCallResult`

Collapsing multi-return lists at subroutine boundaries matches Perl semantics
and avoids late `RuntimeList.scalar()` on intermediates (see inline comment in
`RuntimeCode`). It alone did **not** fix Layer B; keep it unless regressions justify
narrowing/reverting after the JDBC chain fix lands.

## Next steps

### Implemented in this iteration (carry forward checklist)

| Item | Status |
| ---- | ------ |
| Document layers A/B/C + repro + bundled jcpan knobs | ✓ [`dev/modules/dbix_simple.md`](dbix_simple.md) |
| Repro harness | ✓ [`dev/tools/dbix_simple_chain_repro.pl`](../../dev/tools/dbix_simple_chain_repro.pl) |
| Layer B — blessed invocant refcount across method dispatch | ✓ [`RuntimeCode.java`](../../src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeCode.java) |
| Layer C — JDBC `fetchrow_hashref($sth, 'NAME_lc')` | ✓ [`DBI.java`](../../src/main/java/org/perlonjava/runtime/perlmodule/DBI.java) |
| Regression test for Layer C (`NAME_lc` keys) | ✓ [`unit/dbi_fetchrow_hashref_name_lc.t`](../../src/test/resources/unit/dbi_fetchrow_hashref_name_lc.t) |

### After merge / ongoing

1. **CI / review** — Ensure PR **`make`** and any Cloud CI workflows are green before merge.

2. **Bundled jcpan smoke** — Default `./jcpan -t DBIx::Simple` still skips upstream `t/` when `Simple.pm` is bundled; for full parity use:
```bash
JCPAN_RUN_BUNDLED_TESTS=1 timeout 3600 ./jcpan -t DBIx::Simple > /tmp/jcpan_dbix_simple.txt 2>&1
```

3. **Interpreter parity** (only if regressions reported) — Run the chain repro under `./jperl --interpreter` and compare row counts vs JVM.

4. **Stale `~/.perlonjava`** — If Layer A symptoms return, delete **`$HOME/.perlonjava/lib/DBIx/Simple.pm`** when it should not override the JAR bundle (see [`GlobalContext`](../../src/main/java/org/perlonjava/runtime/runtimetypes/GlobalContext.java) `@INC` ordering).

5. **Optional follow-ups** — If Layer B surfaces again elsewhere, revisit `scalar`/method-chain lowering (e.g. `Dereference` / late `scalar()` collapse) alongside `MortalList` boundaries; extend unit coverage for `FetchHashKeyName` / `NAME_uc` only if bugs appear.

## References

- [`AGENTS.md`](../../AGENTS.md) — jcpan/`timeout`, no orphan JVMs.
- [`dev/modules/jcpan_bundled_dbd_and_convert_ber.md`](jcpan_bundled_dbd_and_convert_ber.md) — bundled-module test knobs.
44 changes: 44 additions & 0 deletions dev/tools/dbix_simple_chain_repro.pl
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#!/usr/bin/env perl
# Repro for DBIx::Simple chained query vs temporaries on PerlOnJava (see dev/modules/dbix_simple.md).
# Run: timeout 120 ./jperl dev/tools/dbix_simple_chain_repro.pl
# (Bundled JDBC fork lives in jar:PERL5LIB unless ~/.perlonjava/lib shadows it.)
# Optional workspace edit cycle: ./jperl -I"$PWD/src/main/perl/lib" dev/tools/dbix_simple_chain_repro.pl

use strict;
use warnings;

use DBIx::Simple ();

my $db = DBIx::Simple->connect( 'dbi:SQLite:dbname=:memory:', '', '',
{ RaiseError => 1 } );

print STDERR "keep_statements=", $db->keep_statements, "\n";

$db->query(q{CREATE TABLE t (id INTEGER NOT NULL)});
$db->query(q{INSERT INTO t VALUES (1),(2),(3)});

my $chain_ref =
scalar $db->query('SELECT id FROM t ORDER BY id')->arrays;

my $r = $db->query('SELECT id FROM t ORDER BY id');
my $stored_ref = scalar $r->arrays;

die "chain: expected ARRAY ref, got "
. ( defined $chain_ref ? ref $chain_ref || 'plain scalar' : 'undef' )
unless ref($chain_ref) eq 'ARRAY';

die "stored: expected ARRAY ref, got "
. ( defined $stored_ref ? ref $stored_ref || 'plain scalar' : 'undef' )
unless ref($stored_ref) eq 'ARRAY';

my $chain_n = scalar @$chain_ref;
my $stored_n = scalar @$stored_ref;

print "chain_rows=$chain_n stored_rows=$stored_n\n";

if ( $chain_n != $stored_n ) {
die "FAIL: scalar \\\$db->query(...)->arrays row count mismatch "
. "(stored=$stored_n, chain=$chain_n)\n";
}

print "OK\n";
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,8 @@ public RuntimeList apply(RuntimeArray args, int callContext) {
WarningBitsRegistry.pushCurrent(warningBitsString);
}
try {
return BytecodeInterpreter.execute(this, args, effectiveContext);
return RuntimeCode.coerceScalarCallResult(
BytecodeInterpreter.execute(this, args, effectiveContext), effectiveContext);
} finally {
if (warningBitsString != null) {
WarningBitsRegistry.popCurrent();
Expand All @@ -318,7 +319,9 @@ public RuntimeList apply(String subroutineName, RuntimeArray args, int callConte
WarningBitsRegistry.pushCurrent(warningBitsString);
}
try {
return BytecodeInterpreter.execute(this, args, effectiveContext, subroutineName);
return RuntimeCode.coerceScalarCallResult(
BytecodeInterpreter.execute(this, args, effectiveContext, subroutineName),
effectiveContext);
} finally {
if (warningBitsString != null) {
WarningBitsRegistry.popCurrent();
Expand Down
83 changes: 63 additions & 20 deletions src/main/java/org/perlonjava/runtime/perlmodule/DBI.java
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,25 @@ public static RuntimeList fetchrow_arrayref(RuntimeArray args, int ctx) {
}, dbh, "fetchrow_arrayref");
}

/** Like Perl: {@code $sth->{$key} || $sth->{NAME}} for column label arrays. */
private static RuntimeArray columnNamesAttribute(RuntimeHash sth, String key) {
if (sth == null) {
return null;
}
String[] tryKeys = "NAME".equals(key) ? new String[] {"NAME"} : new String[] {key, "NAME"};
for (String k : tryKeys) {
RuntimeScalar ref = sth.get(k);
if (ref == null || ref.type == RuntimeScalarType.UNDEF) {
continue;
}
RuntimeArray arr = ref.arrayDeref();
if (arr != null && !arr.isEmpty()) {
return arr;
}
}
return null;
}

/**
* Fetches the next row from a result set as a hash reference.
*
Expand Down Expand Up @@ -632,16 +651,39 @@ public static RuntimeList fetchrow_hashref(RuntimeArray args, int ctx) {
RuntimeHash row = new RuntimeHash();
ResultSetMetaData metaData = rs.getMetaData();

// Get the column name style to use
String nameStyle = sth.get("FetchHashKeyName").toString();
if (nameStyle.isEmpty()) {
// Match DBI.pm: fetchrow_hashref($sth, $name) reads $sth->{$name || FetchHashKeyName || 'NAME'}
String nameStyle = null;
if (args.size() > 1) {
RuntimeScalar nameArg = args.get(1);
if (nameArg != null && nameArg.type != RuntimeScalarType.UNDEF) {
String explicit = nameArg.toString();
if (explicit != null && !explicit.isEmpty()) {
nameStyle = explicit;
}
}
}
if (nameStyle == null) {
RuntimeScalar fk = sth.get("FetchHashKeyName");
if (fk != null && fk.type != RuntimeScalarType.UNDEF) {
String s = fk.toString();
if (!s.isEmpty()) {
nameStyle = s;
}
}
}
if (nameStyle == null) {
nameStyle = "NAME";
}
RuntimeArray columnNames = sth.get(nameStyle).arrayDeref();
int colCount = metaData.getColumnCount();
RuntimeArray columnNames = columnNamesAttribute(sth, nameStyle);
if (columnNames == null || columnNames.size() < colCount) {
throw new IllegalStateException(
"fetchrow_hashref: missing column NAME list for key \"" + nameStyle + "\"");
}

// For each column, add column name -> value pair to hash.
// See fetchrow_arrayref for rationale on UTF-8 encode to BYTE_STRING.
for (int i = 1; i <= metaData.getColumnCount(); i++) {
for (int i = 1; i <= colCount; i++) {
String columnName = columnNames.get(i - 1).toString();
Object value = rs.getObject(i);
RuntimeScalar val;
Expand Down Expand Up @@ -738,8 +780,12 @@ public static RuntimeList disconnect(RuntimeArray args, int ctx) {
}

/**
* Finishes a statement handle, closing the underlying JDBC PreparedStatement.
* This releases database locks (e.g., SQLite table locks) held by the statement.
* Finishes a statement handle: drops any open {@link ResultSet} but keeps the
* {@link PreparedStatement} usable for subsequent {@code execute()} calls.
* <p>
* Closing the JDBC {@code PreparedStatement} here breaks real DBI semantics and
* modules that recycle handles (DBIx::Simple statement cache, etc.). Perl's driver
* finish ends the active cursor, not the lifetime of the prepared statement.
*
* @param args RuntimeArray containing:
* [0] - Statement handle (sth)
Expand All @@ -749,23 +795,20 @@ public static RuntimeList disconnect(RuntimeArray args, int ctx) {
public static RuntimeList finish(RuntimeArray args, int ctx) {
RuntimeHash sth = args.get(0).hashDeref();

// Close the JDBC PreparedStatement to release locks
RuntimeScalar stmtScalar = sth.get("statement");
if (stmtScalar != null && stmtScalar.value instanceof PreparedStatement stmt) {
RuntimeScalar prevResultRef = sth.get("execute_result");
if (prevResultRef != null && RuntimeScalarType.isReference(prevResultRef)) {
try {
if (!stmt.isClosed()) {
stmt.close();
RuntimeHash prevResult = prevResultRef.hashDeref();
RuntimeScalar rsScalar = prevResult.get("resultset");
if (rsScalar != null && rsScalar.value instanceof ResultSet rs) {
if (!rs.isClosed()) {
rs.close();
}
}
} catch (Exception e) {
// Ignore close errors — statement may already be closed
} catch (Exception ignored) {
// Best effort — cursor may already be closed
}
}
// Also close any open ResultSet
RuntimeScalar rsScalar = sth.get("execute_result");
if (rsScalar != null && RuntimeScalarType.isReference(rsScalar)) {
Object rsObj = rsScalar.hashDeref();
// execute_result may be stored differently; check raw value
}

sth.put("Active", new RuntimeScalar(false));
return new RuntimeScalar(1).getList();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,55 @@ static String preProcessRegex(String s, RegexFlags regexFlags) {
StringBuilder sb = new StringBuilder();
handleRegex(s, 0, sb, regexFlags, false);
String result = sb.toString();
return result;
return preferOmniHolderLiteralAlternation(result);
}

/**
* Perl prefers the longest matching alternative when several branches succeed at the same
* offset; {@link Pattern} tries alternatives strictly left-to-right. DBIx::Simple's omniholder
* substitution uses {@code ($quoted|\(\?\?\))}: the nullable quoted repetition matches the empty
* string everywhere, so Java never reaches the literal {@code (??)} branch and SQL stays
* unchanged — JDBC then rejects {@code (??)} as invalid SQL.
* <p>
* Detect the usual Java-preprocessed shape {@code ( (?FLAGS:(?:'…'|"…")*)|\(\?\?\) )}
* (single outer capturing group) and swap alternatives so the omniholder literal is tried
* first.
*/
static String preferOmniHolderLiteralAlternation(String javaPattern) {
final String omniBranch = "\\(\\?\\?\\)";
final String needle = "|" + omniBranch;
if (javaPattern.length() < omniBranch.length() + 4
|| javaPattern.charAt(0) != '('
|| javaPattern.charAt(javaPattern.length() - 1) != ')') {
return javaPattern;
}
int pipeIdx = javaPattern.lastIndexOf(needle);
if (pipeIdx <= 1) {
return javaPattern;
}
int omniStart = pipeIdx + 1;
if (!javaPattern.startsWith(omniBranch, omniStart)) {
return javaPattern;
}
int closingParenIdx = omniStart + omniBranch.length();
if (closingParenIdx != javaPattern.length() - 1 || javaPattern.charAt(closingParenIdx) != ')') {
return javaPattern;
}
String leftAlt = javaPattern.substring(1, pipeIdx);
// Narrow heuristic so arbitrary ( A | \(??\) ) patterns are untouched.
if (!looksLikeQuotedStarAlternate(leftAlt)) {
return javaPattern;
}
return "(" + omniBranch + "|" + leftAlt + ")";
}

/** Left branch produced from Perl {@code (?:'[^']*'|"[^"]*")*} plus inline (?FLAGS: … ). */
private static boolean looksLikeQuotedStarAlternate(String leftAlt) {
if (!leftAlt.startsWith("(?")) {
return false;
}
// Typical preprocessing embeds both quote flavours inside a non-capturing cluster.
return leftAlt.contains("(?:'[^") && leftAlt.contains("|\"[^");
}

/**
Expand Down
Loading
Loading