[3.0] Replace count_posts (where 0 = true) with posts_count (where 1 = true)

[Sesquipedalian]

Fixes #9415
Fixes #9518

As explained here, the count_posts column of the boards table uses 0 for true and 1 for false, and it has been doing this since way back in SMF 1.0.

This is insane.

Code that was newly written for SMF 3.0 has been built on the reasonable assumption that 1 means true and 0 means false, whereas old code that was ported over from SMF 2.1 without significant changes still assumes that count_posts uses 0 for true and 1 for false. This inconsistency is what ultimately caused #9415, because it created inconsistent handling of member's post counts.

Rather than patching the new code in order to perpetuate the madness, I have instead decided to fix all the old code in order to end this lunacy once and for all.

In order to avoid confusion in the mind of any developer looking at this code in the future, I have decided to completely replace the old count_posts column with a new posts_count column that uses sane logic. This change means that all existing installs of SMF 3.0 will need be upgraded by running the upgrader after this PR has been merged.

In order to maintain backward compatibility with any old mods that expected the old count_posts column and its inverted logic, I have also added some code to the database API classes that transparently replaces any references to the old count_posts column in query strings with references to the new posts_count column.

[albertlast]

I have tested this on both engines, and the diagnosis and the fix are right.

I reproduced #9415 on release-3.0 first, so I had something to compare against. It is exactly the inversion you describe — in a board that counts posts, starting a topic and replying both add nothing, deleting the reply is a 500 with id_post_group_1, and in a board that does not count posts the count goes up instead:

== a board that counts posts ==                          release-3.0
  FAIL  starting a topic added one post   -- was 0, now 0
  FAIL  replying added one post           -- was 0, now 0
  FAIL  deleting the reply did not error  -- 500 …action=deletemsg…
  FAIL  nothing was logged -- The database value you're trying to insert
                              does not exist: id_post_group_1
== a board that does not count posts ==
  FAIL  starting a topic added nothing    -- was 0, now 1

On this branch the same driver is 16/16 on MySQL and on PostgreSQL, both from a fresh install and after the upgrader.

I then swept the areas around it over real HTTP rather than trusting the grep: the admin board form round trip, creating a board, the board report, movetopic2 in both directions, quick-moderation moves, recounting member posts, approving and unapproving with post moderation on, recycle and restore, and the profile stats panel. 44 of 46 checks pass on each engine, and both failures are pre-existing — they behave the same on release-3.0 — so they are filed separately as #9518 and #9520.

On whether you got all of them: yes, for SMF's own code. Nothing outside the deliberate compatibility code and Db/Schema/v2_1/ still reads the old column, and the 'count_posts' => '!posts_count' alias returns the inverted value correctly both as a property and as an array key. The MySQL migration is right too — I ran it and the values inverted as intended.

Four things I do not think hold up, though.

---

1. backcompatFixes() rewrites comparisons into invalid SQL

$new_col is never imported into the closure (Sources/Db/APIs/MySQL.php:2966, and the same line at PostgreSQL.php:2840):

'/\b' . $old_col . '\s*(!=|<(?:=|>)?|=|>=?)\s*([01])\b/' => function ($m) {   // no use ($new_col)
    ...
    return $new_col . ' ' . $m[1] . ' ' . ((int) !$m[2]);
},

So with $backward_compatibility = 1, every comparison an old mod writes comes back as a syntax error plus a warning:

in:  SELECT id_board FROM {db_prefix}boards WHERE count_posts = 0
out: SELECT id_board FROM smf_boards WHERE = 1
     Warning: Undefined variable $new_col in .../MySQL.php on line 2966

The second pattern only works because it is an arrow function, which captures by value automatically.

2. <= is not inverted

The match has '>=' => '<=' twice and no '<=' arm, so the one case that needs flipping most obviously falls through to default:

count_posts <= 0   ->   posts_count <= 1      (always true; should be >= 1)

The other four operators are correct.

3. The shim never fires at all under SSI on MySQL

MySQL::initiate() rewrites the prefix to `smf`.smf_ when SMF == 'SSI', and the pattern's leading \b cannot match at a backtick preceded by a space, so preg_match() fails and nothing is rewritten:

Db::$db->prefix under SSI: '`smf`.smf_'
  FAIL  count_posts is still rewritten
        got: SELECT id_board FROM `smf`.smf_boards WHERE count_posts = 0

4. Db::$db->insert() is not covered

insert() calls quote() on the VALUES fragment only, which has no table name in it for the pattern to find, and then passes the assembled statement to query() with security_override, which is exactly the condition that skips quote(). So an old mod inserting a board gets:

Database Error: Unknown column 'count_posts' in 'field list'

---

Two smaller notes on the same shim: a rewritten SELECT list loses the old name entirely (SELECT count_posts becomes SELECT posts_count with no AS count_posts), so $row['count_posts'] is simply gone for the mod reading it — and if it were aliased it would still be carrying the flipped value. And an unqualified column beside an aliased table (SELECT id_board FROM {db_prefix}boards AS b WHERE count_posts = 0, which is legal SQL) is left alone, because $old_col has become b.count_posts.

---

5. The migration cannot run on PostgreSQL

addColumn() there emits a bare ADD COLUMN, then leaves not_null and default to change_column() — and in PostgreSQL SET DEFAULT does not backfill existing rows, so the constraint arrives while every row is still NULL:

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

It leaves the table with both columns and a NULL posts_count, and it is not re-runnable — a second attempt fails at the same statement, so the forum is stuck half-migrated and unusable, since the new code reads a column that is NULL for every board. Doing the UPDATE before the column is made NOT NULL would sidestep it. The root cause is in the schema layer rather than in your migration (the recurring-events migration dies the same way), so I have filed it as #9519 — but as it stands this PR cannot be upgraded onto on PostgreSQL. Once I finished the migration by hand, everything else on PostgreSQL was green.

---

Two last things, both minor: in Board::modify() the round trip recomputes $params['posts_count'] from count_posts unconditionally, so a modern mod that sets posts_count in integrate_modify_board has its change silently discarded; and the matching round trip in Msg::remove() is dead code, because that hook takes $row by value.

php-cs-fixer is clean on all 17 files.

[albertlast]

Retested at a3a0fa2, on both engines, and also against a real 2.1 forum this time — the committed SMF 2.1.7 baseline from #9330, 403 members and 6 000 messages across 24 boards, restored and then upgraded.

Four of the five things I raised are fixed. There is one new problem that stops the branch running at all, and one regression in the alias path.

---

The branch does not boot: backcompatInsertFixes() returns the wrong type

It is declared : string and returns an array on both paths, including the early one (MySQL.php:2992, PostgreSQL.php:2866). Since insert() calls it unconditionally, every insert throws — with $backward_compatibility = 0 just as much as with it on:

front page: 500

SMF\Db\APIs\MySQL::backcompatInsertFixes(): Return value must be of type string,
array returned in /var/www/html/Sources/Db/APIs/MySQL.php:2995
  #0 backcompatInsertFixes('smf_background_...', Array, Array, Array)

Logging in is enough to hit it. : array is all it wants.

The insert fix can never match its table

if ($table === $this->prefix . '_boards') {

$table has already had {db_prefix} replaced at that point, so this compares smf_boards against smf__boards. With the underscore removed and the return type corrected, Db::insert() with count_posts works and stores the inverted value, which is what it was for.

Aliased comparisons are now rewritten into nonsense

Making the alias prefix optional turned it into a capturing group:

$old_col = (!empty($matches[1]) ? '(' . $matches[1] . '\.)?' : '') . 'count_posts';

so in the aliased case $m[1] is the alias, the operator has moved to $m[2] and the value to $m[3] — while the callback still reads $m[1] as the operator and $m[2] as the value:

b.count_posts  = 0   ->   b.posts_count b. 0
b.count_posts != 0   ->   b.posts_count b. 0
b.count_posts  > 0   ->   b.posts_count b. 0
b.count_posts <= 0   ->   b.posts_count b. 0
   count_posts  = 0   ->   b.posts_count 0        (operator dropped)

Every operator, and the unqualified form beside an aliased table as well. The unaliased case still works, because then no group is built and the numbering is unchanged — which is why the first batch of checks passes. Aliasing the boards table as b is what SMF's own queries have always done, so it is the form old mods copied. (?:…)? restores the numbering.

The same optional group has a side effect worth a thought: a mod's own table is now caught whenever a boards table appears anywhere in the query, since the alias is optional but $new_col is not.

SELECT m.count_posts FROM {db_prefix}my_mod_table AS m
    INNER JOIN {db_prefix}boards AS b ON (b.id_board = m.id_board)

->  SELECT m.b.posts_count FROM smf_my_mod_table AS m INNER JOIN smf_boards AS b ...

---

What is fixed

The closure captures $new_col, the <= arm is right, and moving to Config::$db_prefix makes the pattern match under SSI, where the prefix is database-qualified. All eight operators come out correct in the unaliased form, through a literal and through {int:…} alike, and a mod's own count_posts in a query with no boards table is still left alone.

With the two blockers patched locally

• MySQL and PostgreSQL, fresh installs: 16/16 on the [3.0] Deleting a reply returns HTTP 500: id_post_group_1 is missing from the query parameters #9415 reproduction, 44/46 across the admin board form, board creation, the board report, movetopic2 both ways, quick-moderation moves, recounting member posts, approve/unapprove under post moderation, recycle and restore, and the profile stats panel. The two failures are the pre-existing ones in [3.0]: Member post counts drift in four places outside the create and delete path #9518 and [3.0]: Two fatals: the board index with unapproved posts, and the CLI installer #9520.
• The migration is right on real 2.1 data. Restoring the 2.1.7 baseline, setting boards 2 and 5 to count_posts = 1 and upgrading gives posts_count = 0 for exactly those two and 1 for the other 22, NOT NULL DEFAULT 1, with messages and per-member post counts untouched (6 000 and 5 999, identical member by member). The drivers then pass on the upgraded forum too.
• The same holds on PostgreSQL once the schema layer is fixed — isCandidate() goes false afterwards and the values invert correctly.

One thing to know about that last point: on PostgreSQL the upgrader cannot currently reach this migration at all. PostgreSQL::change_column() applies SET NOT NULL whenever the key is set even when its value is false, and SET DEFAULT does not backfill the rows that are already there, so posts_count fails its own constraint and the table is left holding both columns with the new one null. Details and the other blockers on that path are in #9519; the engine-independent ones are #9521. None of that is yours to fix here, but it does mean this migration cannot land on PostgreSQL until the schema layer is sorted.

[Sesquipedalian]

The latest commits resolve the flagged issues apart from the migration not being able to run on PostgreSQL. They also add fixes for #9518.

[albertlast]

Retested at 6a8a22a. All three blockers are gone, and the three post-count fixes on top hold up under testing. Details below, then three things that are still open.

I tested this branch with #9522 cherry-picked on top, since that was the last remaining failure in my sweep.

MySQL, fresh install	16/16 and 46/46
MySQL, fresh install, $backward_compatibility = 1	46/46
PostgreSQL, fresh install	16/16 and 46/46
MySQL, upgraded from the 2.1.7 baseline in #9330 (403 members, 6 000 messages, 24 boards)	16/16 and 46/46
The backward compatibility shim	23/25
php-cs-fixer	clean on all 19 files

The three fixes on top

I checked these rather than taking the commit messages for it, and all three are genuinely fixed rather than moved:

Quick moderation now decrements. Separating the accumulator from the User objects is the right call — that reuse of $members for two different things is what made the max(0, …) look reasonable.

The TopicMove2 stub. My earlier driver had postRedirect switched off, so it never covered the case your fix is actually about. Testing it directly, 7/7 in both directions:

== moving out of a board that counts posts ==
  ok    a stub was left behind
  ok    the stub is in the board the topic came from
  ok    the moved post stops counting and the stub starts        (net 0)

== moving into a board that counts posts ==
  ok    a stub was left in the quiet board
  ok    the moved post starts counting and the stub stays quiet  (net +1)

Reading Board::$info->posts_count gets both right; deciding from the target board got both wrong, in opposite directions.

The recount, at a size worth the name: after a full paginated run over the baseline's 403 members (two continuation rounds), not one member disagrees with the messages that ought to count. The m.approved = 1 filter closed the gap I saw before, where the recount produced a higher number than posting and approving ever could.

And the migration still lands correctly on real 2.1 data: the two boards I set to count_posts = 1 come out as posts_count = 0, the other 22 as 1, with messages and per-member counts untouched.

---

Still open

1. The PostgreSQL temp table in recountPosts() still fails.

The column type is fixed, but the part PostgreSQL rejects is the shape rather than the types — there is no CREATE TABLE (columns) SELECT in PostgreSQL:

ERROR:  syntax error at or near "SELECT" at character 129
STATEMENT:  CREATE TEMPORARY TABLE smf_tmp_maint_recountposts (
                    id_member int NOT NULL default '0',
                    PRIMARY KEY (id_member)
                )
                SELECT m.id_member
                FROM smf_messages AS m
                    INNER JOIN smf_boards AS b ON m.id_board = b.id_board
                WHERE m.id_member != 0

CREATE TEMPORARY TABLE … AS SELECT …, with no column list, is the portable form; I confirmed that one runs against the same database. Since the query still carries db_error_skip, this fails quietly, $createTemporary stays false, and the "members with a post count but no posts left" cleanup goes on being skipped on PostgreSQL.

2. A mod's own column is now caught when a boards table is in the same query.

The optional alias group matches the empty string, so the pattern also fires inside a different qualifier, while $new_col always carries the boards alias:

SELECT m.count_posts FROM {db_prefix}my_mod_table AS m
    INNER JOIN {db_prefix}boards AS b ON (b.id_board = m.id_board)

->  SELECT m.b.posts_count FROM smf_my_mod_table AS m INNER JOIN smf_boards AS b ...

Replacing the leading \b with (?<![\w.]) covers it: b.count_posts and a bare count_posts still match, one that already has a different qualifier does not.

3. A rewritten SELECT list still loses the old name.

SELECT count_posts FROM {db_prefix}boards comes back as posts_count, so $row['count_posts'] is not there for the mod that asked for it — and an alias on its own would hand back the inverted value, so it would need something like (1 - posts_count) AS count_posts to be honest. Quite possibly not worth it, given how much of that shim is already best-effort; I mention it only so it is a decision rather than an oversight.

Everything else I raised is fixed. #9522 is right too — unapproved_topics and unapproved_posts are exactly the two placeholders $txt['unapproved_posts'] uses.

For completeness on how the upgrade run was done: the 2.1 → 3.0 upgrade needs the two DropTimeOffset defects in #9521 patched before it will reach the end, and on PostgreSQL it is still blocked earlier by #9519, so that column of the table above is a fresh install rather than an upgrade.

[Sesquipedalian]

The three remaining issues, "The PostgreSQL temp table in recountPosts() still fails," "A mod's own column is now caught when a boards table is in the same query," and "A rewritten SELECT list still loses the old name" should now all be fixed.

[albertlast]

Retested at c612c99, with release-3.0 merged in (7a934c8), on a Docker environment rebuilt from scratch and a fresh install on each engine.

Everything I raised is now fixed. The (?<![\w.]) lookbehind closes the case where a mod's own table was caught, and the new SELECT handling is a better answer than I expected — I had assumed the inversion made it not worth doing, and shielding the alias behind an md5 token so the later patterns leave it alone is neat.

	MySQL	PostgreSQL
The #9415 reproduction	16/16	16/16
Wide sweep (admin form, board creation, report, movetopic2, quick moderation, recount, approve/unapprove, recycle and restore, stats panel)	46/46	46/46
Redirect stub, both directions	7/7	7/7
Backcompat shim, $backward_compatibility = 1	31/31	31/31
Wide sweep with backward compatibility on	46/46	—
php-cs-fixer	clean on all 18 files

The shim checks now go past the rewritten string to the rows themselves: an old mod's SELECT id_board, count_posts FROM {db_prefix}boards comes back with a real count_posts key carrying the old inverted meaning, for every board, on both engines.

I also pushed on that new SELECT pattern deliberately, since (?:.(?!\bFROM\b))* is the sort of construct that tends to come apart on real queries. It holds on every shape I tried — a subquery in the WHERE (only the inner comparison is touched), ORDER BY count_posts after the FROM (renamed, not expanded), GROUP BY, a UNION (both branches expanded), the boards table joined second, SMF's own multi-line formatting, and a mod's own table left alone when no boards table is present. Every rewritten query was then accepted by the engine.

---

One thing still outstanding

The PostgreSQL temp table in recountPosts(). ) became ) AS, which moves the error rather than clearing it:

ERROR:  syntax error at or near "AS" at character 126
STATEMENT:  CREATE TEMPORARY TABLE smf_tmp_maint_recountposts (
                    id_member int NOT NULL default '0',
                    PRIMARY KEY (id_member)
                ) AS
                SELECT m.id_member

PostgreSQL's CREATE TABLE … AS accepts a bare column name list only — no types, no constraints. Either of these runs against the same database:

CREATE TEMPORARY TABLE t (id_member) AS SELECT …
CREATE TEMPORARY TABLE t AS SELECT … ;  ALTER TABLE t ADD PRIMARY KEY (id_member);

The consequence is real rather than cosmetic. Setting every board to not count posts and leaving a member holding posts = 42, then running a full recount:

MySQL        42 -> 0     the cleanup step runs
PostgreSQL   42 -> 42    the cleanup step is skipped

db_error_skip is what keeps it quiet — nothing reaches smf_log_errors, and only the PostgreSQL server log shows it. So on PostgreSQL the recount goes on doing three quarters of its job: it corrects the members who still have qualifying posts, and never clears the ones who have none.

That is item 4 of #9518, which this pull request is set to close.

Everything else about the branch looks right to me. With #9524 in flight for the schema layer, the upgrade path this migration needs on PostgreSQL should open up too — I will retest that against #9524 separately.

[albertlast]

Retested at d3ad5df with current release-3.0 merged in, on fresh installs of both engines.

The one thing still outstanding is fixed. The recount cleanup step now runs on PostgreSQL:

                 before        after
MySQL         42 posts  ->   0 posts
PostgreSQL    42 posts  ->   0 posts      (was 42 -> 42)

Verified twice over: once by replaying the step exactly as recountPosts() performs it, and once by driving the real maintenance action over HTTP with every board set not to count posts and a member left holding 42. The PostgreSQL server log is clean through both — no db_error_skip swallowing anything this time.

CREATE TEMPORARY TABLE … AS SELECT with the key added afterwards is the right answer, and it is worth saying that add_index() on a temporary table is less obviously safe than it looks. list_columns() filters on table_schema = 'public', and a temporary table lives in pg_temp_N, so that lookup cannot see it. It works out because the return value is never used, and because the ALTER TABLE … ADD PRIMARY KEY that follows is unqualified and resolves through search_path, where pg_temp comes first. I checked that a primary key really lands on the table rather than the call merely returning true, and it does, on both engines.

(While looking: $cols = $this->list_columns($table_name, true); at PostgreSQL.php:1385 is assigned and never read — the loop below rewrites $index_info['columns'] instead. Dead as far as I can tell, and not something this pull request introduced.)

The Msg.php simplification is right. The block that used to convert count_posts back after the hook was doing a round trip that could not carry anything: integrate_pre_remove_message receives [$message, $decreasePostCount, $row] by value, so a mod cannot write to $row through it. Reading $row['posts_count'] directly at line 2245, instead of re-deriving it from the compatibility key, is also strictly better — the old form collapsed anything that was not 0 or 1.

Everything else still passes

	MySQL	PostgreSQL
The #9415 reproduction	16/16	16/16
Wide sweep (admin form, board creation, report, movetopic2, quick moderation, recount, approve/unapprove, recycle and restore, stats panel)	46/46	46/46
Redirect stub, both directions	7/7	7/7
Backcompat shim, $backward_compatibility = 1	31/31	31/31
Rewritten SELECT shapes accepted by the engine	10/10	10/10
php-cs-fixer	clean on all 18 files

The wide sweep was run with the shim on as well as off on both engines this time.

One suggestion, not a defect

The temporary table has no IF NOT EXISTS and is never dropped, so a second recountPosts() on the same database connection cannot create it:

first  CREATE  ok
second CREATE  failed

Both engines. db_error_skip is set on that query, so $createTemporary is simply false and the whole cleanup step is skipped without a word — the same silent-skip this pull request just fixed, reachable by a different route. It needs $db_persist = true for a connection to outlive the request, so most installs will never see it, and the shape predates this branch. Still, a DROP TABLE IF EXISTS in front of the CREATE would close it, and this is the commit that is already rewriting those lines.

Not this pull request

One error appears in the PostgreSQL log during the sweep:

ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT specification
STATEMENT:  INSERT INTO smf_approval_queue("id_msg")

smf_approval_queue has no unique index for a conflict target to match. That is pre-existing and #9524 fixes it; I mention it only so it is not read as fallout from here.

Looks good to me.

[albertlast]

Following up on the temporary table point from my last comment, because "add IF NOT EXISTS" is the obvious fix and it is the wrong one. I tested the options on MySQL 8.4, MariaDB 11.8 and PostgreSQL 17.

The problem, first. recountPosts() creates the table with no IF NOT EXISTS and never drops it. Run it twice on one connection and the second create cannot happen:

plain CREATE, twice        first ok, second failed

Both engines. db_error_skip is set on that query, so $createTemporary is false and the whole cleanup step is skipped in silence — the same failure mode this pull request has just fixed, reached another way. It takes $db_persist = true for a connection to outlive the request, so most installs never meet it, and the shape predates this branch. It is worth closing while these lines are being rewritten anyway.

IF NOT EXISTS does exist on all three, to answer the question directly — including on the AS SELECT form, which is the part I was least sure of:

MySQL 8.4      CREATE TEMPORARY TABLE IF NOT EXISTS … AS SELECT   accepted
MariaDB 11.8   CREATE TEMPORARY TABLE IF NOT EXISTS … AS SELECT   accepted
PostgreSQL 17  CREATE TEMPORARY TABLE IF NOT EXISTS … AS SELECT   accepted

But it makes the bug quieter rather than fixing it. IF NOT EXISTS skips the CREATE, and skipping the create means keeping the rows the previous run put there. Creating it from a query returning three rows and then again from one returning none:

                  after the first    after the second (wanted 0)
MySQL 8.4              3 rows                 3 rows
MariaDB 11.8           3 rows                 3 rows
PostgreSQL 17          1 row                  1 row

So the second recount would decide who has a stale post count by consulting the first recount's answer. That is worse than the current behaviour, which at least does nothing.

DROP TABLE IF EXISTS in front of the CREATE is the fix, and it is portable:

                  drop    create    rows after (wanted 0)
MySQL 8.4          ok       ok            0
MariaDB 11.8       ok       ok            0
PostgreSQL 17      ok       ok            0

Two things to avoid on the way there:

• DROP TEMPORARY TABLE IF EXISTS is MySQL family only. It is the safer spelling there, since it cannot reach a permanent table of the same name by mistake — but PostgreSQL rejects it outright, so it cannot go in shared code. Plain DROP TABLE IF EXISTS resolves through search_path to pg_temp first on PostgreSQL, and prefers the temporary table on MySQL, so it does the right thing on both.

• Db::$db->drop_table() will not do it. It guards on list_tables(), and a temporary table does not appear there, so the call returns having done nothing at all:

  Db::$db->drop_table()      before 1 row(s), after 1 row(s)
  

  Both engines. Same blind spot as list_columns() in add_index(), which I mentioned last time — the catalogue simply cannot see temporary tables, so the schema helpers quietly no-op on them. Worth knowing, given add_index() is now called on this table two lines further down.