Re: Assert in pg_stat_statements

2015-08-10 0:04 GMT+09:00 Robert Haas <robertmhaas(at)gmail(dot)com>:
> 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.

I think so too, but unfortunately, I have to do that to work
with 9.4 and 9.5.

> 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?

Basically yes, because both modules should work not only together,
but also solely. So, my module need to have capability to calculate
queryId itself.

More precisely, if we have following configuration,

shared_preload_libraries = 'foo,pg_stat_statements'

my module "foo" calculates queryId itself, and pg_stat_statements
would do the same. (and gets crashed when it has --enable-cassert)

If we have following configuration,

shared_preload_libraries = 'pg_stat_statements,foo'

my module "foo" can skip queryId calculation because
pg_stat_statements has already done that.

Of course, my module "foo" should be able to work solely as following.

shared_preload_libraries = 'foo'

> 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.

Yes, that's exactly what I'm doing in my module, and what I intended
in my first patch for pg_stat_statements on this thread. :)

Regards,
--
NAGAYASU Satoshi <snaga(at)uptime(dot)jp>