Контрибьютим в PostgreSQL: примеры реальных патчей, часть 1 из N

  • Tutorial


Ранее в статье Становимся контрибьютером в PostgreSQL был подробно рассмотрен процесс разработки PostgreSQL и используемые при этом инструменты, были предложены некоторые идеи для первого патча и рассказано, куда и как эти патчи нужно посылать. Также были приведены ссылки на дополнительные источники информации касательно внутреннего устройства РСУБД.

Теперь же мы рассмотрим примеры реальных патчей, принятых в PostgreSQL за последнее время. Какие-то из этих патчей были написаны непосредственно мной, при разработке других я активно участвовал в качестве ревьювера. Это сравнительно небольшие патчи. На момент написания этих строк я занимаюсь разработкой PostgreSQL менее года, и ранее разработкой СУБД я не занимался (ровно как и разработкой на языке C за деньги). Поэтому есть основания полагать, что данные патчи будут интересны новичкам, желающим начать участвовать в разработке открытых проектов, притом не обязательно именно PostgreSQL. Чтобы не писать лонгридов, статья разбита на части.

Заинтересовавшихся прошу проследовать под кат.

1. Удаление дублированного кода в ReorderBufferCleanupTXN()


Мне нравится время от времени проходиться по коду PostgreSQL разными статическими анализаторами, особенно Clang Static Analyzer. Часто эти анализаторы ругаются на какую-то сомнительную ерунду, но среди этой ерунды иногда можно найти действительно очень подозрительные куски кода. Один из таких кусков выглядел следующим образом:

/* delete from list of known subxacts */
if (txn->is_known_as_subxact)
{
    /* NB: nsubxacts count of parent will be too high now */
    dlist_delete(&txn->node);
}
/* delete from LSN ordered list of toplevel TXNs */
else
{
    dlist_delete(&txn->node);
}

Согласитесь, довольно странно делать в блоках if и else одно и то же. После короткого обсуждения этой проблемы в рассылке и всего лишь одного переписывания патча код превратился в:

/*
 * Remove TXN from its containing list.
 *
 * Note: if txn->is_known_as_subxact, we are deleting the TXN from its
 * parent's list of known subxacts; this leaves the parent's nsubxacts
 * count too high, but we don't care.  Otherwise, we are deleting the TXN
 * from the LSN-ordered list of toplevel TXNs.
 */
dlist_delete(&txn->node);

Обсуждение: 20160404190345.54d84ee8@fujitsu
Коммит: b6182081be4a795d70b966be2542ad32d1ffbc20

2. Исправление двойной инициализации переменных


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

char	   *qual_value;
ParseState *qual_pstate = make_parsestate(NULL);

/* parsestate is built just to build the range table */
qual_pstate = make_parsestate(NULL);

Как видите, переменная инициализируется дважды, только напрасно грея процессор. Патч был принят моментально без особых вопросов.

Обсуждание: 20160316112019.64057481@fujitsu
Коммит: bd0ab28912d7502b237b8aeb95d052abe4ff6bc6

3. Исправление опечаток в комментариях


В любом достаточно крупном проекте присутствует изрядное количество опечаток. Найти их очень просто, включив проверку орфографии в вашей IDE или текстовом редакторе. Я лично пишу код в Vim. Для проверки орфографии в ~/.vimrc у меня есть строчки:

command! SpellOn :set spell spelllang=en_us,ru_ru
command! SpellOff :set nospell

Если кому-то интересно, то полная версия моего ~/.vimrc, ровно как и всех остальных конфигурационных файлов, доступны здесь.

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

Обсуждение: (что-то не удается найти)
Коммит: 2d8a1e22b109680204cb015a30e5a733a233ed64

4. Исправление двух идентичных комментариев


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

/*-----------
 * pgstat_progress_update_param() -
 *
 * Update index'th member in st_progress_param[] of own backend entry.
 *-----------
 */
void
pgstat_progress_update_param(int index, int64 val)
{
   volatile PgBackendStatus *beentry = MyBEEntry;

   Assert(index >= 0 && index < PGSTAT_NUM_PROGRESS_PARAM);

   if (!beentry || !pgstat_track_activities)
       return;

   pgstat_increment_changecount_before(beentry);
   beentry->st_progress_param[index] = val;
   pgstat_increment_changecount_after(beentry);
}

/*-----------
 * pgstat_progress_end_command() -
 *
 * Update index'th member in st_progress_param[] of own backend entry.
 *-----------
 */
void
pgstat_progress_end_command(void)
{

Можно заметить, что две разные процедуры имеют совершенно одинаковое описание, что явно какой-то косяк.

Обсуждение: 20160310120506.5007ea28@fujitsu
Коммит: 090b287fc59e7a44da8c3e0823eecdc8ea4522f2

5. Ворнинги при компиляции на FreeBSD


Большинство разработчиков PostgreSQL сидят под MacOS и Linux. Поэтому бывает полезно попытаться собрать проект на экзотике типа Microsoft Windows :) или FreeBSD. Используя этот прием, мне, например, удалось обнаружить, что PostgreSQL на FreeBSD собирается со следующими warning'ами:

pg_locale.c:1290:12: warning: implicit declaration of function
'wcstombs_l' is invalid in C99 [-Wimplicit-function-declaration]

result = wcstombs_l(to, from, tolen, locale);

pg_locale.c:1365:13: warning: implicit declaration of function
'mbstowcs_l' is invalid in C99 [-Wimplicit-function-declaration]

result = mbstowcs_l(to, str, tolen, locale);

2 warnings generated.

Исправить эту проблему оказалось не слишком сложно, хотя и потребовало повозиться с Autotools, что, по моему опыту, обычно не очень приятное занятие.

Обсуждение: 20160310163632.53d8e2cc@fujitsu
Коммит: 0e9b89986b7ced6daffdf14638a25a35c45423ff

Продолжение следует...


Как видите, чтобы начать контрибьютить в PostgreSQL, не требуется глубокого знания устройства реляционных баз данных или десяти лет опыта программирования на языке C. По большому счету, стать контрибьютором может любой человек, в теории — даже не умеющий программировать вообще. В этой части были рассмотрены, пожалуй, самые тривиальные патчи. В следующий раз мы рассмотрим патчи поинтереснее, решающие проблему lock contention, уменьшающие сложность алгоритма с O(N) до O(1), реализующие обход бинарных деревьев, чинящие утечки ресурсов, и не только.

Как всегда, я буду рад любым вашим комментариям и вопросам!

Продолжение: Примеры реальных патчей в PostgreSQL: часть 2 из N
Метки:
Postgres Professional 116,43
Российский вендор PostgreSQL
Поделиться публикацией
Похожие публикации
Комментарии 12
  • 0
    На фото: третье исправление сверху, это как вообще? Снова Tab VS. Space?
    Да и следующее Size*тпаолвптвыв*elementSize, что с этой строкой? Куда её ровняли?

    Вот так поступают те кто в недавнем топике кричали Space используют трУсы не умеющие настивать IDE под себя?
    • +1
      Надо спросить коммиттера (Tom Lane), но скорее всего это автоматическое исправление утилитой pgindent. Официально в проекте есть один правильный способ форматировать код — с ее помощью. Но иногда во время разработки код форматируют «на глазок», а потом перед релизом комиттеры прогоняют pgindent по всему коду.
      • 0
        Ясно, спасибо что разъяснили, буду знать про pgindent.
        Но вот здесь у меня тоже возник вопрос:
        | char *qual_value;
        | ParseState *qual_pstate = make_parsestate(NULL);
        |
        | /* parsestate is built just to build the range table */
        | qual_pstate = make_parsestate(NULL);

        пока не увидел в оригинале

        | @@ -1081,7 +1081,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
        | if (!attr_isnull)
        | {
        | char *qual_value;
        | — ParseState *qual_pstate = make_parsestate(NULL);
        | + ParseState *qual_pstate;
        |
        | /* parsestate is built just to build the range table */
        | qual_pstate = make_parsestate(NULL);
        | @@ -1122,7 +1122,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
        | if (!attr_isnull)
        | {
        | char *with_check_value;
        | — ParseState *with_check_pstate = make_parsestate(NULL);
        | + ParseState *with_check_pstate;
        |
        | /* parsestate is built just to build the range table */
        | with_check_pstate = make_parsestate(NULL);
        • 0
          Искренне извините за вертикальный слэш :(
          Это невозможно не то что читать, даже видеть: глаза выпадают.
          Впредь, никогда не буду так форматировать коммент!
    • 0
      Нередко опечатки появляются по той причине, что перед принятием патчей коммиттеры немного, совсем чуть-чуть, переписывают их. В результате получается совершенно новый код, который никто до этого не вычитывал.

      А разве нельзя сделать так, чтобы при подаче коммита делался хеш, затем, при принятии, делался хеш и после принятия, если хеши не совпали, об этом сразу появлялось уведомление и можно было бы посмотреть разность и варианты?

      • +3

        Почему автора не записывают в поле "author" в коммите? Обидно как-то.

        • 0
          Если честно, я даже не знаю, как это делается и возможно ли в git. Наверное просто потому что неудобно, проще в commit message указать. Плюс иной раз ревьюверы и тестировщики делают больше, чем сам автор.
          • +2

            Автор и коммитер в git — отдельные сущности. При коммите можно указать другого автора.
            Собственно это и было добавлено для того, что бы можно было указать истинного автора при пересылке патчей через почту.

            • +1
              и возможно ли в git

              так делается, например, в pgadmin-е.


              проще в commit message указать

              да, но иногда забывают. потом в соседнем письме в pgsql-commiters указывают автора)

              • +1
                так делается, например, в pgadmin-е.

                и в самом Git'е
          • 0
            Подскажите — есть ли встроенная функция, наподобие pg_reload_conf, или способ перезапускать службу СУБД из psql? (про "\!" я знаю, но хотелось бы какой-нибудь встроенный метод, не завязанный на расположение внешних файлов).

          Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

          Самое читаемое