iremcaginyurtturk commented on PR #1635:
URL: https://github.com/apache/iceberg-go/pull/1635#issuecomment-5237008631
Thanks for the detailed pass. Pushed the fixes:
- **Error shape:** the race-loser now returns the same `fmt.Errorf("%w: %s",
…)` as the serial pre-check, so the two paths are byte-identical and
`errors.As` no longer unwraps to the driver error only on the race path.
- **Dropped causes:** the rollback-failure branch now `errors.Join(err,
rbErr)` and the re-check-failure branch `errors.Join(err, checkErr)`, so a
poisoned tx / failed re-check isn't reported as a plain insert error.
- **mssql:** confirmed bun emits `SAVE TRANSACTION` for the MSSavepoint
feature, so the savepoint path holds across the declared dialects.
- **Emulation robustness:** added a comment naming the bun SQL shape the
matchers depend on.
On the `boolRows` EOF suggestion — I went a different way, and wanted to
flag why: bun's `Exists()` runs `SELECT EXISTS(...)` and scans the value via
`QueryRowContext(...).Scan(&bool)`, so an empty result comes back as
`sql.ErrNoRows` rather than "false" and the probe errors out (I hit this — the
tests fail). So I kept `boolRows` emitting an explicit `0`/`1` row and
documented that it must, rather than signalling EOF. I did confirm the pinning
holds: reverting the savepoint makes `TestCreateNamespaceLosesInsertRace` fail,
and the negative case would fail if the re-check ever reported "exists" for the
unrelated-failure path — so both are genuinely pinned. Happy to revisit if
you'd prefer a different shape there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
