diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md new file mode 100644 index 00000000000..50cca4dd48d --- /dev/null +++ b/MIGRATION_GUIDE.md @@ -0,0 +1,212 @@ +# Migration Guide: Add `updated_at` to MUC Rooms + +## Overview + +This migration adds an `updated_at` column to the `muc_room` table to properly track when rooms are modified, separate from when they were created. + +## Why This Change? + +Previously, the `created_at` field was incorrectly: +1. Set to `1970-01-02 00:00:00` for new rooms +2. Updated every time the room was modified +3. Confused with `hibernation_time` (when room goes to sleep) + +This migration fixes these issues by: +- Ensuring `created_at` is set correctly on room creation and never changes +- Adding `updated_at` to track when room configuration is modified +- Separating concerns between creation time, update time, and hibernation time + +## Prerequisites + +1. Backup your database before running migration +2. Stop ejabberd or ensure no rooms are being created/modified during migration +3. Compile the new ejabberd code + +## Migration Steps + +### Step 1: Backup Database + +```bash +# MySQL +mysqldump -u root -p ejabberd > ejabberd_backup_$(date +%Y%m%d).sql + +# PostgreSQL +pg_dump ejabberd > ejabberd_backup_$(date +%Y%m%d).sql + +# SQLite +cp /path/to/ejabberd.db /path/to/ejabberd_backup_$(date +%Y%m%d).db +``` + +### Step 2: Run Migration SQL + +#### For MySQL/MariaDB: + +```sql +USE ejabberd; + +-- Add updated_at column +ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; + +-- Initialize updated_at with created_at value +UPDATE muc_room SET updated_at = created_at; + +-- Verify +SELECT name, host, created_at, updated_at FROM muc_room LIMIT 10; +``` + +#### For PostgreSQL: + +```sql +\c ejabberd; + +-- Add updated_at column +ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; + +-- Initialize updated_at with created_at value +UPDATE muc_room SET updated_at = created_at; + +-- Verify +SELECT name, host, created_at, updated_at FROM muc_room LIMIT 10; +``` + +#### For SQLite: + +```sql +-- Add updated_at column +ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; + +-- Initialize updated_at with created_at value +UPDATE muc_room SET updated_at = created_at; + +-- Verify +SELECT name, host, created_at, updated_at FROM muc_room LIMIT 10; +``` + +#### For MSSQL: + +```sql +USE ejabberd; + +-- Add updated_at column +ALTER TABLE muc_room ADD updated_at datetime NOT NULL DEFAULT CURRENT_TIMESTAMP; + +-- Initialize updated_at with created_at value +UPDATE muc_room SET updated_at = created_at; + +-- Verify +SELECT TOP 10 name, host, created_at, updated_at FROM muc_room; +``` + +### Step 3: Compile and Restart ejabberd + +```bash +cd /path/to/ejabberd +./rebar3 compile +ejabberdctl restart +``` + +### Step 4: Verify + +```bash +# Create a test room +ejabberdctl create_room testroom conference.localhost localhost + +# Check database +# MySQL/PostgreSQL/SQLite: +SELECT name, created_at, updated_at FROM muc_room WHERE name='testroom'; + +# Both timestamps should be current time and equal + +# Update room option +ejabberdctl change_room_option testroom conference.localhost title "Test Room" + +# Check database again +SELECT name, created_at, updated_at FROM muc_room WHERE name='testroom'; + +# created_at should be unchanged, updated_at should be newer +``` + +## Rollback + +If you need to rollback: + +```sql +-- MySQL/PostgreSQL/SQLite +ALTER TABLE muc_room DROP COLUMN updated_at; + +-- MSSQL +ALTER TABLE muc_room DROP COLUMN updated_at; +``` + +Then restore your backup and use the old ejabberd code. + +## Troubleshooting + +### Issue: Migration fails with "column already exists" + +**Solution:** The column may have been added in a previous attempt. Check if it exists: + +```sql +-- MySQL +SHOW COLUMNS FROM muc_room LIKE 'updated_at'; + +-- PostgreSQL +SELECT column_name FROM information_schema.columns +WHERE table_name='muc_room' AND column_name='updated_at'; + +-- SQLite +PRAGMA table_info(muc_room); +``` + +If it exists, skip to Step 3. + +### Issue: Old rooms still have `1970-01-02` in `created_at` + +**Solution:** This is expected for rooms created before the migration. You can optionally update them: + +```sql +-- Set created_at to updated_at for old rooms +UPDATE muc_room +SET created_at = updated_at +WHERE created_at < '1971-01-01 00:00:00'; +``` + +### Issue: Performance concerns on large databases + +**Solution:** For very large `muc_room` tables (>1M rows), consider: + +1. Add the column without default first: +```sql +ALTER TABLE muc_room ADD COLUMN updated_at timestamp NULL; +``` + +2. Update in batches: +```sql +-- MySQL +UPDATE muc_room SET updated_at = created_at WHERE updated_at IS NULL LIMIT 10000; +-- Repeat until all rows updated + +-- PostgreSQL +UPDATE muc_room SET updated_at = created_at WHERE updated_at IS NULL; +``` + +3. Make it NOT NULL after all rows are updated: +```sql +ALTER TABLE muc_room MODIFY updated_at timestamp NOT NULL; +``` + +## Benefits After Migration + +1. ✅ Accurate room creation timestamps +2. ✅ Track when room configuration changes +3. ✅ Better analytics and reporting +4. ✅ Clearer separation of concerns +5. ✅ No more `1970-01-02 00:00:00` timestamps! + +## Support + +If you encounter issues, please report them on the ejabberd GitHub repository with: +- Database type and version +- ejabberd version +- Error messages +- Output of `SELECT * FROM muc_room LIMIT 1;` diff --git a/PULL_REQUEST_PROPOSAL.md b/PULL_REQUEST_PROPOSAL.md new file mode 100644 index 00000000000..71ead830cc2 --- /dev/null +++ b/PULL_REQUEST_PROPOSAL.md @@ -0,0 +1,228 @@ +# Fix MUC Room `created_at` Timestamp Issue + +## 🐛 Problem + +Currently, the `created_at` field in the `muc_room` table is incorrectly set to `1970-01-02 00:00:00` for newly created rooms. This happens because: + +1. The `store_room/5` function in `mod_muc_sql.erl` uses `hibernation_time` from room options to set `created_at` +2. When a room is first created, `hibernation_time` is not set (or is `undefined`) +3. The fallback value is hardcoded to `1970-01-02 00:00:00` + +### Current Code (mod_muc_sql.erl:146-150) +```erlang +Timestamp = case lists:keyfind(hibernation_time, 1, Opts) of + false -> <<"1970-01-02 00:00:00">>; + {_, undefined} -> <<"1970-01-02 00:00:00">>; + {_, Time} -> usec_to_sql_timestamp(Time) +end, +``` + +### Issues with Current Implementation: +- **Semantic confusion**: `hibernation_time` represents when a room goes to sleep (no users), not when it was created +- **Incorrect timestamps**: All new rooms get `1970-01-02 00:00:00` as creation time +- **UPSERT behavior**: On updates, `created_at` is overwritten with hibernation time, which is semantically wrong + +## 💡 Proposed Solutions + +### Solution 1: Separate `created_at` and `updated_at` (Recommended) + +Add a new `updated_at` column and fix the semantics: + +**Database Schema Changes:** +```erlang +#sql_table{ + name = <<"muc_room">>, + columns = + [#sql_column{name = <<"name">>, type = text}, + #sql_column{name = <<"host">>, type = text}, + #sql_column{name = <<"server_host">>, type = text}, + #sql_column{name = <<"opts">>, type = {text, big}}, + #sql_column{name = <<"created_at">>, type = timestamp, + default = true}, + #sql_column{name = <<"updated_at">>, type = timestamp, + default = true}], + ... +} +``` + +**Code Changes (mod_muc_sql.erl):** +```erlang +store_room(LServer, Host, Name, Opts, ChangesHints) -> + {Subs, Opts2} = case lists:keytake(subscribers, 1, Opts) of + {value, {subscribers, S}, OptN} -> {S, OptN}; + _ -> {[], Opts} + end, + SOpts = misc:term_to_expr(Opts2), + CurrentTime = usec_to_sql_timestamp(erlang:system_time(microsecond)), + + F = fun () -> + case ejabberd_sql:sql_query_t( + ?SQL("select @(created_at)t from muc_room where " + "name=%(Name)s and host=%(Host)s")) of + {selected, [{CreatedAt}]} -> + % Room exists, update only updated_at + ?SQL_UPSERT_T( + "muc_room", + ["!name=%(Name)s", + "!host=%(Host)s", + "server_host=%(LServer)s", + "opts=%(SOpts)s", + "created_at=%(CreatedAt)t", + "updated_at=%(CurrentTime)t"]); + _ -> + % New room, set both created_at and updated_at + ?SQL_UPSERT_T( + "muc_room", + ["!name=%(Name)s", + "!host=%(Host)s", + "server_host=%(LServer)s", + "opts=%(SOpts)s", + "created_at=%(CurrentTime)t", + "updated_at=%(CurrentTime)t"]) + end, + % Handle subscribers... + end, + ejabberd_sql:sql_transaction(LServer, F). +``` + +**Migration SQL:** +```sql +-- MySQL +ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; +UPDATE muc_room SET updated_at = created_at WHERE updated_at IS NULL; + +-- PostgreSQL +ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; +UPDATE muc_room SET updated_at = created_at WHERE updated_at IS NULL; + +-- SQLite +ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; +UPDATE muc_room SET updated_at = created_at WHERE updated_at IS NULL; +``` + +### Solution 2: Fix `created_at` to Use Current Time (Simple Fix) + +If we don't want to add `updated_at`, at least fix `created_at` to use the current time: + +```erlang +Timestamp = case lists:keyfind(hibernation_time, 1, Opts) of + false -> usec_to_sql_timestamp(erlang:system_time(microsecond)); + {_, undefined} -> usec_to_sql_timestamp(erlang:system_time(microsecond)); + {_, Time} -> usec_to_sql_timestamp(Time) +end, +``` + +**Pros:** +- Minimal code change +- No database migration needed +- Fixes the immediate issue + +**Cons:** +- Still uses `hibernation_time` for `created_at` (semantic confusion) +- `created_at` gets updated on every room update + +### Solution 3: Add `hibernation_time` Support in API (Workaround) + +Allow users to pass `hibernation_time` via API: + +**Code Changes (mod_muc_admin.erl:1734):** +```erlang +format_room_option(OptionString, ValueString) -> + Option = misc:binary_to_atom(OptionString), + Value = case Option of + ... + hibernation_time -> try_convert_integer(Option, ValueString); + ... + end, + {Option, Value}. +``` + +**API Usage:** +```bash +curl -X POST /api/create_room_with_opts \ + -H "Content-Type: application/json" \ + -d '{ + "room": "myroom", + "service": "conference.example.com", + "host": "example.com", + "options": [ + { + "name": "hibernation_time", + "value": "1704672000000000" + } + ] + }' +``` + +**Pros:** +- Gives API users control over `created_at` +- Minimal code change + +**Cons:** +- Still semantic confusion between `hibernation_time` and `created_at` +- Requires users to manually calculate microsecond timestamps + +## 🎯 Recommendation + +**Solution 1** is the best approach because: +1. ✅ Separates concerns: `created_at` vs `updated_at` vs `hibernation_time` +2. ✅ Follows database best practices +3. ✅ Provides more information (when created AND when last updated) +4. ✅ Fixes the semantic confusion +5. ✅ Backward compatible with proper migration + +## 📊 Impact Analysis + +### Files to Modify: +- `src/mod_muc_sql.erl` - Update schema and store_room function +- `sql/mysql.new.sql` - Add updated_at column +- `sql/pg.new.sql` - Add updated_at column +- `sql/lite.new.sql` - Add updated_at column +- `sql/mssql.new.sql` - Add updated_at column + +### Backward Compatibility: +- Migration script provided +- Existing rooms will have `updated_at = created_at` initially +- No breaking changes to API + +## 🧪 Testing + +```bash +# Test 1: Create new room +ejabberdctl create_room testroom1 conference.localhost localhost + +# Verify created_at and updated_at are set to current time +SELECT name, created_at, updated_at FROM muc_room WHERE name='testroom1'; + +# Test 2: Update room options +ejabberdctl change_room_option testroom1 conference.localhost title "New Title" + +# Verify created_at unchanged, updated_at updated +SELECT name, created_at, updated_at FROM muc_room WHERE name='testroom1'; + +# Test 3: Room hibernation +# Let room hibernate (all users leave) +# Verify created_at and updated_at remain unchanged +``` + +## 📝 Additional Notes + +The current implementation conflates three different concepts: +1. **`created_at`**: When the room was first created (should never change) +2. **`updated_at`**: When the room configuration was last modified +3. **`hibernation_time`**: When the room went to sleep (runtime state, not persistent metadata) + +This PR proposes to properly separate these concerns. + +## 🔗 Related Issues + +- Rooms created via API have incorrect `created_at` timestamp +- `get_hibernated_rooms_older_than` query filters out rooms with `1970-01-02` timestamp +- Confusion between room creation time and hibernation time + +--- + +**Author**: Community Contribution +**Type**: Bug Fix + Enhancement +**Priority**: Medium +**Backward Compatible**: Yes (with migration) diff --git a/sql/lite.new.sql b/sql/lite.new.sql index 42f289fb38a..b7da77053e3 100644 --- a/sql/lite.new.sql +++ b/sql/lite.new.sql @@ -282,7 +282,8 @@ CREATE TABLE muc_room ( server_host text NOT NULL, host text NOT NULL, opts text NOT NULL, - created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ); CREATE UNIQUE INDEX i_muc_room_name_host ON muc_room (name, host); diff --git a/sql/lite.sql b/sql/lite.sql index b31e02b793d..c272587ac22 100644 --- a/sql/lite.sql +++ b/sql/lite.sql @@ -260,7 +260,8 @@ CREATE TABLE muc_room ( name text NOT NULL, host text NOT NULL, opts text NOT NULL, - created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ); CREATE UNIQUE INDEX i_muc_room_name_host ON muc_room (name, host); diff --git a/sql/migration_add_updated_at.sql b/sql/migration_add_updated_at.sql new file mode 100644 index 00000000000..ac3a535dc34 --- /dev/null +++ b/sql/migration_add_updated_at.sql @@ -0,0 +1,21 @@ +-- Migration script to add updated_at column to muc_room table +-- This script should be run after upgrading ejabberd + +-- For MySQL/MariaDB +-- ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; +-- UPDATE muc_room SET updated_at = created_at; + +-- For PostgreSQL +-- ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; +-- UPDATE muc_room SET updated_at = created_at; + +-- For SQLite +-- ALTER TABLE muc_room ADD COLUMN updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP; +-- UPDATE muc_room SET updated_at = created_at; + +-- For MSSQL +-- ALTER TABLE muc_room ADD updated_at datetime NOT NULL DEFAULT CURRENT_TIMESTAMP; +-- UPDATE muc_room SET updated_at = created_at; + +-- Note: Uncomment the appropriate section for your database + diff --git a/sql/mssql.new.sql b/sql/mssql.new.sql index f67033eedef..c818278b54e 100644 --- a/sql/mssql.new.sql +++ b/sql/mssql.new.sql @@ -122,7 +122,8 @@ CREATE TABLE [dbo].[muc_room] ( [host] [varchar] (250) NOT NULL, [server_host] [varchar] (250) NOT NULL, [opts] [text] NOT NULL, - [created_at] [datetime] NOT NULL DEFAULT GETDATE() + [created_at] [datetime] NOT NULL DEFAULT GETDATE(), + [updated_at] [datetime] NOT NULL DEFAULT GETDATE() ) TEXTIMAGE_ON [PRIMARY]; CREATE UNIQUE CLUSTERED INDEX [muc_room_name_host] ON [muc_room] (name, host) diff --git a/sql/mssql.sql b/sql/mssql.sql index ab5596d4869..d2fa5d8f19f 100644 --- a/sql/mssql.sql +++ b/sql/mssql.sql @@ -113,7 +113,8 @@ CREATE TABLE [dbo].[muc_room] ( [name] [varchar] (250) NOT NULL, [host] [varchar] (250) NOT NULL, [opts] [text] NOT NULL, - [created_at] [datetime] NOT NULL DEFAULT GETDATE() + [created_at] [datetime] NOT NULL DEFAULT GETDATE(), + [updated_at] [datetime] NOT NULL DEFAULT GETDATE() ) TEXTIMAGE_ON [PRIMARY]; CREATE UNIQUE CLUSTERED INDEX [muc_room_name_host] ON [muc_room] (name, host) diff --git a/sql/mysql.new.sql b/sql/mysql.new.sql index cf818ad3dd8..e248b8c4eae 100644 --- a/sql/mysql.new.sql +++ b/sql/mysql.new.sql @@ -301,7 +301,8 @@ CREATE TABLE muc_room ( host text NOT NULL, server_host varchar(191) NOT NULL, opts mediumtext NOT NULL, - created_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP + created_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ) ENGINE=InnoDB CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci; CREATE UNIQUE INDEX i_muc_room_name_host USING BTREE ON muc_room(name(75), host(75)); diff --git a/sql/mysql.sql b/sql/mysql.sql index 630c4a55786..7e87b292a6f 100644 --- a/sql/mysql.sql +++ b/sql/mysql.sql @@ -277,7 +277,8 @@ CREATE TABLE muc_room ( name text NOT NULL, host text NOT NULL, opts mediumtext NOT NULL, - created_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP + created_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ) ENGINE=InnoDB CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci; CREATE UNIQUE INDEX i_muc_room_name_host USING BTREE ON muc_room(name(75), host(75)); diff --git a/sql/pg.new.sql b/sql/pg.new.sql index 1e59ec571eb..820c81da015 100644 --- a/sql/pg.new.sql +++ b/sql/pg.new.sql @@ -453,7 +453,8 @@ CREATE TABLE muc_room ( host text NOT NULL, server_host text NOT NULL, opts text NOT NULL, - created_at TIMESTAMP NOT NULL DEFAULT now() + created_at TIMESTAMP NOT NULL DEFAULT now(), + updated_at TIMESTAMP NOT NULL DEFAULT now() ); CREATE UNIQUE INDEX i_muc_room_name_host ON muc_room USING btree (name, host); diff --git a/sql/pg.sql b/sql/pg.sql index dd83e087ea6..a88857fa1ae 100644 --- a/sql/pg.sql +++ b/sql/pg.sql @@ -279,7 +279,8 @@ CREATE TABLE muc_room ( name text NOT NULL, host text NOT NULL, opts text NOT NULL, - created_at TIMESTAMP NOT NULL DEFAULT now() + created_at TIMESTAMP NOT NULL DEFAULT now(), + updated_at TIMESTAMP NOT NULL DEFAULT now() ); CREATE UNIQUE INDEX i_muc_room_name_host ON muc_room USING btree (name, host); diff --git a/src/mod_muc_admin.erl b/src/mod_muc_admin.erl index 9a8ab60b1c4..152e7a6d72b 100644 --- a/src/mod_muc_admin.erl +++ b/src/mod_muc_admin.erl @@ -1731,6 +1731,7 @@ format_room_option(OptionString, ValueString) -> subject_author ->ValueString; max_users -> try_convert_integer(Option, ValueString); voice_request_min_interval -> try_convert_integer(Option, ValueString); + hibernation_time -> try_convert_integer(Option, ValueString); vcard -> ValueString; vcard_xupdate when ValueString /= <<"undefined">>, ValueString /= <<"external">> -> diff --git a/src/mod_muc_sql.erl b/src/mod_muc_sql.erl index 31c8703c1f3..3513c01ce62 100644 --- a/src/mod_muc_sql.erl +++ b/src/mod_muc_sql.erl @@ -73,6 +73,8 @@ sql_schemas() -> #sql_column{name = <<"server_host">>, type = text}, #sql_column{name = <<"opts">>, type = {text, big}}, #sql_column{name = <<"created_at">>, type = timestamp, + default = true}, + #sql_column{name = <<"updated_at">>, type = timestamp, default = true}], indices = [#sql_index{ columns = [<<"name">>, <<"host">>], @@ -143,11 +145,7 @@ store_room(LServer, Host, Name, Opts, ChangesHints) -> _ -> {[], Opts} end, SOpts = misc:term_to_expr(Opts2), - Timestamp = case lists:keyfind(hibernation_time, 1, Opts) of - false -> <<"1970-01-02 00:00:00">>; - {_, undefined} -> <<"1970-01-02 00:00:00">>; - {_, Time} -> usec_to_sql_timestamp(Time) - end, + CurrentTime = usec_to_sql_timestamp(erlang:system_time(microsecond)), F = fun () -> ?SQL_UPSERT_T( "muc_room", @@ -155,7 +153,8 @@ store_room(LServer, Host, Name, Opts, ChangesHints) -> "!host=%(Host)s", "server_host=%(LServer)s", "opts=%(SOpts)s", - "created_at=%(Timestamp)t"]), + "-created_at=%(CurrentTime)t", + "updated_at=%(CurrentTime)t"]), case ChangesHints of Changes when is_list(Changes) -> [change_room(Host, Name, Change) || Change <- Changes]; @@ -277,7 +276,7 @@ get_hibernated_rooms_older_than(LServer, Host, Timestamp) -> case catch ejabberd_sql:sql_query( LServer, ?SQL("select @(name)s, @(opts)s from muc_room" - " where host=%(Host)s and created_at < %(TimestampS)t and created_at > '1970-01-02 00:00:00'")) of + " where host=%(Host)s and created_at < %(TimestampS)t")) of {selected, RoomOpts} -> lists:map( fun({Room, Opts}) ->