Closed anse1 closed 6 years ago
rjuju
commented
6 years ago Hello,
Thanks a lot for the detailed report and the analysis! I can also reproduce this issue. I'll dig into this, since I don't see an obvious reason why no instrumentation is available here.
rjuju
commented
6 years ago Ok I understand the issue. FTR, the related statement is
DO $$
DECLARE
objtype text;
names text[];
args text[];
BEGIN
FOR objtype IN VALUES
('table'), ('index'), ('sequence'), ('view'),
('materialized view'), ('foreign table'),
('table column'), ('foreign table column'),
('aggregate'), ('function'), ('type'), ('cast'),
('table constraint'), ('domain constraint'), ('conversion'), ('default value'),
('operator'), ('operator class'), ('operator family'), ('rule'), ('trigger'),
('text search parser'), ('text search dictionary'),
('text search template'), ('text search configuration'),
('policy'), ('user mapping'), ('default acl'), ('transform'),
('operator of access method'), ('function of access method')
LOOP
FOR names IN VALUES ('{eins}'), ('{addr_nsp, zwei}'), ('{eins, zwei, drei}')
LOOP
FOR args IN VALUES ('{}'), ('{integer}')
LOOP
BEGIN
PERFORM pg_get_object_address(objtype, names, args);
EXCEPTION WHEN OTHERS THEN
RAISE WARNING 'error for %,%,%: %', objtype, names, args, sqlerrm;
END;
END LOOP;
END LOOP;
END LOOP;
END;
$$;I'm not familiar with plpgsql internals, but IIUC with such nested FOR loop, multiple executor are started in a nested way. Therefore, each executor start will be seen as a nested_level of 0, so query_is_sampled will be set independantly. The problem is that query_is_sampled is a global variable.
So if the first executor isn't sampled but the second is, query_is_sampled will be set to true, and the first call to pgqs_ExecutorEnd will think that the query was sampled while it wasn't, try to read the instrument field and happily fail.
I don't really see an easy way to fix it. We could of course add an if (planstate->instrument) in pgqs_ExecutorEnd to ignore "false positive sampled queries", but we'd still miss the opposite case. Also, we'd need to make sure it wasn't something else, like auto_explain adding some instrumentations.
The easiet and safest solution I can imagine would be to save the query_is_sampled information in the QueryDesc, but after a quick look I don't see any custom field available nearby.
I guess we could track start/stop sequences and keep the same sampled state for any inner executor, but if any error happens, executor end won't be called, which means the backend will be forever stuck in either sampled state :(
Do you have any idea on a proper fix?
rjuju
commented
6 years ago Hi @anse1, sorry for late answer.
I couldn't find an easy solution to fix this issue, so I just added a test on the effective instrumentation to avoid the "falise positive" case. The point of sampling is to avoid overhead with highly loaded servers, so missing some queries in such cases shouldn't make any difference.
Fix pushed in ae73c10b23f6a976cba3783c2fe9a2c1d52ed6cc, thanks again for the report!
Hi,
running the PostgreSQL 9.6.5 regression tests on a database with powa + pg_qualstats installed has a 30% chance of crashing for me while running the object_address test. Backtrace below.
regards, andreas