-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
autovacuum = off | ||
shared_preload_libraries = 'postgres_fdw, aqo' | ||
max_parallel_maintenance_workers = 1 # switch off parallel workers because of unsteadiness | ||
aqo.wide_search = 'on' | ||
aqo.wide_search = 'on' | ||
compute_query_id = 'regress' | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ $$ LANGUAGE PLPGSQL; | |
SET aqo.join_threshold = 0; | ||
SET aqo.mode = 'learn'; | ||
SET aqo.show_details = true; | ||
SET compute_query_id = 'auto'; | ||
CREATE TABLE t(x int); | ||
INSERT INTO t (x) (SELECT * FROM generate_series(1, 100) AS gs); | ||
ANALYZE t; | ||
|
@@ -20,33 +21,37 @@ SELECT true FROM aqo_reset(); -- Remember! DROP EXTENSION doesn't remove any AQO | |
(1 row) | ||
|
||
-- Check AQO addons to explain (the only stable data) | ||
SELECT str FROM expln(' | ||
SELECT regexp_replace( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. А зачем здесь тебе таки показывать Query Identifier ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Да, делала проверку на вывод Query Identifier. Показалось логичным оставить это только в gucs тесте. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ну, это достаточно слабое объяснение. Query Identifier вообще ванильная штука, чего его проверять? Я бы скорее сказал, что проверяются другие параметры экплейна, AQO-специфичные. А для них Identifier не нужен There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Тогда здесь можно убрать, как это сделано в остальных тестах? |
||
str,'Query Identifier: -?\m\d+\M','Query Identifier: N','g') as str FROM expln(' | ||
EXPLAIN (ANALYZE, VERBOSE, COSTS OFF, TIMING OFF, SUMMARY OFF) | ||
SELECT x FROM t; | ||
') AS str WHERE str NOT LIKE '%Query Identifier%'; | ||
') AS str; | ||
str | ||
------------------------------------------------ | ||
Seq Scan on public.t (actual rows=100 loops=1) | ||
AQO not used | ||
Output: x | ||
Query Identifier: N | ||
Using aqo: true | ||
AQO mode: LEARN | ||
JOINS: 0 | ||
(6 rows) | ||
(7 rows) | ||
|
||
SELECT str FROM expln(' | ||
SELECT regexp_replace( | ||
str,'Query Identifier: -?\m\d+\M','Query Identifier: N','g') as str FROM expln(' | ||
EXPLAIN (ANALYZE, VERBOSE, COSTS OFF, TIMING OFF, SUMMARY OFF) | ||
SELECT x FROM t; | ||
') AS str WHERE str NOT LIKE '%Query Identifier%'; | ||
') AS str; | ||
str | ||
------------------------------------------------ | ||
Seq Scan on public.t (actual rows=100 loops=1) | ||
AQO: rows=100, error=0% | ||
Output: x | ||
Query Identifier: N | ||
Using aqo: true | ||
AQO mode: LEARN | ||
JOINS: 0 | ||
(6 rows) | ||
(7 rows) | ||
|
||
SET aqo.mode = 'disabled'; | ||
-- Check existence of the interface functions. | ||
|
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-файл с порядком выполнения тестов. Почему бы не устанавливать это значение в первом тесте и не сбрасывать в последнем?
Uh oh!
There was an error while loading. Please reload this page.
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.
compute_query_id = 'regress' => не выводить query id, даже если он посчитался. Вроде тогда мы и другие тесты не ломаем, и функциональность по факту тестируется?.. (если что-то упадет при подсчете, то это будет заметно?)
Uh oh!
There was an error while loading. Please reload this page.
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.
Во-первых, 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.
Не поможет. 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.
Ты имеешь в виду другие расширения, которые работают одновременно с 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.
Ну так и сделайте это глобальным параметром своих тестов, если возможно, зачем это в открытое расширение складывать?
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.
Потому что тесты падают только с загруженным расширением (в данном случае - aqo). Без вас все норм.
Uh oh!
There was an error while loading. Please reload this page.
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.