-
Notifications
You must be signed in to change notification settings - Fork 55
Fix failed test #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix failed test #91
Conversation
It is necessary for avoiding output Query Identifier while vanille's test are running. (Look at more in explain.c:612. We can get fail condition if only query identifier is not null) Where clause 'NOT LIKE '%Query Identifier%'' is throwed away due to being necessary any more. This addition parameter is appeared if we set compute_query_id parameter with value as 'auto'. Appearance of the parameter is checked in only gucs test.
Delete platform dependent lines containing Memory and add order by command in feature_subspace test for statical result.
7466361 to
e474ecb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А зачем здесь тебе таки показывать Query Identifier ?
@MarinaPolyakova
MarinaPolyakova
Oct 26, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Видимо, проверяется, что он вообще выводится..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, делала проверку на вывод Query Identifier. Показалось логичным оставить это только в gucs тесте.
Специально добавила установку параметра compute_query_id в 'auto'.
В других тестах не добавляла и убрала Query Identifier из-за его ненужности там.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Видимо, проверяется, что он вообще выводится..
Ну, это достаточно слабое объяснение. Query Identifier вообще ванильная штука, чего его проверять? Я бы скорее сказал, что проверяются другие параметры экплейна, AQO-специфичные. А для них Identifier не нужен
@MarinaPolyakova
MarinaPolyakova
Oct 27, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тогда здесь можно убрать, как это сделано в остальных тестах?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А это никак не повлияет на другие тесты, выполняемые installcheck ? И почему?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У нас сейчас есть schedule-файл с порядком выполнения тестов. Почему бы не устанавливать это значение в первом тесте и не сбрасывать в последнем?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А это никак не повлияет на другие тесты, выполняемые installcheck ? И почему?
compute_query_id = 'regress' => не выводить query id, даже если он посчитался. Вроде тогда мы и другие тесты не ломаем, и функциональность по факту тестируется?.. (если что-то упадет при подсчете, то это будет заметно?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У нас сейчас есть schedule-файл с порядком выполнения тестов. Почему бы не устанавливать это значение в первом тесте и не сбрасывать в последнем?
Во-первых, compute_query_id = 'regress' в любом случае нужен, чтобы не сломался make installcheck в дереве исходников postgres с загруженным aqo. Как писала Алена, мы проверили, что pg_stat_statements от этого спасает только NO_INSTALLCHECK = 1 в contrib/pg_stat_statements/Makefile. aqo себе такого позволить не может.
Во-вторых, вроде каждый тест = отдельное соединение в psql? Т.е. переменная сбросится между тестами?..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У нас сейчас есть schedule-файл с порядком выполнения тестов. Почему бы не устанавливать это значение в первом тесте и не сбрасывать в последнем?
Не поможет. sheduller настроен на прогон aqo тестов и соответствующие настройки с параметрами, как понимаю, применяются к ним. У нас изначально возникла проблема с ванильными тестами.
Или ты говоришь про sheduller ванильных тестов? Но тогда, это пойдут изменения в ванильный код, чего мы хотим избежать
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А это никак не повлияет на другие тесты, выполняемые installcheck ? И почему?
Ты имеешь в виду другие расширения, которые работают одновременно с AQO и данная настройка их коснется или про тесты в AQO? Или вообще про другое? Можешь уточнить вопрос, пожалуйста.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А это никак не повлияет на другие тесты, выполняемые installcheck ? И почему?
compute_query_id = 'regress' => не выводить query id, даже если он посчитался. Вроде тогда мы и другие тесты не ломаем, и функциональность по факту тестируется?.. (если что-то упадет при подсчете, то это будет заметно?)
Ну так и сделайте это глобальным параметром своих тестов, если возможно, зачем это в открытое расширение складывать?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У нас сейчас есть schedule-файл с порядком выполнения тестов. Почему бы не устанавливать это значение в первом тесте и не сбрасывать в последнем?
Во-первых, compute_query_id = 'regress' в любом случае нужен, чтобы не сломался make installcheck в дереве исходников postgres с загруженным aqo. Как писала Алена, мы проверили, что pg_stat_statements от этого спасает только NO_INSTALL CHECK = 1 в contrib/pg_stat_statements/Makefile. aqo себе такого позволить не может.
AQO построен так же, как и pg_stat_statements. По крайней мере гитхабовская версия проекта желательно должна выглядеть традиционно. А патчи для различных энтерпрайзных версий можно и внутри хранить ...Во-вторых, вроде каждый тест = отдельное соединение в psql? Т.е. переменная сбросится между тестами?..
ALTER SYSTEM + pg_reload_conf() там не срабатывает?
@MarinaPolyakova
MarinaPolyakova
Oct 27, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ну так и сделайте это глобальным параметром своих тестов, если возможно, зачем это в открытое расширение складывать?
Потому что тесты падают только с загруженным расширением (в данном случае - aqo). Без вас все норм.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALTER SYSTEM + pg_reload_conf() там не срабатывает?
- pg_reload_conf вроде только отправляет сигнал postmaster? В смысле, надеюсь, в тестах не будет гонок..
- Если запустить паралельный installcheck-world (make -jN installcheck-world), с помощью ALTER SYSTEM вы повлияете на другие тесты?..
It turned out that vanilla regression tests failed using aqo_pg 14.patch in this version. This problem is related to running the Enable Query Id function in _PG_init(). We cannot remove the call to this function due to the launch of assert when query_id_enabled is disabled in query jumble. A similar function to enable query_id_enabled (enable query id) is used in pg_stat_statements, but the Makefile contains the addition of NO_INSTALL CHECK = 1 and does not run regression tests by default.
So, I have prepared changes for fix it.