adam/Piwigo
1
0 Fork 0
mirror of https://github.com/Piwigo/Piwigo.git synced 2026-11-03 07:22:22 +01:00
Code Issues 657 Packages Projects Releases Wiki Activity

MySQL error when deleting categories #1002

New Issue
Open
opened 2026-10-05 00:27:59 +02:00 by adam · 3 comments
adam commented 2026-10-05 00:27:59 +02:00
Owner
Copy link
Copy source

I'm currently developing a new Perl module for the WS API and ran into the following MySQL error:

Warning:  [mysql error 1064] You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ')' at line 4

SELECT id
  FROM images
  WHERE storage_category_id IN (
)
; in [redacted]/include/dblayer/functions_mysqli.inc.php on line 856


Fatal error: Uncaught Error: Call to a member function fetch_assoc() on boolean in [redacted]/include/dblayer/functions_mysqli.inc.php:917
Stack trace:
#0 [redacted]/admin/include/functions.php(71): query2array('\nSELECT id\n FR...', NULL, 'id')
#1 [redacted]/admin/cat_list.php(145): delete_categories(Array, 'no_delete')
#2 [redacted]/admin.php(309): include('/var/www/pics.t...')
#3 {main} thrown in [redacted]/include/dblayer/functions_mysqli.inc.php on line 917

Unfortunately I cannot reproduce it. It happened on the second deletion after I called pwg.categories.add twice with the same (unset) parent and the same name (create_virtual_category() should probably prevent this in the first place as it can only lead to confusion), then tried to delete the two categories again.
Apparently it can happen that get_subcat_ids() returns an empty list, and the SQL assembly in admin/include/functions.php doesn't guard against that.

From a security standpoint this is horrible BTW. This kind of SQL bricolage should happen, if at all in this day and age, in a very small DB layer and not all over the place. I could easily add a check for this instance, but that wouldn't solve much.

I'm currently developing a new Perl module for the WS API and ran into the following MySQL error: ``` Warning: [mysql error 1064] You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ')' at line 4 SELECT id FROM images WHERE storage_category_id IN ( ) ; in [redacted]/include/dblayer/functions_mysqli.inc.php on line 856 Fatal error: Uncaught Error: Call to a member function fetch_assoc() on boolean in [redacted]/include/dblayer/functions_mysqli.inc.php:917 Stack trace: #0 [redacted]/admin/include/functions.php(71): query2array('\nSELECT id\n FR...', NULL, 'id') #1 [redacted]/admin/cat_list.php(145): delete_categories(Array, 'no_delete') #2 [redacted]/admin.php(309): include('/var/www/pics.t...') #3 {main} thrown in [redacted]/include/dblayer/functions_mysqli.inc.php on line 917 ``` Unfortunately I cannot reproduce it. It happened on the second deletion after I called `pwg.categories.add` twice with the same (unset) parent and the same name (`create_virtual_category()` should probably prevent this in the first place as it can only lead to confusion), then tried to delete the two categories again. Apparently it can happen that `get_subcat_ids()` returns an empty list, and the SQL assembly in `admin/include/functions.php` doesn't guard against that. From a security standpoint this is horrible BTW. This kind of SQL bricolage should happen, if at all in this day and age, in a very small DB layer and not all over the place. I could easily add a check for this instance, but that wouldn't solve much. <!-- github-sync-id:560116697 -->
adam commented 2026-10-05 00:28:00 +02:00
Author
Owner
Copy link
Copy source

Hi @mbethke

When calling pwg.categories.delete, function ws_categories_delete is executed, in which you call the delete_categories functions with a list of category_ids. At this point, the list cannot be empty. The get_subcat_ids can only return an empty list if the album (=category) has been deleted in the meantime by another script. So here I think we have a highly unlikely concurrence issue. That's certainly why you can't reproduce it easily.

From a security standpoint this is horrible BTW. This kind of SQL bricolage [...]

I do not agree. This error message helps us to understand the problem. What kind of security issue is there here? You're feared to reveal the database model? Piwigo is opensource, anybody can know the table structure.

if at all in this day and age, in a very small DB layer and not all over the place [...]

If your point is to say "it's bad to embed SQL queries in PHP code", I also strongly disagree. I know it's not hype to not use a multiple abstraction layer to access the database, but at least we have an optimized use of database, instead of generic SQL queries which are so badly optimized that you need to add cache...

Hi @mbethke When calling `pwg.categories.delete`, function `ws_categories_delete` is executed, in which you call the `delete_categories` functions with a list of `category_ids`. At this point, the list cannot be empty. The `get_subcat_ids` can only return an empty list if the album (=category) has been deleted in the meantime by another script. So here I think we have a highly unlikely concurrence issue. That's certainly why you can't reproduce it easily. > From a security standpoint this is horrible BTW. This kind of SQL bricolage [...] I do not agree. This error message helps us to understand the problem. What kind of security issue is there here? You're feared to reveal the database model? Piwigo is opensource, anybody can know the table structure. > if at all in this day and age, in a very small DB layer and not all over the place [...] If your point is to say "it's bad to embed SQL queries in PHP code", I also strongly disagree. I know it's not hype to not use a multiple abstraction layer to access the database, but at least we have an optimized use of database, instead of generic SQL queries which are so badly optimized that you need to add cache... <!-- github-comment-id:583429376 -->
adam commented 2026-10-05 00:28:01 +02:00
Author
Owner
Copy link
Copy source

Hi,
makes sense, I don't remember exactly but it might be that I deleted the two from different browser tabs.

I wasn't trying to suggest you hide the SQL errors, that's fine when it happens, and even if the schema wasn't publicly available, security by obscurity was never a good idea anyway. But the piecing together of SQL queries from strings should be isolated in a dedicated module. I'm not usually a fan of fat ORM layers either but looking at those queries such as in get_subcat_ids() that use DISTINCT on a primary key, treat numeric values as strings and always do full table scans with multiple regex applications makes me wonder if an ORM couldn't do better (instead of saying WHERE uppercats REGEXP '(^|,)1(,|$)' OR uppercats REGEXP '(^|,)23(,|$)' OR uppercats REGEXP '(^|,)42(,|$)' you could at least build the regex and say WHERE uppercats REGEXP '(^|,)(1|23|42)(,|$)'). At least it would probably turn the whole thing into one race-free query like DELETE FROM categories WHERE <criteria> instead of successive SELECT id FROM categories WHERE <criteria> and DELETE FROM categories WHERE id IN (<hand-assembled list>).

You could probably speed up this category lookup business by a few orders of magnitude by dropping the uppercats column completely and adding an intermediate table. I just tried with this:

# Copied from DESCRIBE with all columns but uppercats
CREATE TABLE `categories_new` (
`id` smallint(5) unsigned NOT NULL AUTO_INCREMENT,
`name` varchar(255) NOT NULL DEFAULT '',
`id_uppercat` smallint(5) unsigned DEFAULT NULL,
`comment` text,   `dir` varchar(255) DEFAULT NULL,
`rank` smallint(5) unsigned DEFAULT NULL,
`status` enum('public','private') NOT NULL DEFAULT 'public',
`site_id` tinyint(4) unsigned DEFAULT NULL,
`visible` enum('true','false') NOT NULL DEFAULT 'true',
`representative_picture_id` mediumint(8) unsigned DEFAULT NULL,
`commentable` enum('true','false') NOT NULL DEFAULT 'true',
`global_rank` varchar(255) DEFAULT NULL,
`image_order` varchar(128) DEFAULT NULL,
`permalink` varchar(64) CHARACTER SET utf8 COLLATE utf8_bin DEFAULT NULL,
`lastmodified` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
`community_user` mediumint(8) unsigned DEFAULT NULL,
PRIMARY KEY (`id`),
UNIQUE KEY `categories_i3` (`permalink`),
KEY `categories_i2` (`id_uppercat`),
KEY `lastmodified` (`lastmodified`)
) ENGINE=innodb DEFAULT CHARSET=utf8;

# Intermediate - note the storage engine, so FKs work
create table uppercats (
id SMALLINT(5) unsigned NOT NULL,
upper SMALLINT(5) unsigned NOT NULL,
PRIMARY KEY(id,upper),
foreign key(id) references categories_new(id) on update cascade on delete cascade,
foreign key(upper) references categories_new(id) on update cascade on delete cascade
) engine=innodb;

# Fill from original categories
insert into categories_new
select id, name, id_uppercat, comment, dir, rank, status, site_id, visible,
representative_picture_id, commentable, global_rank, image_order,
permalink, lastmodified, community_user
from categories;

# Fill uppercats table from original column
mysql -Dpics -B -e 'select id,uppercats from categories' | \
tail -n+2 | \
perl -Mstrict -w -nE 'chomp;my($id,$u)=split/\t/,$_; say "INSERT INTO uppercats VALUES($id,$_);" for split /,/,$u;' | \
mysql -Dpics

Then you find all parent categories for a given category ID using

select c.id,c.name from categories c join uppercats u on c.id=u.upper where u.id=<ID>;

and all children:

select c.id,c.name from categories c join uppercats u on c.id_uppercat=u.id where u.upper=<ID>;

No regex, fully indexed, and the intermediate table has quite a few rows but is still tiny.

Hi, makes sense, I don't remember exactly but it might be that I deleted the two from different browser tabs. I wasn't trying to suggest you hide the SQL errors, that's fine when it happens, and even if the schema wasn't publicly available, security by obscurity was never a good idea anyway. But the piecing together of SQL queries from strings should be isolated in a dedicated module. I'm not usually a fan of fat ORM layers either but looking at those queries such as in `get_subcat_ids()` that use `DISTINCT` on a primary key, treat numeric values as strings and always do full table scans with multiple regex applications makes me wonder if an ORM couldn't do better (instead of saying `WHERE uppercats REGEXP '(^|,)1(,|$)' OR uppercats REGEXP '(^|,)23(,|$)' OR uppercats REGEXP '(^|,)42(,|$)'` you could at least build the regex and say WHERE uppercats REGEXP '(^|,)(1|23|42)(,|$)'). At least it would probably turn the whole thing into one race-free query like `DELETE FROM categories WHERE <criteria>` instead of successive `SELECT id FROM categories WHERE <criteria>` and `DELETE FROM categories WHERE id IN (<hand-assembled list>)`. You could probably speed up this category lookup business by a few orders of magnitude by dropping the `uppercats` column completely and adding an intermediate table. I just tried with this: ```SQL # Copied from DESCRIBE with all columns but uppercats CREATE TABLE `categories_new` ( `id` smallint(5) unsigned NOT NULL AUTO_INCREMENT, `name` varchar(255) NOT NULL DEFAULT '', `id_uppercat` smallint(5) unsigned DEFAULT NULL, `comment` text, `dir` varchar(255) DEFAULT NULL, `rank` smallint(5) unsigned DEFAULT NULL, `status` enum('public','private') NOT NULL DEFAULT 'public', `site_id` tinyint(4) unsigned DEFAULT NULL, `visible` enum('true','false') NOT NULL DEFAULT 'true', `representative_picture_id` mediumint(8) unsigned DEFAULT NULL, `commentable` enum('true','false') NOT NULL DEFAULT 'true', `global_rank` varchar(255) DEFAULT NULL, `image_order` varchar(128) DEFAULT NULL, `permalink` varchar(64) CHARACTER SET utf8 COLLATE utf8_bin DEFAULT NULL, `lastmodified` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, `community_user` mediumint(8) unsigned DEFAULT NULL, PRIMARY KEY (`id`), UNIQUE KEY `categories_i3` (`permalink`), KEY `categories_i2` (`id_uppercat`), KEY `lastmodified` (`lastmodified`) ) ENGINE=innodb DEFAULT CHARSET=utf8; # Intermediate - note the storage engine, so FKs work create table uppercats ( id SMALLINT(5) unsigned NOT NULL, upper SMALLINT(5) unsigned NOT NULL, PRIMARY KEY(id,upper), foreign key(id) references categories_new(id) on update cascade on delete cascade, foreign key(upper) references categories_new(id) on update cascade on delete cascade ) engine=innodb; # Fill from original categories insert into categories_new select id, name, id_uppercat, comment, dir, rank, status, site_id, visible, representative_picture_id, commentable, global_rank, image_order, permalink, lastmodified, community_user from categories; ``` ```bash # Fill uppercats table from original column mysql -Dpics -B -e 'select id,uppercats from categories' | \ tail -n+2 | \ perl -Mstrict -w -nE 'chomp;my($id,$u)=split/\t/,$_; say "INSERT INTO uppercats VALUES($id,$_);" for split /,/,$u;' | \ mysql -Dpics ``` Then you find all parent categories for a given category ID using ```sql select c.id,c.name from categories c join uppercats u on c.id=u.upper where u.id=<ID>; ``` and all children: ```sql select c.id,c.name from categories c join uppercats u on c.id_uppercat=u.id where u.upper=<ID>; ``` No regex, fully indexed, and the intermediate table has quite a few rows but is still tiny. <!-- github-comment-id:583735418 -->
adam commented 2026-10-05 00:28:01 +02:00
Author
Owner
Copy link
Copy source

Hi @mbethke

Very interesting. This is technical interesting. Of course, manipulating integer instead of strings would be better. On the other side, the categories.uppercats is straighforward to understand, on a coding point of view. I had also studied a hierachy model with "left/right" values, see http://mikehillyer.com/articles/managing-hierarchical-data-in-mysql/ to get an idea, but I rejected it because it's too complex. Much more complex than the way Piwigo did it. And I deeply think that simplicity makes a code more sustainable.

The other reason I think it would be a bad idea to change the way we deal with hierarchy is the impact in current code. categories.uppercats is used in many places, including in plugins. We don't really have a problem of performance with the current model, so changing it would mainly bring problems. The "perfection of the code" is not enough to justify such a big model change.

But I say it again: it's a good idea to challenge the way things are currently made.

you could at least build the regex and say WHERE uppercats REGEXP '(^|,)(1|23|42)(,|$)')

OK, this is much more feasible. If it really improves speed, it's a good idea. I need to make test/benchmark on a big categories table (at least 1000 albums) to see how faster it could be.

Hi @mbethke Very interesting. This is technical interesting. Of course, manipulating integer instead of strings would be better. On the other side, the `categories.uppercats` is straighforward to understand, on a coding point of view. I had also studied a hierachy model with "left/right" values, see http://mikehillyer.com/articles/managing-hierarchical-data-in-mysql/ to get an idea, but I rejected it because it's too complex. Much more complex than the way Piwigo did it. And I deeply think that simplicity makes a code more sustainable. The other reason I think it would be a bad idea to change the way we deal with hierarchy is the impact in current code. `categories.uppercats` is used in many places, including in plugins. We don't really have a problem of performance with the current model, so changing it would mainly bring problems. The "perfection of the code" is not enough to justify such a big model change. But I say it again: it's a good idea to challenge the way things are currently made. > you could at least build the regex and say `WHERE uppercats REGEXP '(^|,)(1|23|42)(,|$)')` OK, this is much more feasible. If it really improves speed, it's a good idea. I need to make test/benchmark on a big categories table (at least 1000 albums) to see how faster it could be. <!-- github-comment-id:584160995 -->
Sign in to join this conversation.
No Branch/Tag Specified
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
adam (Adam Wojcieszonek)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: adam/Piwigo#1002