Re: Assert in pg_stat_statements

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> 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?

Not only that, but you'd need to keep the two modules in sync, which
would be one of the greatest recipes for bugs we've thought of yet.

If there's actually a use case for that sort of thing, I would vote
for moving the jumble-calculation code into core and making both of
the interested extensions do

/* Calculate query hash if it's not been done yet */
if (query->queryId == 0)
calculate_query_hash(query);

I note also that pg_stat_statements is using query->queryId == 0 as a
state flag, which means that if some other module has already calculated
the hash that would break it, even if the value were identical.
(In particular, it's using the event of calculating the queryId as an
opportunity to possibly make an entry in its own hashtable.) So you'd
need to do something to replace that logic if you'd like to use
pg_stat_statements concurrently with some other use of queryId.

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

Yeah, an explicit runtime check and elog() would be safer.

regards, tom lane