Date: Fri, 19 Mar 2021 22:27:51 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Bruce Momjian <bruce@momjian.us>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Pavel Stehule <pavel.stehule@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Michael Paquier <michael@paquier.xyz>,
	torikoshia <torikoshia@oss.nttdata.com>,
	Atsushi Torikoshi <atorik@gmail.com>,
	Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	Evgeny Efimkin <efimkin@yandex-team.ru>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Feature improvement: can we add queryId for
 pg_catalog.pg_stat_activity view?
Message-ID: <20210319142751.vsvsjbp4bsvbh4ek@nol>
References: <20210317162444.GD16650@momjian.us>
 <20210317164907.c4uogoqvchnwcnjy@nol>
 <CA+Tgmoa5r2yxtuyHiYM3dVVEorr3vbVexYm9m+maMMiZtA25uA@mail.gmail.com>
 <20210317223216.GE16650@momjian.us>
 <20210317232956.veah6bax3mespdtq@nol>
 <20210318134729.GA8529@momjian.us>
 <20210318180656.3aat6jkbs7ylcg4k@nol>
 <20210318192349.GD3721@momjian.us>
 <20210319031650.aqy3ad7fpqsudo3b@nol>
 <20210319132906.GG3572@momjian.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210319132906.GG3572@momjian.us>
Archived-At: 
 <https://www.postgresql.org/message-id/20210319142751.vsvsjbp4bsvbh4ek%40nol>
Precedence: bulk

On Fri, Mar 19, 2021 at 09:29:06AM -0400, Bruce Momjian wrote:
> On Fri, Mar 19, 2021 at 11:16:50AM +0800, Julien Rouhaud wrote:
> > Now that I'm back on the code I remember why I did it this way.  It's
> > unfortunately not really possible to make things work this way.
> > 
> > pg_stat_statements' post_parse_analyze_hook relies on a queryid already being
> > computed, as it's where we know where the constants are recorded.  It means:
> > 
> > - we have to call post_parse_analyze_hook *after* doing core queryid
> >   calculation
> > - if users want to use a third party module to calculate a queryid, they'll
> >   have to make sure that the module's post_parse_analyze_hook is called
> >   *before* pg_stat_statements' one.
> > - even if they do so, they'll still have to pay the price of core queryid
> >   calculation
> 
> OK, that makes perfect sense.  I think the best solution is to document
> that compute_query_id just controls the built-in computation of the
> query id, and that extensions can also compute it if this is off, and
> pg_stat_activity and log_line_prefix will display built-in or extension
> computed query ids.

So the last version of the patch should implement that behavior right?  It's
just missing some explicit guidance that third-party extensions should only
calculate a queryid if compute_query_id is off

> It might be interesting someday to check if the hook changed a
> pre-computed query id and warn the user in the logs, but that could
> cause more log-spam problems than help.  I am a little worried that
> someone might have compute_query_id enabled and then install an
> extension that overwrites it, but we will just have to document this
> issue.  Hopefully extensions will be clear that they are computing their
> own query id.

I agree.  And hopefully they will split the queryid calculation from the rest
of the extension so that users can use the combination they want.