Piwigo.org

nicolas · 2009-11-14 15:26:36

Hi all,

I want to improve queries and how we manage datas comming from database. There will be many steps because many files will be involved ! Theses steps are :

1) I want to replace all mysql_fetch_array() functions by mysql_fetch_assoc(). We allways use hash tables and mysql_fetch_array() return two arrays : hash and array indexes by field number. So we retrieve the datas twice.

In some place we can't do that and I will remplace mysql_fetch_array() by mysql_fetch_row() or mysql_result(). For example, when we want to retrieve a single value :

Code:

$query='
SELECT galleries_url
  FROM '.SITES_TABLE.'
  WHERE id = '.$site_id;
list($site_url) = mysql_fetch_array(pwg_query($query));

We only want galleries_url value. I think the following code is better :

Code:

list($site_url) = mysql_fetch_row(pwg_query($query));

or that one :

Code:

$site_url = mysql_result(pwg_query($query), 0);

2) I want the [Bugtracker] ticket 928 to be resolved (by me or plg I don't care). It's not so easy as it seems to be.

3) I want to improve php functions that manipulate datas like pwg_query(). The idea is to allways use that functions and to easily manipulate datas from plugins when needed.

4) I want to add code/library to add database abstraction. I found in love with postgreSQL and I want to replace mysql by postgres. But I think it will be interresting to improve queries for example. I think it could be interresting to also use SQLite for functionnal, non-regression tests.

What do you think of that ?

VDigital · 2009-11-14 16:31:38

1) Yes, step by step, and think about plugins which need some rewrite too. We should have more pwg functions around managing Db. (pwg_single_query for example).

2) Yes, absolutely.

3) Yes. Link to 1)

4) Yes, the real objective in fact.

nicolas · 2009-11-14 20:41:18

VDigital wrote:
4) Yes, the real objective in fact.

Yes you understand.

plg · 2009-11-14 23:29:10

1) very good point, I'm not sure it will improve anything in term of performance, but in my recent development (outside Piwigo) I quite never use mysql_fetch_array, only mysql_fetch_row or mysql_fetch_assoc

2) reassign it to yourself if you want :-)

3) you mean some functions like array_from_query, mass_inserts or mass_updates? In PEM, we also have array_of_arrays_from_query and simple_hash_from_query. For inserts, I've recently found the following syntax:

Code:

INSERT INTO table
    SET col1 = val1
        , col2 = val2
        , col3 = val3
;

(MySQL specific I suppose) and I find it terrific. Better than the function simple I've coded for dlcounter.php (the script you call when you download Piwigo ou piwigo.org):

Code:

function clean_insert($table_name, $fields, $ignore=false) {
  $query = '
INSERT '.($ignore ? 'IGNORE' : '').' INTO '.$table_name.'
  ('.implode(',', array_keys($fields)).')
  VALUES (';

  $first = true;
  foreach ($fields as $value) {
    if ($first) {
      $first = false;
    }
    else {
      $query.= ', ';
    }
    $query.= "'".$value."'";
  }

  $query.= ')';
  // echo $query; exit();
  mysql_query($query);
}

4) Of course, THIS is the big point of your topic.

First I would say that supporting PostgreSQL doesn't not seem that relevant for Piwigo, in my opinion. It's interesting to work with PostgreSQL (I've done it on other projects) and it's a really nice database BUT for Piwigo I see a big work in the code for only a technical challenge that is not useful for users (In 8 years, I think we've had 5 requests for supporting PostgreSQL, not more, that's a very low number).

Then I would say that supporting SQLite is much more interesting. Because YES, it makes Piwigo easier to install, and that means a real benefit for users.

I suppose that if we modify the code to support SQLite, we will also be able to support PostgreSQL.

There are several ways to become "database independant" in a software, at implementation level. What do you have in mind? grouping database calls in a single place? having some if(mysql)elseif(postgresql)elseif(sqlite) next to each query which would be database dependant (they do this in punBB/fluxBB, and it's just awful)

related topics/bugs/features:

* [Bugtracker] ticket 511

nicolas · 2009-11-15 10:23:00

plg wrote:
1) very good point, I'm not sure it will improve anything in term of performance, but in my recent development (outside Piwigo) I quite never use mysql_fetch_array, only mysql_fetch_row or mysql_fetch_assoc

I agree. It doesn't really change performance but it's better so why not !

plg wrote:
2) reassign it to yourself if you want :-)

ok.

plg wrote:
point 3

I mean using functions like pwg_query() but more specific : pwg_db_connect(), pwg_db_insert(), pwg_db_delete(), pwg_db_mass_insert(),...

plg wrote:
4) Of course, THIS is the big point of your topic.

First I would say that supporting PostgreSQL doesn't not seem that relevant for Piwigo, in my opinion. It's interesting to work with PostgreSQL (I've done it on other projects) and it's a really nice database BUT for Piwigo I see a big work in the code for only a technical challenge that is not useful for users (In 8 years, I think we've had 5 requests for supporting PostgreSQL, not more, that's a very low number).

I think it could be useful for code improvement like php code is better when severals people look at it or like when software have to run on many platforms or architectures. I think sql code will be better if it has to run on many database engine. Actually some piece of sql code are awful and not written in a standard way.

plg wrote:
Then I would say that supporting SQLite is much more interesting. Because YES, it makes Piwigo easier to install, and that means a real benefit for users.

I suppose that if we modify the code to support SQLite, we will also be able to support PostgreSQL.

You're absolutly right. No difference in supporting two or several databases engines. If people want to use something else they can.

plg wrote:
There are several ways to become "database independant" in a software, at implementation level. What do you have in mind? grouping database calls in a single place? having some if(mysql)elseif(postgresql)elseif(sqlite) next to each query which would be database dependant (they do this in punBB/fluxBB, and it's just awful)

My idea is to write independant functions to manipulate datas and group them in a database engine specific file like include/dblayer/mysql.inc.php, include/dblayer/pgsql.inc.php, include/dblayer/sqlite.inc.php,... for example. With this way piwigo core code change less and there less chance to have bugs.

Ok, so let's go.

VDigital · 2009-11-15 11:05:29

Assume this global rule: Piwigo does not provide any migration tool.

( postgresql <-> MySQL <-> sqlite ): Never !!!

nicolas · 2009-11-15 11:37:57

VDigital wrote:
Assume this global rule: Piwigo does not provide any migration tool.

( postgresql <-> MySQL <-> sqlite ): Never !!!

Ok but we can retrieve datas in a same way in theses databases engines even if queries aren't exactly the same.

nicolas · 2009-11-15 14:21:48

Step 1 : done [Subversion] r4265

Step 2 : [Bugtracker] ticket 928
The idea is to move the content of mysql.inc.php at the end of config_local.inc.php file.

The config file mysql.inc.php is used in many places : install.php, upgrade.php and by config_default.inc.php.
The file config_default.inc.php used $prefixeTable comming from mysql.inc.php. $prefixeTable is used is this file to do external authentification. I proposed to move that customization in config_local.inc.php which is the right place like the others customizations. I will had a comment in config_default.inc.php to explain that.

Todo after code commit :
Documentation will have to be updated in many places. LocaleFileEditor plugin will have to be updated to prevent people from removing database configuration.

Last edited by nicolas (2009-11-15 20:11:42)

VDigital · 2009-11-15 15:37:29

[Bugtracker] ticket 985
or

Step 2: [Bugtracker] ticket 928

Don't forget the grum's note on [Bugtracker] ticket 545

plg · 2009-11-15 21:35:36

nicolas wrote:
The idea is to move the content of mysql.inc.php at the end of config_local.inc.php file.

And make it config_$.inc.php compliant I think. I mean having $conf['cfgBase'] instead of $cfgBase (and even better $conf['db_name'], $conf['db_user'], $conf['db_password']).

nicolas wrote:
The file config_default.inc.php used $prefixeTable comming from mysql.inc.php. $prefixeTable is used is this file to do external authentification. I proposed to move that customization in config_local.inc.php which is the right place like the others customizations. I will had a comment in config_default.inc.php to explain that.

Nicely spotted, and I agree with the solution you propose.

nicolas wrote:
Todo after code commit :
Documentation will have to be updated in many places. LocaleFileEditor plugin will have to be updated to prevent people from removing database configuration.

Here is a question maybe more important than it seems: by default LocalFilesEditor is simply writting a configuration file, without taking into account its content. Now we mean it has to understand and prevent from doing "mistakes". I ask P@t to come and give his feeling about this.

nicolas · 2009-11-15 21:52:55

plg wrote:
nicolas wrote:
The idea is to move the content of mysql.inc.php at the end of config_local.inc.php file.
And make it config_$.inc.php compliant I think. I mean having $conf['cfgBase'] instead of $cfgBase (and even better $conf['db_name'], $conf['db_user'], $conf['db_password']).

good idea.

plg wrote:
Here is a question maybe more important than it seems: by default LocalFilesEditor is simply writting a configuration file, without taking into account its content. Now we mean it has to understand and prevent from doing "mistakes". I ask P@t to come and give his feeling about this.

LocalFilesEditor can verify some important parameters to be present like database configuration. My idea is to include config_local.inc.php and parse $conf array to save content of the file and allow user to modify content. If importants parameters are missing, new file is not save.

plg · 2009-11-15 22:00:08

VDigital wrote:
Don't forget the grum's note on [Bugtracker] ticket 545

Sorry VDigital, this is exactly what I had mind when I replied (without having read your post).

Let's also ask grum to participate to this discussion.

plg · 2009-11-15 22:04:02

nicolas wrote:
LocalFilesEditor can verify some important parameters to be present like database configuration. My idea is to include config_local.inc.php and parse $conf array to save content of the file and allow user to modify content. If importants parameters are missing, new file is not save.

The problem is also to hide database connection parameters in LocalFilesEditor, not only prevent from removing them.

As config_local.inc.php is a PHP file that may contain more than a list of $conf['key'] = 'val'; (function declation for example), I think it can be more complex than it seems.

VDigital · 2009-11-15 22:19:10

May I suggest.

config_database.inc.php (Not LocalFiles Editable)
config_default.inc.php (Not LocalFiles Editable)
config_local.inc.php ( LocalFiles Editable and with database config overrides enable ).

grum · 2009-11-15 23:02:07

VDigital wrote:
config_database.inc.php (Not LocalFiles Editable)
config_default.inc.php (Not LocalFiles Editable)
config_local.inc.php ( LocalFiles Editable and with database config overrides enable ).

Ok to rename the mysql.inc.php file into "config_database.inc.php" : if we want to be able to manage other databases engines (SQLite, PostgreSQL) it's a good thing to have a config file name not database oriented.
And Ok to use a $conf[] value

But I don't understand why we want to allow the database config in the config_local.inc.php ?

I think it's a hole for the security.
You can try to hide database config in the editor, so in this case, I really don't understand the "plus value".
You can try to improve the LocalFileEditor to prevent error, but LocalFileEditor can have a bug, or the user can made mistake not managed by the editor.

About the database improvment, I can read "I mean using functions like pwg_query() but more specific : pwg_db_connect(), pwg_db_insert(), pwg_db_delete(), pwg_db_mass_insert(),..."
May be we can made a class providing the database functions.
Something like $pwg_db->querie($sql) or $pwg_db->fetch_row()
By using something like this, it will be easiest to use SQLite or PostgreSQL.
And probably we can use framework dedicated to this..

Piwigo.org

Announcement

#1 2009-11-14 15:26:36

Database improvements

Code:

Code:

Code:

#2 2009-11-14 16:31:38

Re: Database improvements

#3 2009-11-14 20:41:18

Re: Database improvements

VDigital wrote:

#4 2009-11-14 23:29:10

Re: Database improvements

Code:

Code:

#5 2009-11-15 10:23:00

Re: Database improvements

plg wrote:

plg wrote:

plg wrote:

plg wrote:

plg wrote:

plg wrote:

#6 2009-11-15 11:05:29

Re: Database improvements

#7 2009-11-15 11:37:57

Re: Database improvements

VDigital wrote:

#8 2009-11-15 14:21:48

Re: Database improvements

#9 2009-11-15 15:37:29

Re: Database improvements

#10 2009-11-15 21:35:36

Re: Database improvements

nicolas wrote:

nicolas wrote:

nicolas wrote:

#11 2009-11-15 21:52:55

Re: Database improvements

plg wrote:

nicolas wrote:

plg wrote:

#12 2009-11-15 22:00:08

Re: Database improvements

VDigital wrote:

#13 2009-11-15 22:04:02

Re: Database improvements

nicolas wrote:

#14 2009-11-15 22:19:10

Re: Database improvements

#15 2009-11-15 23:02:07

Re: Database improvements

VDigital wrote:

Board footer