[3.0] Misc PostgreSQL fixes

[Sesquipedalian]

Fixes #9519

[albertlast]

doesn't work since null is a state like true or false, so you had to use "is" operator and not =

[Sesquipedalian]

Derp. Of course you are correct.

[Sesquipedalian]

Once the = NULL has been corrected to IS NULL, do you see any other issues with this code?

[albertlast]

to dense logic for me, dunno what it all do

[Sesquipedalian]

$indexed_columns will be a list of columns that are included in any PRIMARY KEY or UNIQUE index. This list is then used to filter the list of columns that will be included in $key_str. That way, only columns that are part of a unique index will be included in the ON CONFLICT (...) statement.

This is my fix for the "Unapproving a post fails at runtime" issue in #9519, where ON CONFLICT (id_msg) DO NOTHING causes the error there is no unique or exclusion constraint matching the ON CONFLICT specification.

[albertlast]

I tested this against a real 2.1 → 3.0 upgrade on PostgreSQL 17, restoring the committed SMF 2.1.7 baseline from #9330 (403 members, 6 000 messages, 24 boards), and against a fresh PostgreSQL install. release-3.0 merged in for testing, since the branch is a little behind.

The substance of this is right and it fixes the things it set out to fix — but three faults stop it running at all on PostgreSQL, so none of that is reachable as it stands.

---

1. array_merge() is given named parameters

list_indexes($table, true) returns an array keyed by index name, and spreading a string-keyed array passes those keys as named arguments:

PHP Fatal error:  Uncaught ArgumentCountError: array_merge() does not accept unknown named parameters
    in /var/www/html/Sources/Db/APIs/PostgreSQL.php:436
Stack trace:
#0 /var/www/html/Sources/Db/APIs/PostgreSQL.php(436): array_merge(pkey: Array)
#1 /var/www/html/Sources/Config.php(1359): SMF\Db\APIs\PostgreSQL->insert('replace', 'smf_settings', Array, Array, Array)
#2 /var/www/html/Sources/Maintenance/Tools/ToolsBase.php(645): SMF\Config::updateModSettings(Array, false)
#3 /var/www/html/Sources/Maintenance/Tools/Install.php(826): SMF\Maintenance\Tools\Install->databasePopulation()

A fresh PostgreSQL install dies at the first updateModSettings(), during database population. Every insert() using replace or ignore is affected, so this reaches install, upgrade and normal running alike. array_values() around the array_map() result clears it.

2. AlertsObsolete does not import Db

The file imports only MigrationBase, so the new Db::$db->update_from() call resolves against the migration's own namespace:

+++ Updating obsolete alerts from before RC3... failed with error:
    "Class "SMF\Maintenance\Migration\v2_1\Db" not found"

Worth noting that neither phplint nor php-cs-fixer can catch this one — it only shows up when the migration actually runs.

3. update_from() returns the wrong type on PostgreSQL

+++ Updating obsolete alerts from before RC3... failed with error:
    "SMF\Db\APIs\PostgreSQL::update_from(): Return value must be of type bool, PgSql\Result returned"

The method is declared : bool and ends return $this->query(...), which on PostgreSQL is a PgSql\Result. This predates the pull request, but this is the first caller update_from() has ever had, so it surfaces here. MySQL's copy has the same shape and escapes only because mysqli_query() returns true for an UPDATE — the declaration is equally wrong there, just latent.

---

With those three patched locally, the rest works

The upgrade gets from "Updating obsolete alerts", where it used to stop, through to "Adding support for recurring events":

• PostgreSqlSchemaDiff runs its fixes for the first time. That migration has never done anything on any install, so this is the first time those ~30 statements have been applied.
• AlertsObsolete no longer uses MySQL-only syntax.
• change_column() respects a nullable column. messages.edit_history, which is declared with no not_null at all, now gets past where it used to fail.
• The backfill works wherever a default is available.
• Primary keys survive. 70 in the restored 2.1 baseline, 70 after the upgrade — it was 70 → 61 before, with smf_settings, smf_permissions and smf_board_permissions_view among the casualties. That was the data-loss half of [3.0]: PostgreSQL: the upgrader cannot finish, and unapproving a post errors #9519 and it is fixed.
• smf_approval_queue inserts stop erroring, since with no unique index at all it now emits no ON CONFLICT clause. (Its schema class defines three columns and no indexes, which is why nothing could ever have matched.)

Two things it does not finish

A NOT NULL column with no default is still stuck. The backfill is guarded by $default !== 'NULL', and smf_calendar.rdates and exdates are declared not_null: true, default: null — so there is nothing to backfill from and the upgrade stops:

ERROR:  column "rdates" of relation "smf_calendar" contains null values
STATEMENT:  ALTER TABLE smf_calendar
                ALTER COLUMN rdates SET NOT NULL

Arguably the column definitions are the thing at fault — NOT NULL with no default only means anything on MySQL, which invents one — but as it stands the upgrade cannot get past them on PostgreSQL.

ON CONFLICT can still name a set that matches no index. array_intersect($keys, $indexed_columns) can only shrink $keys; it can never complete them. So a key list that is a subset of a real index still produces a clause, and PostgreSQL requires the set to match a unique index exactly:

actual key:  PRIMARY KEY (id_group, id_board, deny)
generated:   ON CONFLICT (id_board,id_group) DO NOTHING

ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT specification
STATEMENT:  INSERT INTO smf_board_permissions_view("id_board", "id_group", "deny") VALUES (3, -1, 0), …

Three such statements in a single upgrade run, so board permission views are still going missing. The test this wants is "is there a unique or primary index whose columns are exactly the key set", emitting no clause when there is not — the intersection is close but lets the partial-overlap case through. MySQL never notices because INSERT IGNORE takes no column list.

---

For what it is worth, the primary-key work is the part I would most want to see land: silently dropping nine primary keys during an upgrade is a good deal worse than any of the loud failures around it.

[albertlast]

Retested at f22c999 with current release-3.0 merged in, against a real 2.1 → 3.0 upgrade of the committed 2.1.7 baseline from #9330 on PostgreSQL 17.

All four things I raised are fixed, and the effect is bigger than the sum of them.

• The array_merge() named-parameter fatal is gone — a plain foreach accumulating into $indexed_columns cannot hit it.
• AlertsObsolete imports Db now.
• update_from() returns a real boolean, and the MySQL copy got the same treatment rather than being left latent.
• calendar.rdates and exdates are default: '', so the backfill has something to work from and SET NOT NULL no longer trips over its own nulls.

The upgrade completes on PostgreSQL. As far as I can tell that is the first time it has. With the two DropTimeOffset defects from #9521 patched locally — unrelated to this branch, and they stop MySQL in exactly the same place — it runs to the end:

72 tables       fresh 72   upgraded 72
primary keys    fresh 69   upgraded 69
sequences       fresh 41   upgraded 41

Primary keys surviving is the part worth pausing on. That was the data-loss half of #9519, and it holds.

---

The migration this now reaches cannot run

The upgrade gets all the way to SearchResultsPrimaryKey and stops:

+++ Improving search results storage... ERROR:  constraint "pkey" of relation "smf_log_search_results" does not exist

That migration calls remove_index($table, 'primary'), which emits ALTER TABLE … DROP CONSTRAINT followed by whatever list_indexes() called the index. This is not a regression — I checked it both ways against the same database:

	list_indexes() calls it	remove_index(…, 'primary')
release-3.0	PRIMARY	fails — syntax error at or near "PRIMARY"
with this branch	pkey	fails — constraint "pkey" … does not exist

The real name is smf_log_search_results_pkey. It was broken before and it is broken now; the difference is that on release-3.0 the upgrade dies at AlertsObsolete long before reaching it, and this branch clears the way.

It is worth fixing here rather than elsewhere, because 37df0da is already aiming at exactly this — it drops the 'PRIMARY' special case so the true name survives. The trouble is the line that replaced it:

$row['name'] = str_replace($real_table_name . '_', '', $row['name']);

which strips smf_log_search_results_ from smf_log_search_results_pkey and leaves pkey. Skipping that for the primary key is enough:

if (empty($row['is_primary'])) {
    $row['name'] = str_replace($real_table_name . '_', '', $row['name']);
}

With that in, the same probe reports:

the constraint is really called   smf_log_search_results_pkey
list_indexes() calls it           smf_log_search_results_pkey
remove_index(.., "primary")       dropped it

and the upgrade runs to completion, which is where the numbers above come from.

ON CONFLICT still names a set no index matches

Unchanged from last time. array_intersect($keys, $indexed_columns) can only shrink $keys, never complete them, so a key list that is a subset of a real index still produces a clause PostgreSQL will not accept:

actual primary key:  (id_group, id_board, deny)
generated:           ON CONFLICT (id_board,id_group) DO NOTHING

ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT specification
STATEMENT:  INSERT INTO smf_board_permissions_view("id_board", "id_group", "deny") VALUES (24, 2, 0) …

Three of those per upgrade run. The union of every indexed column does contain deny, but $keys does not, so the intersection drops it again. The test this wants is "is there a unique or primary index whose columns are exactly this set", emitting nothing when there is not.

Two smaller things the comparison turned up

Neither is this branch's doing; noting them so they are not mistaken for it.

• 77 indexes that 3.0 does not define are never dropped — 182 on a fresh install against 259 after an upgrade. Same pattern MySQL shows.
• migrate_inet(val anyelement) is left behind. The upgrade creates it and never drops it, so an upgraded database carries a function a fresh one has not got — 20 against 19.

There is also some type drift worth a separate look some time: id_msg, id_attach and friends come out bigint after an upgrade where a fresh install makes them integer, and defaults render as '1'::smallint against 1. I have not chased whether that matters.

Everything else here looks right to me, and getting a PostgreSQL forum through the upgrader at all is a big step.

[albertlast]

I went and wrote the two remaining fixes so the suggestion is a concrete one rather than a description. Happy either way: fold them into this branch, or say the word and I will open a pull request against it.

Both are in PostgreSQL.php, 35 lines added and 9 removed, php-cs-fixer clean.

1. The primary key keeps the name the database gave it

remove_index() hands $index['name'] to DROP CONSTRAINT, so the strip has to skip the primary key:

// The primary key keeps the name the database gave it, because that
// is the name remove_index() has to hand to DROP CONSTRAINT.
// Stripping the table off smf_foo_pkey leaves 'pkey', which names
// nothing.
if (empty($row['is_primary'])) {
    $row['name'] = str_replace($real_table_name . '_', '', $row['name']);
}

the constraint is really called   smf_log_search_results_pkey
list_indexes() calls it           smf_log_search_results_pkey
remove_index(.., "primary")       dropped it

2. The conflict target comes from an index, not from $keys

The set has to match one index exactly, which $keys cannot be relied on to describe — the smf_board_permissions_view caller names two columns of a three column primary key. Choosing an index whose columns are all being supplied, preferring the primary key, describes something real every time:

$possibly_conflicting_columns = [];
$inserting = array_keys($columns);

foreach ($this->list_indexes($table, true) as $index) {
    if (!\in_array($index['type'], ['primary', 'unique'])) {
        continue;
    }

    if (array_diff($index['columns'], $inserting) !== []) {
        continue;
    }

    if ($possibly_conflicting_columns === [] || $index['type'] === 'primary') {
        $possibly_conflicting_columns = $index['columns'];
    }
}

Two consequences worth being explicit about. The EXCLUDED list is now built from the conflict target rather than from $keys, since a column that identifies the row is not one to overwrite. And a replace whose every column is part of the target has nothing left to set, which would emit DO UPDATE SET with an empty list, so that case falls through to DO NOTHING — the same outcome.

What they do

A 2.1 → 3.0 upgrade of the #9330 baseline on PostgreSQL 17, with #9521 patched locally as before:

                        before these        after
ON CONFLICT failures      3 per run         0
the upgrade              stops at           runs to the end
                         SearchResultsPrimaryKey

Not just quieter — I checked the behaviour rather than the absence of errors, against INSERT IGNORE and REPLACE semantics:

== ignore, on a three column primary key the caller under-describes ==
  a new row                       inserted
  the same row again              ignored, no duplicate
  differing only in deny          inserted, as a distinct row

== replace, the everyday path ==
  a setting written twice         holds the second value
  and only one row of it          yes

== a table with no unique index at all ==
  insert with no index to match   inserted, no error

The third case is the one that matters for correctness: deny is part of the primary key, so a row differing only in deny is genuinely a different row and has to be allowed in. Keying on $keys alone would have collapsed the two.

smf_approval_queue still has no unique index for a target to name, so no clause is emitted there and the insert goes in plainly, as it does now.

Schema after the upgrade is unchanged by these: 72 tables, 69 primary keys against 69 on a fresh install, 41 sequences.

[Sesquipedalian]

// The primary key keeps the name the database gave it, because that
// is the name remove_index() has to hand to DROP CONSTRAINT.
// Stripping the table off smf_foo_pkey leaves 'pkey', which names
// nothing.
if (empty($row['is_primary'])) {
    $row['name'] = str_replace($real_table_name . '_', '', $row['name']);
}

This suggestion would break other code that use the list returned from list_indexes(). I will fix the problem with remove_index() another way.

[albertlast]

Retested at 2872ca8 with current release-3.0 merged in, on both engines.

Both of the things I raised are fixed, and the PostgreSQL upgrade now completes on this branch alone — the only thing I patched locally is #9521, which is unrelated and stops MySQL in exactly the same place.

                        MySQL       PostgreSQL
upgrade                 completes   completes
tables                  72 / 72     72 / 72
primary keys            —           69 / 69
sequences               —           41 / 41
ON CONFLICT failures    —           0        (was 3 per run)
errors in the server log during the upgrade  none

Handling the primary key in remove_index() rather than in list_indexes() is the better call of the two — mapping the name at the point of use leaves list_indexes() reporting the same thing on both engines, which is what the rest of the schema code expects. Checked on both:

  a primary key to start with       yes
  remove_index(.., "primary")       reports success
  and it is really gone             yes
  add_index() puts it back          yes

And the conflict target now describes a real index, so smf_board_permissions_view goes in. I checked the behaviour rather than the absence of errors — a row differing only in deny is genuinely a different row under that primary key, and it is still treated as one:

  a new row is inserted                            ok
  the same row again is ignored                    ok
  a row differing only in deny is a distinct row   ok
  a setting written twice holds the second value   ok

---

Two things in that block are still worth a look. Both predate this branch, and neither blocks the upgrade — I only found them because this is the code being rewritten.

DO UPDATE SET with nothing to set

When $keys covers every column being inserted, the EXCLUDED list comes out empty and the statement ends mid-clause:

INSERT INTO smf_board_permissions_view("id_board", "id_group", "deny")
VALUES (98, 3, 0) ON CONFLICT (id_group,id_board,deny) DO UPDATE SET
ERROR:  syntax error at end of input

The EXCLUDED loop still tests against $keys while the conflict target now comes from the index, so the two can disagree. Falling through to DO NOTHING when the list ends up empty covers it — the outcome is the same either way, since there is nothing to update.

An operator class in a column name stops the index matching

list_indexes() reports a PostgreSQL index column with its opclass attached, so smf_scheduled_tasks_idx_task comes back as:

  index idx_task      unique (task varchar_pattern_ops)

array_intersect($index['columns'], $column_names) compares that against the real column name task, never matches, and the clause is left off. For ignore that is the difference between MySQL's INSERT IGNORE and an error:

ERROR:  duplicate key value violates unique constraint "smf_scheduled_tasks_idx_task"
STATEMENT:  INSERT INTO smf_scheduled_tasks("next_time", "time_offset", …)

Worth being careful reading that one: the row count afterwards is 1, which looks like the duplicate was ignored. It was not — the second insert failed, and only the PostgreSQL log says so. Nothing reaches smf_log_errors.

One unique index on a stock install carries an opclass, and it is that one; 13 indexes in total have one. v2_1\ScheduledTasks is the caller that meets it, and it survives today only because the mismatch means no clause is emitted and its insert has nothing to conflict with. Stripping the opclass — everything from the first space on — before comparing would close it.

---

Nothing else changed: MySQL is 55 schema differences either side of the upgrade as before, PostgreSQL 205, both the same lists as the previous run. The remaining PostgreSQL differences are the ones I mentioned last time and are not this branch's doing — 77 indexes that 3.0 no longer defines are never dropped, and migrate_inet() is left behind.

[Sesquipedalian]

Latest commits should address the remaining issues, @albertlast. When you get a chance, please confirm.

[albertlast]

Checked ebde515bb against release-3.0 at ba5b86e43. One of the two is fixed, the other is fixed in the wrong place and still fails.

To be clear about what this is: I have not re-run the full 2.1 → 3.0 upgrade for this round. I replayed the two code paths against the real catalog rows from a PostgreSQL 17 install and then ran the SQL they produce. That is enough to settle both questions, but the end-to-end run is still owed once the second one lands.

DO UPDATE SET with nothing to set — fixed

ae03034e5 moves the EXCLUDED loop onto $possibly_conflicting_columns instead of $keys, so the two can no longer disagree, and !empty($col_str) drops the empty case through to DO NOTHING. That covers the smf_board_permissions_view statement that ended mid-clause.

The operator class — not fixed

ebde515bb is titled "Removes opclass suffix from column names", but the regex is applied to $row['name'], the index name:

// We only want the basic column name.
$row['name'] = preg_replace(
	[
		'/^' . preg_quote($real_table_name . '_') . '/',
		'/ \w+_ops$/',
	],
	'',
	$row['name'],
);

An index name never carries an opclass. It is in the column list, which the loop just above still only trim()s. Straight from the catalog:

smf_members_idx_member_name  | CREATE INDEX ... USING btree (member_name varchar_pattern_ops)
smf_scheduled_tasks_idx_task | CREATE UNIQUE INDEX ... USING btree (task varchar_pattern_ops)

Replaying list_indexes() and the insert() target selection from this branch against those rows:

list_indexes() reports:
  pkey          primary  ["id_task"]
  idx_next_time index    ["next_time"]
  idx_disabled  index    ["disabled"]
  idx_task      unique   ["task varchar_pattern_ops"]

conflict target for insert('replace', 'smf_scheduled_tasks', ...):
  NONE -> no ON CONFLICT clause emitted

and the bare insert that leaves behind, run against the database:

ERROR:  duplicate key value violates unique constraint "smf_scheduled_tasks_idx_task"
DETAIL:  Key (task)=(birthdayemails) already exists.

Unchanged from what I reported on the 17th. Moving the strip onto the columns is enough:

foreach ($columns as $k => $v) {
	// PostgreSQL reports the operator class as part of the column in
	// pg_get_indexdef(), but callers want the bare column name.
	$columns[$k] = preg_replace('/\s+\S+_ops$/', '', trim($v));
}

$row['name'] = preg_replace('/^' . preg_quote($real_table_name . '_', '/') . '/', '', $row['name']);

preg_quote() is also missing its '/' delimiter argument. It cannot bite on a table name, but the form is wrong.

Worth saying that this reaches further than insert(). Table::fixIndexName() compares array_map(fn($col) => $col['name'], $index->columns) against $existing_index['columns'], so it can never match any of the 13 stock indexes that carry an opclass either.

---

Four smaller things, none of them blocking

The first two are this branch's, the last two are not — noting them so they are not mistaken for it.

The backfill guard disagrees with the one below it. f2a1ea1e8 changed the NOT NULL decision to !empty($column_info['not_null']), but the backfill two blocks above is still isset(...). Column::__construct() always assigns not_null, so isset() is true for an explicitly nullable column as well, and its existing NULLs would be overwritten with the default. Nothing in the schema is not_null: false with a non-null default, so nothing breaks today — but the two guards ought to say the same thing.

insert() now does a catalog query per upsert. list_indexes() runs on every replace and ignore. log_topics, log_boards and log_online all upsert on an ordinary page view, so that is several extra pg_class/pg_index queries per request on PostgreSQL where there were none. A static cache keyed by table, for the request, would cost nothing.

The new comment in MySQL::remove_index() is inaccurate. It says list_indexes() reports the primary key as 'primary' on MySQL; it reports 'PRIMARY', straight from Key_name. The code is fine, since the comparison is against the passed-in $index_name rather than the reported one. But the engines now disagree with each other where they both used to say PRIMARY, and SearchResultsPrimaryKey::isCandidate() reads $existing_structure['indexes']['primary'] ?? null, which is still null on both — so that migration keeps re-running the primary key rebuild on every upgrade. Pre-existing, and I would not ask this branch to carry it.

AlertsObsolete steps around MigrationBase::query(). Calling Db::$db->update_from() directly means this one statement does not get disableQueryCheck, db_error_skip or the processError() handling that every other statement in the migration has. The rewrite itself is right; it just loses the upgrade-time error path on the way.

---

The primary key work is still the part I most want to see land, and ae03034e5 closes the last of the ON CONFLICT failures I was seeing. Fix the opclass strip and I will put the full upgrade through on both engines again and post the numbers.

[Sesquipedalian]

The operator class — not fixed

ebde515bb is titled "Removes opclass suffix from column names", but the regex is applied to $row['name'], the index name:

That was silly of me. I'm not sure what I was thinking there. Fixed now.

The backfill guard disagrees with the one below it. f2a1ea1e8 changed the NOT NULL decision to !empty($column_info['not_null']), but the backfill two blocks above is still isset(...). Column::__construct() always assigns not_null, so isset() is true for an explicitly nullable column as well, and its existing NULLs would be overwritten with the default. Nothing in the schema is not_null: false with a non-null default, so nothing breaks today — but the two guards ought to say the same thing.

Good catch. Fixed now.

insert() now does a catalog query per upsert. list_indexes() runs on every replace and ignore. log_topics, log_boards and log_online all upsert on an ordinary page view, so that is several extra pg_class/pg_index queries per request on PostgreSQL where there were none. A static cache keyed by table, for the request, would cost nothing.

Good idea. Added in latest commits.

The new comment in MySQL::remove_index() is inaccurate. It says list_indexes() reports the primary key as 'primary' on MySQL; it reports 'PRIMARY', straight from Key_name. The code is fine, since the comparison is against the passed-in $index_name rather than the reported one. But the engines now disagree with each other where they both used to say PRIMARY, and SearchResultsPrimaryKey::isCandidate() reads $existing_structure['indexes']['primary'] ?? null, which is still null on both — so that migration keeps re-running the primary key rebuild on every upgrade. Pre-existing, and I would not ask this branch to carry it.

SearchResultsPrimaryKey::isCandidate() has been fixed to check the index type in order to find the primary key.

AlertsObsolete steps around MigrationBase::query(). Calling Db::$db->update_from() directly means this one statement does not get disableQueryCheck, db_error_skip or the processError() handling that every other statement in the migration has. The rewrite itself is right; it just loses the upgrade-time error path on the way.

Don't care.

[albertlast]

Checked ebe5fa3a3. The opclass fix and the backfill guard are right. The two new commits each introduce something, and the index cache is a regression I can show either side of.

Method, so it is clear what this is and is not: I loaded the branch's own classes against the dev PostgreSQL 17 container and drove them directly — no full upgrade run this round. Where I say "before" and "after" below, that is the same script against ebde515bb and ebe5fa3a3, same database, nothing else changed.

The opclass — fixed

preg_replace('/\s+\w+_ops$/', '', trim($v)) on the columns is the right place. The index it was aimed at now reports a column name the caller can match:

  pkey           primary  ["id_task"]
  idx_next_time  index    ["next_time"]
  idx_disabled   index    ["disabled"]
  idx_task       unique   ["task"]

and the ScheduledTasks upsert that used to die on it now behaves like REPLACE:

before: ["0","birthdayemails"]
insert('replace') completed
after:  ["999999","birthdayemails"]
rows with that task: 1

The backfill guard — fixed

!empty($column_info['not_null']) now, matching the block below it.

---

The index cache breaks Table::fixIndexName()

list_indexes() caches under $parsed_table_name alone, but the method returns two different shapes depending on $detail — a flat list of names, or an array keyed by name with type and columns. One key, two shapes, and whichever call lands first decides what everyone after it gets.

if (isset($this->index_cache[$parsed_table_name])) {
	return $this->index_cache[$parsed_table_name];
}

Asking for the flat list after the detailed one has been cached hands back the detailed one:

first call, detail = true:
array ( 'pkey' => 'pkey (primary)', 'idx_old' => 'idx_old (index)' )

second call, detail = false, same table:
array ( 'pkey' => array ( 'name' => 'pkey', 'type' => 'primary', 'columns' => ... ) ... )

rename_index() is the one caller inside the class that asks for the flat list, and it tests \in_array($old_name, $indexes). Against an array of arrays that never matches, so it returns false having done nothing — no error, nothing in the log.

Table::fixIndexName() reaches it directly after a detailed lookup, which is the ordinary schema-normalisation path: normalize() calls getCurrentStructure() first, and that is what fills the cache. A table whose index is under an older name, and a schema that wants it renamed:

=== A) ebde515bb, before the cache ===
index in the database before: smf_zz_pr9524_oldname
the schema wants it called:   idx_b
fixIndexName() returned: true
index in the database after:  smf_zz_pr9524_idx_b

=== B) ebe5fa3a3, with the cache ===
index in the database before: smf_zz_pr9524_oldname
the schema wants it called:   idx_b
fixIndexName() returned: false
index in the database after:  smf_zz_pr9524_oldname

normalize() then treats the false as "no matching index" and calls addIndex(), so the old index stays and a second one appears beside it under the new name.

The other order is louder. If the flat list is cached first — rename_index() on the same table earlier in the run — then the next caller expecting detail walks into it:

=== flat list cached first, then insert() asks for detail ===
TypeError: Cannot access offset of type string on string

=== and add_index(), which also expects detail ===
TypeError: Cannot access offset of type string on string

Keying on $parsed_table_name . '|' . (int) $detail covers it, or cache only the detailed form and derive the flat one from it on the way out.

Two invalidation gaps

create_table() and drop_table() do not invalidate. Every other schema-mutating method got an unset(); these two did not:

after create, list_indexes():                                  ["idx_first"]
after drop + create with a different index, list_indexes():    ["idx_first"]
what the database actually has:                                ["smf_zz_pr9524_idx_second"]

create_table() with if_exists = 'update' renames the old table aside and builds a new one, which is exactly this shape and is a path the upgrader takes.

rename_table() unsets a key that may not exist. It uses $full_old_name, built from $real_prefix — the prefix with the database name stripped — while list_indexes() caches under $parsed_table_name, built from $this->prefix. Where the prefix is database-qualified those are different strings and the unset misses. I have not set up a qualified prefix to demonstrate it; it is visible in the two lines.

---

SearchResultsPrimaryKey::isCandidate()

Three things, and the middle one is the reason the rewrite was asked for.

It still returns true after the migration has run. The new primary key is self::$columns by construction, so array_intersect($idx['columns'], self::$columns) cannot be empty. Against the real table, already migrated:

indexes reported: {"pkey":"pkey:primary:id_search,id_topic,id_msg"}
isCandidate() says: true

Checking the type instead of the name did fix the lookup, but the test around it wants to be "the existing primary key is not already the one we want" — $idx['columns'] !== self::$columns — rather than "it overlaps".

$idx is undefined when there are no indexes. table_structure() returns 'indexes' => [] for a table that is not there, and the foreach then leaves nothing behind for the return to read:

PHP warning: Undefined variable $idx
PHP warning: Trying to access array offset on null
returned: false

A table with no primary key is no longer a candidate. The old $idx == null || … returned true in that case; the new $idx['type'] === 'primary' && … returns false:

=== a table that has indexes but no primary key ===
returned: false  (the old code returned true here)

That is the case the migration exists for, given #9519 was losing primary keys. A foreach with the match inside it, rather than a break and a read afterwards, avoids both this and the undefined variable.

---

One cosmetic thing

In add_column(), the cache unset() was merged into the unset() that strips $column_info, which left its comment stranded:

unset($this->index_cache[$short_table_name],
	$column_info['type'],
	...
);

// If there's more attributes they need to be done via a change on PostgreSQL.


if (\count($column_info) != 1) {

The comment describes the unset() it is no longer next to, and there are two blank lines under it. php-cs-fixer does not mind, so it will not come back on CI.

---

Everything the last round was about is genuinely fixed, and the caching idea is worth keeping — it just needs $detail in the key and the two missing unset()s. I will put the full upgrade through on both engines once those are in.

[Sesquipedalian]

Done. Should be ready for testing now.

[albertlast]

Checked a707a0943. Everything I raised is fixed. Same method as last round — the branch's own classes driven directly against the dev PostgreSQL 17 container, not a full upgrade run.

The cache now answers for the shape it was asked for

Splitting the entry into detail and simple fixes both orders. Asking for the flat list after the detailed one:

first call, detail = true:   [ 'pkey' => 'pkey (primary)', 'idx_old' => 'idx_old (index)' ]
second call, detail = false: [ 0 => 'pkey', 1 => 'idx_old' ]

and rename_index(), which is the caller that asks for it, does its job again:

index names before: pkey, idx_old
rename_index('idx_old', 'idx_new') returned: true
what the database actually has: smf_zz_pr9524_idx_new, smf_zz_pr9524_pkey

The other order no longer hands a flat list to code expecting detail:

=== flat list cached first, then insert() asks for detail ===
insert('ignore') completed

=== and add_index(), which also expects detail ===
add_index() returned: true

Deriving simple from the detailed fetch with array_keys() is the part I most wanted to check, since a name collision after the table prefix is stripped would silently shorten the list. It does not happen anywhere here — I compared the derived form against a real detail = false call on every table:

72 tables checked
the derived simple form matches the direct one everywhere

And Table::fixIndexName(), which is what put me onto this, now behaves the same as it did before the cache existed:

index in the database before: smf_zz_pr9524_oldname
the schema wants it called:   idx_b
fixIndexName() returned: true
index in the database after:  smf_zz_pr9524_idx_b

The invalidation gaps are closed

create_table() and drop_table() both clear now:

after create, list_indexes():                                ["idx_first"]
after drop + create with a different index, list_indexes():  ["idx_second"]
what the database actually has:                              ["smf_zz_pr9524_idx_second"]

rename_table() unsets $short_old_name, which is built the same way as the key list_indexes() caches under. That one is by reading, not by test — I did not set up a database-qualified prefix.

isCandidate()

I ran the real method rather than a transcription of it, against smf_log_search_results reshaped four ways, each inside a transaction that was rolled back:

target primary key: (id_search, id_topic, id_msg)

already the target                     pkey:primary:(id_search, id_topic, id_msg)  isCandidate() false
the 2.1 primary key                    pkey:primary:(id_search, id_topic)          isCandidate() true
no primary key, other indexes present  idx_zz:index:(id_topic)                     isCandidate() true
no indexes at all                      (none)                                      isCandidate() true

All four right, and no warnings from the empty case now that !isset($idx) is tested first. The table is left with smf_log_search_results_pkey, as it started.

Still good from last round

The opclass strip and the ScheduledTasks upsert, re-checked against this head:

  idx_task  unique  ["task"]

before: ["0","birthdayemails"]
insert('replace') completed
after:  ["999999","birthdayemails"]
rows with that task: 1

---

One note, not a defect. In add_column() the cache unset() now sits below the early return $this->change_column(...), so it only runs for a column that carries nothing but a name. That is harmless twice over — change_column() clears the cache itself on the path that delegates, and adding a column does not change any index — but it reads like it covers a case it does not.

What I still owe is the thing I said I would do: the full 2.1 → 3.0 upgrade of the #9330 baseline on both engines. Nothing above replaces that, and I will post the numbers when I have them.

[albertlast]

Here is the full run I owed, at a707a0943: the committed SMF 2.1.7 baseline from #9330 (small — 403 members, 6 000 messages), restored and upgraded on both engines, in a stack of its own.

The only thing patched locally is #9521's DropTimeOffset — the two-line $row['offset'] / md5((string) $offset) pair. It is unrelated to this branch and stops MySQL and PostgreSQL in exactly the same place. Nothing else was touched.

                                        MySQL 8.4        PostgreSQL 17
upgrade                                 completes        completes
errors logged during the upgrade        0                0
ERROR lines in the PostgreSQL log       —                0

tables            fresh / upgraded      72 / 72          72 / 72
primary keys      fresh / upgraded      69 / 69          69 / 69
sequences         fresh / upgraded      —                41 / 41
indexes           fresh / upgraded      179 / 183        182 / 259

members / messages after                403 / 6 000      403 / 6 000
forum browses afterwards                yes              yes

The zero on the PostgreSQL log line is the one that needed proving, since the failures this branch is about never reach smf_log_errors. I checked the log was capable of recording one rather than trusting the silence — a deliberate SELECT 1/0 shows up in it, and across the whole run the only two ERROR: lines are that and another probe of my own.

What that means for the three things

ON CONFLICT no longer names a set no index matches. smf_board_permissions_view comes out with 70 rows against 3 on a fresh forum, which is what 24 boards should produce. That is the table that was losing three statements per run.

The opclass upsert works. smf_scheduled_tasks is 11 rows on both the upgraded and the fresh database.

Primary keys survive, 69 against 69 on both engines. That was the data-loss half of #9519 and it holds on a real upgrade, not just a probe.

One result worth reading carefully

SearchResultsPrimaryKey is skipped on both engines, and that is correct rather than a miss. The 2.1.7 baseline already carries the target key:

ADD CONSTRAINT "smf_log_search_results_pkey" PRIMARY KEY ("id_search", "id_topic", "id_msg");

so isCandidate() is right to say no. Before a707a0943 it said yes regardless — $existing_structure['indexes']['primary'] was never set on either engine — and execute() then ran into DROP CONSTRAINT and stopped the whole upgrade. So the migration that used to end the PostgreSQL run now correctly declines to run at all.

The flip side is that this baseline does not exercise the remove_index()/add_index() inside execute(). I checked that path separately against a live database rather than leave it unproven: remove_index(…, 'primary') reports success, the constraint is really gone, and add_index() puts it back.

MySQL specifics

The baseline is a genuine pre-3.0 forum rather than a clean one, and the engine-specific steps did their work: after the upgrade there are 0 non-InnoDB tables (it starts with two MyISAM ones) and 0 tables not in utf8mb4 (the database starts as utf8mb3).

What is left over, none of it this branch's

• Stale background task rows. Browsing either upgraded forum logs Invalid background task specified: class CreatePost_Notify_Background not found and the same for Update_TLD_Regex. That is [3.0]: Upgrading from 2.1 stops in DropTimeOffset, and TFA logins and old background tasks break afterwards #9521's third item; the rows are never removed, so it repeats.
• 77 indexes PostgreSQL keeps that 3.0 does not define — 259 against 182. Same figure as before, same cause: nothing drops them. MySQL shows a much smaller version of it, 183 against 179.
• A failed upgrade is not resumable. My first MySQL attempt died in DropTimeOffset; re-running after patching it then failed with Table 'smf.smf_calendar_holidays' doesn't exist, because HolidaysToEvents had already dropped it on the first pass. The fix is to restore and start again, which is what I did for both engines here. Worth knowing, since it means a partial run cannot simply be repeated.

---

Both engines through the upgrader, both forums browsing afterwards, and no errors either engine kept quiet about. This looks right to me.