mariiaKraievska commented on PR #6327: URL: https://github.com/apache/fineract/pull/6327#issuecomment-5491676336
> The changelog itself is well-formed - separate part file, backfill before the constraint, `MARK_RAN` preconditions, matching column data types. My concern is with deploy sequencing rather than the SQL. > > `#6299` (which added the nullable `submitted_on_date` column) is itself unreleased. If both PRs ship in the same release, then during a rolling upgrade the "old" instances are running code that never writes `submitted_on_date` at all - it doesn't exist in their entity mapping. Every insert from those instances hits the NOT NULL constraint the moment this migration has run on the new schema, and the whole write path 500s until the rollout completes. The re-backfill changeset here only protects against N-1 writers that _do_ know about the column but raced the earlier backfill - it can't protect against N-1 writers that don't know about the column at all. > > On top of that, the backfill and the `addNotNullConstraint` are two separate changesets/transactions, so there's a window between them where a concurrent write can still insert NULL and abort the whole migration (rather than just leaving a bad row) once the constraint changeset runs. > > Two ways to fix this: > > 1. If #6299 and this PR are meant to ship together, add a `DEFAULT` value (e.g. `CURRENT_DATE`) before enforcing NOT NULL, so N-1 writers that omit the column entirely still get a valid value rather than an error. And merge the backfill + constraint into one changeset per table so the whole thing is atomic. > 2. If they're meant to ship in separate releases, this PR should wait for the next one, and the comment on the re-backfill changeset should be corrected to explain it's belt-and-braces (for the narrower "column exists but wasn't written" case) rather than implying it closes the full rolling-deploy window. > > The in-repo precedent for exactly this situation (`parts/0016_configurable_attributes_not_null.xml`) adds a default value for this reason and explicitly says so in its comment - worth following that pattern here. > > Recommendation: CHANGES_REQUESTED @galovics Thank you for your review. 0074 now does DEFAULT → backfill → NOT NULL in one changeset per table; verified on MariaDB and PostgreSQL that N-1 omit-column inserts succeed. Please take another look -- 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]
