208680 - Support read only filesystems and load params from db

Status: REOPENED

(Bugzilla :: Installation & Upgrading, enhancement)

Description

[Jason Pyeron]

User-Agent:       Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0; .NET CLR 1.0.3705)
Build Identifier: 

Right now defparams.pl and data/params is called from Bugzilla/Config.pm 
EACH time a new Bugzilla operation is done.

this should be stored in the database. (a cache may still be wanted on disk, 
like data/versioncache)

the defaults should be loaded from checksetup.pl

something like this...

create table params
(
 id int not null primary key auto_increment,
 sortindx int,
 name varchar(255) not null,
 desc text,
 type char not null, -- use and enum?
 choices text,
 valuedefault text,
 checker varchar(255),
 value text -- this is the local value
)

Reproducible: Always

Steps to Reproduce:

Comment 1

[Dave Miller [:justdave]]

This was discussed in-depth in bug 46296 before the focus was narrowed on that
bug, and it was decided that it wouldn't work because too many of the params are
required for basic operation even when the database is not accessible.

Although my opinion now is that it still may be a possibility if the params in
question are identified and moved into localconfig perhaps instead of being in
params.

Comment 2

[Zach Lipton [:zach]]

In reality, this probably isn't a good idea. Since some variables will always
have to come from some form of file (database server, userid, etc...), we will
always have to run a small perl script from disk at runtime. This doesn't really
take very long though I don't think. Moving it into the database means that we
have to do the current process of loading the params needed for Bugzilla to
function at all and another process of loading the additional params from the
database. That seems like it might be slower to me :)

Of course, this assumes that the purpose of putting the params in the database
is for speed. Is this what you had in mind? If so, there are a lot slower
operations we do than loading the params.

I tried profiling index.cgi on my machine and the amount of time we spend in
Bugzilla::Config is only about 0.94% of the time index.cgi took to execute on my
box. 

Comment 3

[Jason Pyeron]

IN SUMMARY FROM (will discuss them in next comment) BUG 46296: The params need 
to be moved out of a flat-file into the database, so that the parameter 
definition is easily expansible (such as adding a category column for each 
parameter - see below).

FROM BUG 46296, comment 8: Once all the params are in the DB, would they still 
be cached somewhere in a file?

I was looking at the 'bugzilla is unavailable because of a backup' page a while
ago, and I wondered how the 'headerhtml' param, for instance, would be retrieved
if the DB was unavailable?

FROM BUG 46296, comment 9: I've got a few questions and a db proposal, if 
anyone has any ideas or suggestions...

1) (as per my previous comment) How should parameters like 'shutdownhtml' be
managed? If the DB is down, then we can't look it up, and wouldn't want to try.

2) I like the idea of only reading (and caching) parameters when they are
needed. In people's experience, would that actually make much of a startup
difference compared to reading in all the parameters early on for every Bugzilla
page?

3) There is a bit of a problem when it comes to reading the Params from the DB:
if the SendSQL() routine needs any params (which it does), then it will need to
look them up, which will, of course require SendSQL()... :( 

I'm contemplating number 3) at the moment.

Anyway, here is a proposed DB to support the global parameters, which,
fortunately, should also support the user params in bug#98123...

Index: checksetup.pl
===================================================================
RCS file: /opt/repositories/sysadmin/src/bugzilla/checksetup.pl,v
retrieving revision 1.1.1.4
diff -w -u -r1.1.1.4 checksetup.pl
--- checksetup.pl	2001/08/30 09:01:06	1.1.1.4
+++ checksetup.pl	2001/09/14 15:17:18
@@ -1127,6 +1127,120 @@
      index(userid)';
 
 
+#
+# Preferences Group table
+#
+# pref_group_name - e.g. 'mail', 'admin', 'ldap', 
+#
+# pref_group_display_name - Used for GUIs
+#
+# pref_group_desc - Description of the group
+#
+# pref_group_sort_order - Used to order the display of preference groups,
+#                         however they are displayed
+#
+$table{'pref_group'} = 
+    'pref_group_id mediumint not null auto_increment primary key ,
+     pref_group_name varchar(50) not null ,
+     pref_group_display_name varchar(50) not null ,
+     pref_group_desc varchar(255) not null ,
+     pref_group_sort_order smallint not null ,
+
+     index(pref_group_name),
+     index(pref_group_sort_order)
+
+    ';
+
+# Preference definitions
+#
+# pref_defs_id - Unique Id for this preference definition (is auto_increment a
+#                MYSQL-specific concept?)
+#
+# pref_group_id - ID of a row in the pref_group table
+#
+# pref_display_order - Used to order the display of prefs on GUIs, within a 
+#                      particular pref. group 
+#
+# pref_display_name - string to display on GUIs
+#
+# pref_name - string used to id this pref in code. List of valid pref/
+#             names will be stored in globals.pl, or something like a
+#             prefs.pl, if globals.pl is split up. e.g. 'user_css_enable',
+#             'display_search_criteria', 'bannerhtml', 'useldap'
+#
+# pref_data_type - string indicating data type of this pref. List of valid
+#                  pref data types will be stored in globals.pl, or
+#                  something like a prefs.pl, if globals.pl is split
+#                  up. e.g. 'string', 'number', 'boolean', 'url',
+#                  'enum'. (Could be new DB table - but is that overkill>)
+#
+# pref_description - Description of this pref to be used in GUIs etc.
+#
+# pref_default - The default value as distributed with Bugzilla.
+#
+# pref_def_data1 - If the preference has extra data associated with it, then 
+# pref_def_data2   these are the defaults, as distributed with Bugzilla.
+#
+
+$table{pref_defs} = 
+    'pref_defs_id mediumint not null auto_increment primary key ,
+     pref_group_id mediumint  not null ,
+     pref_display_order smallint not null ,
+     pref_display_name varchar(50) not null ,
+     pref_name varchar(50) not null ,
+     pref_data_type varchar(20) not null ,
+     pref_description varchar(255) ,
+     pref_default mediumtext not null,
+     pref_def_data1 mediumtext ,
+     pref_def_data2 mediumtext ,
+
+     index(pref_group_id) ,
+     index(pref_display_order) ,
+     index(pref_name)';
+
+#
+# Bugzilla Prefs table
+#
+# For a pref_type of 'user', there will be an item in this table with a
+# pref_type_id == -1 which indicates the default value for the installed
+# system (which may be different to the Bugzilla distribution default).
+#
+# For a pref_type of 'system', the pref_type_id is not used
+#
+#
+# Column specs
+#-------------------------------------
+# pref_defs_id - What sort of pref is this?
+#
+# pref_type_id - DB ID related to the pref_type field. e.g. if the type is
+#                'user', then this pref_type_id will be the user_id
+#
+# pref_type - The 'type' of preference this is. An enum of
+#             strings. Initially 'user', or 'system', but there may be more
+#             in the future. (user groups?. per product (display) prefs?
+#             Valid pref types will be stored in globals.pl, or something
+#             like prefs.pl, if globals.pl is split up.
+#
+# pref_value - The actual value, as defined by the pref_data_type in the
+#              pref_defs table
+#
+# pref_data1 - Extra data associated with this preference
+#
+# pref_data2 - Extra data associated with this preference
+#
+#
+# Any indexes needed?
+#
+$table{prefs} = 
+    'pref_defs_id mediumint not null ,
+     pref_type_id mediumint ,
+     pref_type varchar(20) not null ,
+     pref_value mediumtext not null ,
+     pref_data1 mediumtext,
+     pref_data2 mediumtext';
+
+
+
 
 ###########################################################################
 # Create tables

FROM BUG 46296, comment 10: Currently lots of stuff is shoved into the 
versioncache so things don't need to
be read over.

As for shutdownhtml, that could be in localconfig, although a template might be
another option.

FROM BUG 46296, comment 13: Some of the params should probably go to 
localconfig, to handle the situation where you have multiple installations 
sharing a database (supporting a compatible schema).

In particular, urlbase, and possibly cookiepath.

FROM BUG 46296, comment 15: We're not putting the params in the database.  Too 
much of that stuff needs to be outside of the database for everything else to 
work properly.  The original reason given was so it could be more 
expansible...  well, that's been done, because it's now stored in a two-layer 
hash instead of a flat file.  If you think specific params need to be in the 
database, for some reason, file a bug for that param specifically..

Comment 4

[Jason Pyeron]

Attachment: GavinS patch for checksetup.pl creating pref's tables

from comment 3 or bug 46296, comment 9

I will be hacking this one up later

Comment 5

[Jason Pyeron]

Addressing GavinS and Matthew Tuck concerns in bug 46296, comment 8, bug 46296, 
comment 9, bug 46296, comment 10, bug 46296, comment 13 (comment 3):

As for data which there are issues which a query may not be wanted (speed?, 
circular?, whatever) each time a Bugzilla operation is performed then make use 
of the versioncache setup. that’s what I stated in comment 1.

As to the issue of params which are used when the database is offline, etc. 
those should go in localconfig. shutdownhtml, etc. see bug 208709.

Addressing GavinS issue #2 (comment 3), this is not relevant here so it should 
be a separate bug, but this is what we do for i18n support it is a good idea.

Addressing the patch (attachment (id=125186)):

pref_group_display_name,
pref_display_name

how can these be i18n ready?

does bugzilla have a strings file/table? how does this impact the "templates"?

from defparams.pl:
# OK, here are the parameter definitions themselves.
#
# Each definition is a hash with keys:
#
# name    - name of the param
# desc    - description of the param (for editparams.cgi)
# type    - see below
# choices - (optional) see below
# default - default value for the param
# checker - (optional) checking function for validating parameter entry
#           It is called with the value of the param as the first arg and a
#           reference to the param's hash as the second argument

it needs a checker reference column.

Addressing Dave's issue bug 46296, comment 15 (comment 3):

No the reason is seperation of code & config, esp for backup issues. IMHO I 
should be able to (1) restore my database from backup tape then (2) checkout 
bugzilla from CVS (3) tweak localconfig to point to my db server (4) 
[./checksetup.pl] and run!

This is proper application design.


PS: GavinS, I have added you to the cc, cause you seemed interested in this and 
I wanted your opinion.

Comment 6

[Zach Lipton [:zach]]

Before we get into patches and schemas, can we determine if we want to do this?
I'm inclined to call this wontfix:

* More complexity

* More places where params are stored

* No speed gain/possible slowdown

I'm no expert with the perl profiler and it's possible I used dprofpp
incorrectly. If so, please let me know, but with my current figures (only .94%
of index.cgi's time is spent in Bugzilla::Config vs more than 15% with
templates), I just don't see the use of this. 

As for keeping the information in the db in order to make it easier to restore
from a backup (see the end of comment 5): the admin would already have to set
the db information, the maintainer email (can't be set in the db because we need
it for error messages if the db cannot be found), and a bunch of other params as
well.

Furthermore, while you propose using a versioncache-style setup to cache params,
I think it's fair to say that most of the Bugzilla maintainers want and are
planning for versioncache to go away in the future. 

Comment 7

[Jason Pyeron]

so other than what is listed below, what else is needed in localconfig?

and this is damn ideal for backup/restore. If you cannot fill this in from the 
README or checksetup instructions then you have no business trying to install 
software.

localconfig:

$default_email = 'root@localhost';
$mysqlpath = "";
$db_host = "127.0.0.1";         # where is the database?
$db_port = 3306;                # which port to use
$db_name = "bugs";              # name of the MySQL database
$db_user = "bugs";              # user to attach to the MySQL database
$db_pass = '';
$db_sock = '';
$db_check = 1;

Comment 8

[Jason Pyeron]

re comment 6:

con: * More complexity
pro: easier to read code and filebase

con: * More places where params are stored
pro: not true, drop defparams, and other misc locations to table storage.

con: * No speed gain/possible slowdown
pro: increased reliability

re: Furthermore, while you propose using a versioncache-style setup to cache 
params, I think it's fair to say that most of the Bugzilla maintainers want and 
are planning for versioncache to go away in the future. 

personally, I think this 'versioncache' is silly. I would fix bugzilla to have 
webserver persistance, and shared memory. But it is neither here nor there. I 
merely pointed it out to refute those who have a feer that bugzilla will grind 
to a halt.

Comment 9

[Dave Miller [:justdave]]

re comment 8 last paragraph, see bug 87406 and its dependencies.

Comment 10

[Jason Pyeron]

great, then versioncache will go the way of the dinosaurs, and the hashes will 
be resident to avoid repeat db calls to get simple info.

This is even more reason to go DB.

I am going stick to reason #1 backup/restore and #2 its just proper

Comment 11

[Zach Lipton [:zach]]

re comment 8: 

I don't see how it makes it "easier to read code and filebase." First of all,
what exactly is a filebase? Secondly, Bugzilla::Config seems pretty clear to me
as it is and the API for accessing data from the params is quite simple.

How would this get rid of defparams? The parameters still have to be  defined
_somewhere_ and a file called defparams.pl seems to make sense to me as a place
to do so. 

"Increased reliability:" A text file is (IMHO) far more reliable in this case.
it rarely changes, but is accessed in a read-only form quite frequently. Using
the database is just overkill. 

Comment 9:

"#1 backup/restore": admins can just backup the data/ directory (I would think
that most admins would backup their bugzilla install since they might have
custom templates or patches installed) or just reset their params. In the
unlikely event that an admin manages to lose their params somehow, it's not a
huge deal to reset them. 

"#2 its just proper": what do you mean by "just proper"? 

Comment 12

[Jason Pyeron]

okay this one got me again....

I need to make this patch ASAP before I get bit again.

Comment 13

[Frédéric Buclin]

updating the bug summary. defparams.pl no longer exists on tip.

Comment 14

[H.-Dirk Schmitt]

Why not closing this bug as WONTFIX after nearly 10 years ?

Comment 15

[Gervase Markham [:gerv]]

If you have performance problems, use persistence like mod_perl and/or we should implement caching. Moving the params to the DB is not the solution.

Gerv

Comment 17

[GitHub Autolander Bot]

Attachment: [bugzilla] dylanwh:bug-208680 > bugzilla:master

Comment 18

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8767440 [details] [review]
[bugzilla] dylanwh:bug-208680 > bugzilla:master

I resolved both issues from the previous version of the pull request.

I stand by this method because it doesn't cause a change in behavior for people running in enterprise-y environments.

Comment 19

[David Lawrence [:dkl]]

Comment on attachment 8767440 [details] [review]
[bugzilla] dylanwh:bug-208680 > bugzilla:master

comments on PR