Skip to content

[OPENJPA-2963] Track dropped tables per database, not per JVM - #179

Merged
cristof merged 2 commits into
masterfrom
OPENJPA-2963
Sep 4, 2026
Merged

[OPENJPA-2963] Track dropped tables per database, not per JVM#179
cristof merged 2 commits into
masterfrom
OPENJPA-2963

Conversation

@rzo1

@rzo1 rzo1 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

The dropped-table tracking was one JVM-global set, and clearDroppedTables() is called whenever any factory with schema-generation properties starts up, so two persistence units initializing at once wiped each other's in-flight tracking. Keys were bare table names too, so two units on different databases with a table of the same name aliased onto one entry.

The set is now partitioned by the database a configuration connects to, and the clear empties only that database's entry. Every access holds one monitor, including the trace line, which previously interpolated the live set while another thread could be writing to it. The no-arg clearDroppedTables() is kept and deprecated.

On the suggestion to move the state onto the configuration: that is not available. The tracking has to outlive the configuration that wrote it — Persistence.generateSchema() closes its own factory, and a later, separately created factory is expected to see what it dropped, which TestSchemaGenDrop.testDropViaGenerateSchema pins. Config-scoped, the set would have almost no readers and that test would fail. So the state stays static and is scoped instead.

On the locale half of the issue: it no longer applies. 53269ab already gave every one of those call sites Locale.ROOT; the line numbers in the issue are from before it.

Deliberately not addressed here, each worth its own issue:

  • The DDL name matching still compares a regex-stripped fragment of the raw statement against a schema-qualified identifier, so it both misses (DROP TABLE IF EXISTS FOO yields an empty key; DROP TABLE ORDER_CASCADE_LOG yields ORDER_) and over-matches. Fixing it in isolation would make things worse rather than better: more names would match, so any suppression defect would fire in more configurations than it does today.
  • It was suggested that a drop-and-create with a script drop source suppresses the create it is paired with, because both run in one action list against one configuration. I wrote the fix and a test for it, and the test passed against unpatched master — with an explicit drop-source=script and with a verifying factory that has no SynchronizeMappings and so cannot create the table itself. I could not reproduce it, so the fix is not included.

There is no new test. The concurrency defect is not deterministically testable, and the one testable claim did not reproduce; the change rests on the existing schema-generation suite (35 tests, passing).

The dropped-table tracking was one JVM-global set, and clearDroppedTables()
is called whenever any factory with schema generation properties starts up,
so two persistence units initializing at once wiped each other's in-flight
tracking. Keys were bare table names too, so two units on different
databases with a table of the same name aliased onto one entry.

The set is now partitioned by the database a configuration connects to, and
the clear only empties that database's entry. The state stays static: it has
to outlive the configuration that wrote it, because generateSchema() closes
its own factory and a later factory is expected to see what it dropped,
which TestSchemaGenDrop pins. Moving it onto the configuration, as the issue
suggests, would leave it with no readers and break that test. Every access
now holds one monitor, including the trace line, which previously
interpolated the live set while another thread could be writing to it.

The locale half of the issue no longer applies: 53269ab already gave every
one of those call sites Locale.ROOT, and the line numbers in the issue are
from before it.

Not addressed here, and worth their own issues: the DDL name matching still
compares a regex-stripped fragment of the raw statement against a
schema-qualified identifier, so it misses and over-matches in several
shapes; and it is claimed that a drop-and-create with a script drop source
suppresses the create it is paired with, which I could not reproduce.
@rzo1 rzo1 self-assigned this Sep 2, 2026
@rzo1
rzo1 requested review from cristof and solomax September 2, 2026 19:31
@cristof

cristof commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Could you provide an unit test for the issue? It would help to avoid future regressions.

Review feedback. The connection factory name is not a reliable identity for
a database, as the review points out. The tracking is now keyed on the
configuration id, which for a persistence unit is its name, since
PersistenceUnitInfoImpl defaults openjpa.Id to it. Two factories for one
unit therefore still share their tracking, which is what lets a factory see
what a closed generateSchema() factory dropped, while two units cannot
clear or consume one another's entries. A configuration with no id falls
back to the connection it names.

The producer and the consumer move into two package visible methods, which
also gives the behaviour a seam to test through without a database.
TestSchemaToolDroppedTables covers the partitioning, the sharing within one
unit, the scoped clear and the consumption; the two partitioning cases fail
if the map is collapsed back to a single set.
@rzo1

rzo1 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Added TestSchemaToolDroppedTables (openjpa-jdbc, no database needed). The producer and consumer moved into two package-visible methods so the behaviour has a seam to test through, and it covers:

  • one persistence unit does not see another's drop, and clearing one leaves the other intact — the actual defect;
  • two factories of the same unit do share the tracking, so the generateSchema()-then-buildSchema flow keeps working;
  • a create forgets the entry, and outside spec-compliant mode the entry suppresses one create only;
  • the key falls back to the connection when a configuration has no id.

The two partitioning cases fail if the map is collapsed back to a single global set, which I checked rather than assumed.

Note the concurrency itself is still not covered: two persistence units initialising at the same time is not deterministically reproducible in a unit test, so the tests pin the partitioning that makes the interleaving harmless rather than the interleaving.

@cristof
cristof merged commit 97c6fee into master Sep 4, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants