Это не все, что нужно изменить. Надо добавить новую переменную с текущей версией расширения, и собирать последний pg_variables-X.Y.sql при помощи cat в отдельном правиле.

@CherkashinSergey собирать последний pg_variables-1.1.sql не нужно. Сейчас так не делают. Достаточно сделать дельта файл pg_variables--1.0--1.1.sql. Для примера можешь посмотреть модуль adminpack в PostgreSQL master ветки. В общем тебе достаточно удалить файл pg_variables-1.1.sql.

@za-arthur это не будет работать со старыми версиями постгреса.

Да, точно. Тогда этот файл нужен.

Я думаю, генерация при помощи cat не является большой проблемой. Все-таки 9.5 и 9.6 довольно популярны, и способ будет совместим с будущими версиями. Для генерации оптимально оставить файл для 1.0 и конкатенировать его последовательно со всеми апдейтами. В таком случае свежая установка будет приводить у тому же результату, что и обновление.

@za-arthur правильно заметил, что контрибы по определению избавлены от этой проблемы.

В винде нет проблем с ним?

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

Хороший вопрос, у нашей сборки проблем не будет, потому что генератор Виктора умеет работать с таким правилом. Насчет ваниллы не уверен.

@vbwagner как сейчас принято решать эту задачу?

Этого файла не должно быть в репозитории, он должен генерироваться автоматически (добавить в .gitignore).

В функции clean_records() осуществляется удаление tuple descriptor, хэш-таблицы и memory context'а. Этот код можно улучить: хэш-таблица и tuple descriptor уже "живут" в этом контексте, поэтому достаточно просто удалить контекст.

Это позволит избавится от хрупкого кода удаления объектов в других местах, например, в release_savepoint() и clean_records():

if (variable->typid == RECORDOID)
{
	hash_destroy(historyEntryToDelete->value.record.rhash);
	FreeTupleDesc(historyEntryToDelete->value.record.tupdesc);
	/* Код выше будет не нужен */
	MemoryContextDelete(historyEntryToDelete->value.record.hctx);
}

Согласен. Можно. Но тут я действовал по принципу наименьшего вмешательства и оставил алгоритм таким, какой он был. Если считаешь, что надо всё-таки убрать лишние удаления - то давай.

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

Да вызывать MemoryContextDelete() только один раз намного логичнее.

Ну, в таком случае я ещё уберу вызов hash_sestroy() из remove_packages() и remove_package(). Это выглядит легальным.

@CherkashinSergey хорошая идея!

Возможно еще стоит его убрать из release_savepoint().

Да, само собой

Здесь особенно заметны проблемы с форматированием. При форматировании кода надо придерживаться стиля PostgreSQL. В разных местах проскакивают лишние пробелы и табуляция.

Здесь производится очистка через цикл + pfree(). В принципе, можно было бы разместить список в отдельном мемори контексте и при необходимости просто выполнять MemoryContextReset().

Зачем перемещен код для работы с mcxt?

Потому что внутри getVariableByNameWithTypeAndTrans() выделяется память с помощь palloc0(), но работы с контекстом там нет.

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

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

Я согласен, но вообще есть функция MemoryContextAllocZero(), которая позволяет напрямую аллоцировать из контекста. Впрочем, я ни на чем не настаиваю :)

Вообще они происходят довольно часто.

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

Написал хрень в комментарии, удалил, пишу заново :)

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

Дима тут оказывается уже написал про копирование переменных :)

Это я ещё в субботу сделал, просто не отписался. Извините.

Здесь не хватает typedef.

Сделал.

Тут можно добавить вывод pgv_get().

Думаю, тут стоит убрать сам вызов pgv_get(). По идее и так понятно, что в этот момент лежит в переменной.

Да, ты прав. Вызов pgv_set() тут никакого смысла не несет.

А вообще, тут MemoryContextSwitchTo() не нужен. Т.к. никакого выделения памяти не происходит, для pfree() он не нужен.

Тут MemoryContextSwitchTo() также не нужен, по-моему.

Да, ты прав. Вызов pgv_set() тут никакого смысла не несет.

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

Возможно еще стоит его убрать из release_savepoint().

Мне кажется, что эту функцию нужно объединить с getVariableByNameWithType(), которая по сути была скопирована с некоторыми изменениями.

Я давно ломаю голову над этими двумя функциями, но там не всё так просто: аргумент is_transactional известен не всегда. По сути, getVariableByNameWithTypeAndTrans() используется в функциях setter-ах (если их так можно назвать), а getVariableByNameWithType() - в getter-ах. В pgv_get() мы не знаем флаг is_transactional и он там и не нужен. По сути я могу вырезать строки из getVariableByNameWithType(), которые нужны для функции pgv_set(). Но там их не так уж и много.

Мне кажется, что нужно не стремиться еще сильнее разделить эти функции, а объединить в одну, и флаг транзакционности проверять только при модификации.

Я не уверен, что правильно понял. Но получается, что при каждом SAVEPOINT копируются все записи record переменной? Нет ли более оптимального способа? Иначе это может быть довольно медленным, если записей очень много.

Пока на ум приходит следующий способ. Я его не проверял и не уверен, что можно так делать. Это использование XID'ов в качестве ключа HashRecordKey, чтобы отличать записи разных транзакций.

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

Тут то же самое. Переменная еще не поменялась, а мы его уже копируем.

Переменную копируем до изменений, чтобы сами изменения вносить в новую версию точки сохранения. Можно было бы и не копировать старую версию, а просто сразу присваивать новое значение, но я использую эту функцию и для создания точки сохранения вне pgv_set(). В принципе, можно делать для каждой подтранзакции свой список изменённых переменных и работать уже с ним. Эта оптимизация имеет смысл только в случае большого количества подтранзакций и изменяемых переменных. Стоит делать?

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

В итоге, сделал.

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

Если "поддерживается, вот и всё", то она должна по-умолчанию работать. А тут не так.

Не согласен.

Текущая формулировка привлекает внимание к проблеме, которой на самом деле нет.

Хорошо, но мне кажется, что всё равно надо обозначить где-то вначале, что у переменных может быть разное поведение. Может сделать так:
Note that the module can either support and not support transactions and savepoints. For example:
Ну а дальше уже показать на примерах.

module can either support and not support transactions

Как раз этот текст меня и смущает. Модуль в любом случае будет поддерживать транзакционность, это пользователь может хотеть или не хотеть использовать данную фичу. Поддержка зависит от кода, а не от настроек.

Я предлагаю написать примерно так:

By default, created variables are not transactional (i.e. they are not affected by begin, commit or rollback statements). This, however, is customizable via the ... options of pgv_set()...

Это правило нужно переместить в конец Makefile, потому что оно срабатывает по дефолту первым, и сборка ломается.

@CherkashinSergey , похоже ты случайно удалил нужные иконки.