New subscriptions don't get stored in the DB because the test for 'fallback2db' was wrong.
I just noticed the insert_shtable() function is also used to load from DB initially, so my patch is wrong. Fix is a bit more non-trivial.
The problem is that the function is used for both DB-load at startup and new subscriptions during runtime. The db_flag in the hash table record needs to be INSERTDB_FLAG during runtime, and NO_UPDATEDB_FLAG for initial load.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The function is used in presence and rls modules. Does not look like big changes to add a new param to specify the mode. Do you think of something else?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
fallback to db is in presence and the insert function is used by rls module as well. I had no time to check the impact now.
With this idea, to be safe, fallback2db param has to be directed to another variable, fallback2db initialized to 0 and set to param value in each child, via child_init. Otherwise can be a conflict of loading order.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I decided to dive into the code some more. It's a bit messy. There are 2 modes of operation determined by fallback2db. Both modes don't work correctly, the documentation for the modes contradict the code it contradicts ITSELF. So the first thing to do to fix this is to get the desired behavior for both modes documented.
From the code I deduced the following behavior:
fallback mode:
init: load from database, keep data in tables
runtime: insert/update records in table on timer event
shutdown: flush cache to db
non-fallback mode:
init: load from database, empty table
runtime: no inserts but try to update table (won't work because table is emptied)
shutdown: flush cache to db (fails, because all entries are marked NO_UPDATEDB_FLAG du to the (failed) updates at runtime). Result is an empty table after shutdown.
The documentation says in fallback mode the cache isn't used. This is impossible in the current code. Later in the docs it is said only on cache misses, the db is used. This seems reasonable behavior.
The main question is: should the db be updated during runtime or only on shutdown when in non-fallback mode?
Attached patch creates the following behavior:
for both modes:
init: load from database, keep table contents
runtime: update/insert into table on timer event
shutdown: update/insert final time
The only difference in the modes is on lookup (as documentation says) and is untouched by this patch.
The non-fallback mode should be optimized by not updating the table if the answer to the above question says so, but that is not as trivial as this patch.
ps. I found some nice descriptive function names: update_db_subs() and update_subs_db()....
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
db_mode=
=0: no DB
=1: read from cache, write/update to cache and DB
=2: read from cache, write to cache and delayed write/update to DB
=3: db_only, no cache
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I agree that choice is better, but it requires a major rewrite. Maybe a common library can be created for all modules using DB and cache. Maybe even throw in a memcached cache as a choice. And prepared statements in case the DB is used during runtime.....
Just missing my favorite mode:
=4: cache only at runtime, DB save/restore at shutdown/init time.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Please provide a bit more details of what got broken.
New subscriptions don't get stored in the DB because the test for 'fallback2db' was wrong.
I just noticed the insert_shtable() function is also used to load from DB initially, so my patch is wrong. Fix is a bit more non-trivial.
The problem is that the function is used for both DB-load at startup and new subscriptions during runtime. The db_flag in the hash table record needs to be INSERTDB_FLAG during runtime, and NO_UPDATEDB_FLAG for initial load.
The function is used in presence and rls modules. Does not look like big changes to add a new param to specify the mode. Do you think of something else?
I don't know if changing the API and ABI is appropriate for the stable series.
What about setting the fallback2db variable only AFTER loading the DB contents?
fallback to db is in presence and the insert function is used by rls module as well. I had no time to check the impact now.
With this idea, to be safe, fallback2db param has to be directed to another variable, fallback2db initialized to 0 and set to param value in each child, via child_init. Otherwise can be a conflict of loading order.
I decided to dive into the code some more. It's a bit messy. There are 2 modes of operation determined by fallback2db. Both modes don't work correctly, the documentation for the modes contradict the code it contradicts ITSELF. So the first thing to do to fix this is to get the desired behavior for both modes documented.
From the code I deduced the following behavior:
fallback mode:
init: load from database, keep data in tables
runtime: insert/update records in table on timer event
shutdown: flush cache to db
non-fallback mode:
init: load from database, empty table
runtime: no inserts but try to update table (won't work because table is emptied)
shutdown: flush cache to db (fails, because all entries are marked NO_UPDATEDB_FLAG du to the (failed) updates at runtime). Result is an empty table after shutdown.
The documentation says in fallback mode the cache isn't used. This is impossible in the current code. Later in the docs it is said only on cache misses, the db is used. This seems reasonable behavior.
The main question is: should the db be updated during runtime or only on shutdown when in non-fallback mode?
Attached patch creates the following behavior:
for both modes:
init: load from database, keep table contents
runtime: update/insert into table on timer event
shutdown: update/insert final time
The only difference in the modes is on lookup (as documentation says) and is untouched by this patch.
The non-fallback mode should be optimized by not updating the table if the answer to the above question says so, but that is not as trivial as this patch.
ps. I found some nice descriptive function names: update_db_subs() and update_subs_db()....
Fix try 2
Patch is ok, cleaner with new parameter rather than playing with falback2db value. I will apply it.
Updating on timer is good for the cases of sudden crash, maybe a new mode to skip timer updates and store only at shutdown is more appropriate.
What about having a new parameter like userloc?
db_mode=
=0: no DB
=1: read from cache, write/update to cache and DB
=2: read from cache, write to cache and delayed write/update to DB
=3: db_only, no cache
I agree that choice is better, but it requires a major rewrite. Maybe a common library can be created for all modules using DB and cache. Maybe even throw in a memcached cache as a choice. And prepared statements in case the DB is used during runtime.....
Just missing my favorite mode:
=4: cache only at runtime, DB save/restore at shutdown/init time.
IIRC, this was fixed, so item can be closed, right?
Yes, as far as I remember it is fixed.