Страшной ненависти к быдлокодерам пост

[dil]

А сегодня ночью один скриптик перловый, запускаемый из крона, не отработал. Бывает. Ну я его перезапустил. А он чего-то думает долго. Минут через десять мне надоело ждать и я посмотрел в код.  А там ТАКОЕ… что прям хоть целиком в code_wtf отправляй.

Начало было уже хорошее:

if ( -e $filepath)
{
        unlink($filepath);
}
open (FILE, ">> $filepath");

На хрена, спрашивается, стирать файл, да ещё внешней программой, а потом открывать его на дописывание?

А дальше была реализована тривиальная, в общем-то, задача: выбираем из одной базы список пользователей, потом проверяем для них некоторые критерии в другой базе, и если совпало, то достаём регистрационные данные  и записываем в отдельную таблицу. Но КАК она реализована…

Во-первых, выборка пользователей почему-то работает очень долго, хотя всего по двум таблицам. Вскрытие показало, что в одной из таблиц много миллионов записей, а индекса по полю, по которому она связывается с другой таблицей, нет. Имеем многократный full scan в полный рост. Многократный – потому что там вложенный подзапрос с агрегирующей функцией плюс ещё во внешнем запросе группировка с использованием max(). И мне не очень понятно, зачем так сделано, но это я буду выяснять потом. Короче, работает он минут десять. После добавления простого индекса стал работать четыре, и DBA обещали подумать, как его можно ещё улучшить.

Но это фигня. Про binding параметров аффтар, видать, не подозревал. Не дочитал до этого места в документации. Поэтому дальше в цикле для каждого найденного пользователя формируется _строка_ с запросом на проверку критериев и выборку данных из второй базы. Имя пользователя вставляется непосредственно в эту строку,  далее на неё запускается prepare и execute. Запрос, естественно, никто не проверял на оптимальность, поэтому там тоже full scan, и работает он секунд пять. И  так для каждого из пары тысяч найденных пользователей. Поэтому скрипт работает в общей сложности больше двух часов, напрягая сервер почём зря.

Запись в таблицу реализована аналогично — каждый раз формируется строка с запросом, prepare, execute. Пару тысяч раз.

while(($username,$blah...) = $sth->fetchrow()) {
        ...
        $loadline ="\"$username\", \"$blah\",...";
        $loadline =~ s/,,/,NULL,/g;
        $loadline =~ s/,$/,NULL/g;
        $insertsql = "insert into foo values ($loadline); ";
        $sth2 = $dbh->prepare($insertsql);
        $sth2->execute();
}

Главное, совершенно непонятно,  нафига вытягивать данные в приложение и отправлять их во вторую базу через другое соединение. Базы хоть и разные, но на одном сервере. Обращаться к соседней базе можно через одно соединение  без всяких накладных расходов.

Когда я это осознал, громко выругался вслух. Потом написал письмо DBA. Которые высказались примерно в том же духе, мягко назвав это bad coding.  Потом я за 20 минут переписал этот ужас, сложив результаты первого запроса во временную таблицу и прицепив её ко второму. И попросив DBA добавить пару индексов.

Время работы скрипта сократилось с двух часов до четырёх минут,  которые, собственно,  занимает первый запрос. Второй работает меньше секунды.

Самое страшное, что этот скрипт там не один. Их многие десятки, и я сильно подозреваю, что в остальных ужас примерно такой же.

И ведь этих людей кто-то взял на работу. И они пишут код, который потом годами работает, потребляя ресурсы совершенно зазря. И в который никто не смотрит. Я туда заглянул совершенно случайно…

Upd: на unlink это уже тоже я поменял. В оригинале было так:

if ( -e $filepath)
{
        `/bin/rm $filepath`;
        `/usr/bin/touch $filepath`;
}
else
{
        `/usr/bin/touch $filepath`;
}
open (FILE, ">> $filepath");

Оригинал этой записи. Комментировать можно тут или там.

Любые материалы из этого блога запрещается использовать на сайте livejournal.ru в любой форме и любом объёме

Comments

[unbe.livejournal.com]

> На хрена, спрашивается, стирать файл, да ещё внешней программой
"внешней программой" ?

а в остальном - везде так

[jerom.livejournal.com]

unlink встроенная функция

[dil.livejournal.com]

см. апдейт

[lslarry.livejournal.com]

If MODE is '>>', the file is opened for appending, again being created if necessary
но, собсно, оно в самом деле лишнее

if ( -e $filepath)
{
unlink($filepath);
}
open (FILE, ">> $filepath");

это вот - реальная хуйня. потому что, как минимум,
1) > (а не >>) реально делает ненужным unlink
If MODE is '>', the file is truncated and opened for output, being created if necessary.
2) не проверять то, что возвращает open - просто западло

хотя хули, мир несовершенен.
http://lslarry.livejournal.com/38648.html

[tejblum.livejournal.com]

Это, конечно, в данном случае скорее всего неважно, но:

1. unlink() в подобных случаях может оказаться нужен, поскольку потенциально меняет поведение программы. Стереть файл и создать его заново -- не то же самое, что обрезать его. Например, если указанное имя -- симлинк куда-то далеко, то unlink() и open() сотрет симлинк, и создаст файл в указанном месте, а open() без unlink()'а -- обрежет тот файл, куда указывает симлинк, и будет писать туда.


2. "open for appending", который делает >>, и "open for output", который делает > -- не одно и то же. Подробней об этом написано в man 2 open, в разеделе про O_APPEND.

[dil.livejournal.com]

в данном конкретном случае он не нужен, в этот файл каждый раз пишутся текущие данные, а потом он отправляется по почте. достаточно open на запись.

[alexkuklin.livejournal.com]

Да-да.
"Был на нашей деревне случай" (ц), когда отсутствие unlink привело к потере данных - т.к. копия делалась при помощи hardlink.

[lazylonelion.livejournal.com]

думаю что и микрософтовский код, и, скажем, айбиэмовский, и, скажем, опенсорсный в линуксе полон подобного кода.


Повсеместно бытует и расширяется мнение, что факторизовать и оптимизировать код надо не раньше, чем наступят проблемы с производительностью или багами.

Что твой пример успешно подтверждает: вам приходилось ждать жалкие часы и раньше, да и на правку кода тебе понадобилось 20 минут. При том что подобного кода там однозначно осталось дофига, но он тебе (судя по всему) не мешает.

На всякий случай уточню, что мне такой подход нифига не нравится, но не я рулю рынком и не ты. :(