Re: Assert in pg_stat_statements

On Sun, Aug 9, 2015 at 1:36 AM, Satoshi Nagayasu <snaga(at)uptime(dot)jp> wrote:
> On 2015/08/08 22:32, Robert Haas wrote:
>> On Sat, Aug 8, 2015 at 5:00 AM, Satoshi Nagayasu <snaga(at)uptime(dot)jp> wrote:
>>>
>>> I just found that pg_stat_statements causes assert when queryId is
>>> set by other module, which is loaded prior to pg_stat_statements in
>>> the shared_preload_libraries parameter.
>>>
>>> Theoretically, queryId in the shared memory could be set by other
>>> module by design.
>>>
>>> So, IMHO, pg_stat_statements should not cause assert even if queryId
>>> already has non-zero value --- my module has this problem now.
>>>
>>> Is my understanding correct? Any comments?
>>
>>
>> Seems like an ERROR would be more appropriate than an Assert.
>
>
> Well, it's not that simple, and I'm afraid that it may not fix
> the issue.
>
> Here, let's assume that we have two modules (foo and pg_stat_statements)
> which calculate, use and store Query->queryId independently.
>
> What we may see when two are enabled is following.
>
> (1) Enable two modules in shared_preload_libraries.
>
> shared_preload_libraries = 'foo,pg_stat_statements'
>
> (2) Once a query comes in, a callback function in module "foo"
> associated with post_parse_analyze_hook calculates and store
> queryId in Query->queryId.
>
> (3) After that, a callback function (pgss_post_parse_analyze) in
> "pg_stat_statements" associated with post_parse_analyze_hook
> detects that Query->queryId has non-zero value, and asserts it.
>
> As a result, we can not have two or more modules that use queryId
> at the same time.
>
> But I think it should be possible because one of the advantages of
> PostgreSQL is its extensibility.
>
> So, I think the problem here is the fact that pg_stat_statements
> deals with having non-zero value in queryId as "error" even if
> it has exact the same value with other module.

I'm not too excited about supporting the use case where there are two
people using queryId but it just so happens that they always set
exactly the same value. That seems like a weird setup. Wouldn't that
mean both modules were applying the same jumble algorithm, and
therefore you're doing the work of computing the jumble twice per
query? It would be better to have the second module have an option
not to compute the query ID and just do whatever else it does. Then,
if you want to use that other module with pg_stat_statements, flip the
GUC.

The reason I think an Assert is wrong is that if there are other
modules using queryId, we should catch attempts to use the queryId for
more than one purpose even in non-assert enabled builds.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company