On Wed, Sep 18, 2024 at 03:14:07PM -0500, Sami Imseih wrote:
> Agree, will do start a new thread.
Thanks.
--
Michael
signature.asc
Description: PGP signature
> By the way, with the main issue fixed as of 933848d16dc9, could it be
> possible to deal with the plan cache part in a separate thread? This
> is from the start a separate thread to me, and we've done quite a bit
> here already.
Agree, will do start a new thread.
--
Sami
> So, I have applied 0001 down to 14, followed by 0002 on HEAD.
Thank you!
> 0002 is going to be interesting to see moving forward. I am wondering
> how existing out-of-core extensions will react on that and if it will
> help catching up any issues. So, let's see how the experiment goes
> with HE
On Wed, Sep 18, 2024 at 09:38:32AM +0900, Michael Paquier wrote:
> FWIW, I was thinking about something like what has been done in
> indexcmds.c for 5bbdfa8a18dc as the query ID value is not predictible
> across releases, but we could see whether it is set or not.
By the way, with the main issue f
On Wed, Sep 18, 2024 at 07:50:27AM +0900, Michael Paquier wrote:
> On Tue, Sep 17, 2024 at 05:01:18PM -0500, Sami Imseih wrote:
> > > Then, please see attached two lightly-updated patches. 0001 is for a
> > > backpatch down to v14. This is yours to force things in the exec and
> > > bind messages f
On Tue, Sep 17, 2024 at 06:39:17PM -0500, Sami Imseih wrote:
> FWIW, I do like the INJECTION_POINT idea and actually mentioned something
> similar up the thread [1] for the revalidate cache case, but I can see it
> being applied
> to all the other places we expect the queryId to be set.
>
> [1]
> would help to grab a query ID. A second option I have in mind would
> be to set up an injection point that produces a NOTICE if a query ID
> is set when we end processing an execute message, then check the
> number of NOTICE messages produced as these can be predictible
> depending on the number
On Tue, Sep 17, 2024 at 05:01:18PM -0500, Sami Imseih wrote:
> > Then, please see attached two lightly-updated patches. 0001 is for a
> > backpatch down to v14. This is yours to force things in the exec and
> > bind messages for all portal types, with the test (placed elsewhere in
> > 14~15 branche
> Then, please see attached two lightly-updated patches. 0001 is for a
> backpatch down to v14. This is yours to force things in the exec and
> bind messages for all portal types, with the test (placed elsewhere in
> 14~15 branches). 0002 is for HEAD to add some sanity checks, blowing
> up the test
On Thu, Sep 12, 2024 at 03:58:27PM -0500, Sami Imseih wrote:
> Yes, adding the asserts in execMain.c is better, but there is complications
> there due to the issue you mention. I think the issue you are bumping into
> is when pg_stat_statements.track_utility = on ( default ), the assert in
> Execu
> Do you think that we'd better replace the calls reporting the query ID
> in execMain.c by some assertions on HEAD? This won't work for
> ExecutorStart() because PREPARE statements (or actually EXECUTE,
> e.g. I bumped on that yesterday but I don't recall which one) would
Yes, adding the asserts
On Wed, Sep 11, 2024 at 09:41:58PM -0500, Sami Imseih wrote:
>> The tests in pg_stat_statements are one part I'm pretty sure is one
>> good way forward. It is not perfect, but with the psql meta-commands
>
> I played around with BackgrounsPsql. It works and gives us more flexibility
> in testing,
> After sleeping on it, I'd tend to slightly favor the last option in
> the back-branches and the second option on HEAD where we reduce the
> number of report calls. This way, we are a bit more careful in
>released branches by being more aggressive in reporting the query ID.
I agree with this beca
On Wed, Sep 11, 2024 at 05:02:07PM -0500, Sami Imseih wrote:
> In your 0003-Report-query-ID-for-execute-fetch-in-extended-query-.patch
> patch, you are still setting the queryId inside exec_execute_message
> if (execute_is_fetch). This condition could be removed and don't need to set
> the query
I took a look at your patches and here are my comments
> 1) ExecutorRun() misses the reports, which happens when a query
> does an ExecutorStart(), then a series of ExecutorRun() through a
> portal with bind messages. Robert has mentioned that separately a few
> days ago at [1]. But that's not e
On Mon, Sep 02, 2024 at 10:11:43AM +0900, Michael Paquier wrote:
> I need to spend a bit more time with my head down for this thread, but
> couldn't we use these commands with various query patterns in
> pg_stat_statements and look at the shmem counters reported through its
> view?
My apologies fo
On Mon, Sep 09, 2024 at 06:20:01PM -0500, Sami Imseih wrote:
> On 14/8/2024 23:05, Imseih (AWS), Sami wrote:
>> Also, while writing the test, I found out that now, JumbleQuery takes
>> into account constants of the A_Const node, and calls of the same
>> prepared statement with different parameter
> > >> I think the testing discussion should be moved to a different thread.
> > >> What do you think?
> > > See v4.
> > >
> > > 0001 deals with reporting queryId in exec_execute_message and
> > > exec_bind_message.
> > > 0002 deals with reporting queryId after a cache invalidation.
> > >
> > >
> >> I think the testing discussion should be moved to a different thread.
> >> What do you think?
> > See v4.
> >
> > 0001 deals with reporting queryId in exec_execute_message and
> > exec_bind_message.
> > 0002 deals with reporting queryId after a cache invalidation.
> >
> > There are no tests
Sorry for the late reply on this thread.
On 14/8/2024 23:05, Imseih (AWS), Sami wrote:
> There are no tests as this requires more discussion in a separate thread(?)
> Unfortunately, TAP tests don't allow us to keep a connection and
> manually permutate the order of queries sent to different conne
On 14/8/2024 23:05, Imseih (AWS), Sami wrote:
There are no tests as this requires more discussion in a separate thread(?)
Unfortunately, TAP tests don't allow us to keep a connection and
manually permutate the order of queries sent to different connections.
But isolation tests are designed to d
On 14/8/2024 23:05, Imseih (AWS), Sami wrote:
I think the testing discussion should be moved to a different thread.
What do you think?
See v4.
0001 deals with reporting queryId in exec_execute_message and
exec_bind_message.
0002 deals with reporting queryId after a cache invalidation.
There
On Sat, Aug 31, 2024 at 09:47:41AM +0800, jian he wrote:
> /* test \bind queryid exists */
> select query_id is not null as query_id_exist
> from pg_stat_activity where pid = pg_backend_pid() \bind \g
>
> /* test that \parse \bind_named queryid exists */
> select pg_backend_pid() as current_pid \g
On Thu, Aug 15, 2024 at 5:06 AM Imseih (AWS), Sami wrote:
>
> > I think the testing discussion should be moved to a different thread.
> > What do you think?
> See v4.
>
> 0001 deals with reporting queryId in exec_execute_message and
> exec_bind_message.
> 0002 deals with reporting queryId after a
I think the testing discussion should be moved to a different thread.
What do you think?
See v4.
0001 deals with reporting queryId in exec_execute_message and
exec_bind_message.
0002 deals with reporting queryId after a cache invalidation.
There are no tests as this requires more discussion i
Sounds fine by me (still need to check all three patterns).
+ if (list_length(psrc->query_list) > 0)
+ pgstat_report_query_id(linitial_node(Query, psrc->query_list)->queryId,
false);
Something that slightly worries me is to assume that the first Query
in the query_list is fetched. Usin
On Fri, Jul 26, 2024 at 02:39:41PM +0200, Anthonin Bonnefoy wrote:
> For the tests, there are limited possibilities to check whether a
> query_id has been set correctly.
> - Checking pg_stat_activity is not possible in the regress tests as
> you need a second session to check the reported query_id.
On Thu, Jul 18, 2024 at 10:56 AM Michael Paquier wrote:
> Please attach things to your emails, if your repository disappears for
> a reason or another we would lose knowledge in the archives of the
> community lists.
Noted and thanks for the reminder, I'm still learning about mailing
list etiquet
On Tue, Jul 23, 2024 at 04:00:25PM -0500, Sami Imseih wrote:
> Correct, I also don´t think ExecutorRun is enough. Another reason is we
> should also
> be setting the queryId during bind, right before planning starts.
> Planning could have significant impact on the server and I think we better
>
> On Wed, Jul 17, 2024 at 11:32:49AM +0200, Anthonin Bonnefoy wrote:
>> Wouldn't it be enough to call pgstat_report_query_id in ExecutorRun
>> and ProcessUtility? With those changes [1], both normal statements and
>> utility statements called through extended protocol will correctly
>> report the
On Wed, Jul 17, 2024 at 11:32:49AM +0200, Anthonin Bonnefoy wrote:
> Wouldn't it be enough to call pgstat_report_query_id in ExecutorRun
> and ProcessUtility? With those changes [1], both normal statements and
> utility statements called through extended protocol will correctly
> report the query_i
Hi,
Wouldn't it be enough to call pgstat_report_query_id in ExecutorRun
and ProcessUtility? With those changes [1], both normal statements and
utility statements called through extended protocol will correctly
report the query_id.
-- Test utility statement with extended protocol
show all \bind \g
> I'm unsure if it needs to call ExecutorStart in the bind code. But if we
> don't change the current logic, would it make more sense to move
> pgstat_report_query_id to the ExecutorRun routine?
I initially thought about that, but for utility statements (CTAS, etc.) being
executed with extended q
On 15.05.2024 10:24, Imseih (AWS), Sami wrote:
I discovered the current state of queryId reporting and found that it
may be unlogical: Postgres resets queryId right before query execution
in simple protocol and doesn't reset it at all in extended protocol and
other ways to execute queries.
In e
On Wed, May 15, 2024 at 06:36:23PM +, Imseih (AWS), Sami wrote:
>> Okay, that's what I precisely wanted to understand: queryId doesn't have
>> semantics to show the job that consumes resources right now—it is mostly
>> about convenience to know that the backend processes nothing except
>> (prob
> Okay, that's what I precisely wanted to understand: queryId doesn't have
> semantics to show the job that consumes resources right now—it is mostly
> about convenience to know that the backend processes nothing except
> (probably) this query.
It may be a good idea to expose in pg_stat_activity o
On 15/5/2024 12:09, Michael Paquier wrote:
On Wed, May 15, 2024 at 03:24:05AM +, Imseih (AWS), Sami wrote:
I think we should generally report it when the backend executes a job
related to the query with that queryId. This means it would reset the
queryId at the end of the query execution.
On Wed, May 15, 2024 at 03:24:05AM +, Imseih (AWS), Sami wrote:
>> I think we should generally report it when the backend executes a job
>> related to the query with that queryId. This means it would reset the
>> queryId at the end of the query execution.
>
> When the query completes execution
> I discovered the current state of queryId reporting and found that it
> may be unlogical: Postgres resets queryId right before query execution
> in simple protocol and doesn't reset it at all in extended protocol and
> other ways to execute queries.
In exec_parse_message, exec_bind_message and
On 5/1/24 10:07, Imseih (AWS), Sami wrote:
Here is a new rev of the patch which deals with the scenario
mentioned by Andrei [1] in which the queryId may change
due to a cached query invalidation.
[1]
https://www.postgresql.org/message-id/724348C9-8023-41BC-895E-80634E79A538%40amazon.com
I disc
Here is a new rev of the patch which deals with the scenario
mentioned by Andrei [1] in which the queryId may change
due to a cached query invalidation.
[1]
https://www.postgresql.org/message-id/724348C9-8023-41BC-895E-80634E79A538%40amazon.com
Regards,
Sami
0001-v2-Fix-Extended-Query-Proto
On 27/4/2024 20:54, Imseih (AWS), Sami wrote:
But simplistic case with a prepared statement shows how the value of
queryId can be changed if you don't acquire all the objects needed for
the execution:
CREATE TABLE test();
PREPARE name AS SELECT * FROM test;
EXPLAIN (ANALYSE, VERBOSE, COSTS OF
>> But simplistic case with a prepared statement shows how the value of
>> queryId can be changed if you don't acquire all the objects needed for
>> the execution:
>> CREATE TABLE test();
>> PREPARE name AS SELECT * FROM test;
>> EXPLAIN (ANALYSE, VERBOSE, COSTS OFF) EXECUTE name;
>> DROP TABLE
> We choose a arguably more user-friendly option:
> https://www.postgresql.org/docs/current/sql-prepare.html
Thanks for pointing this out!
Regards,
Sami
On Sat, Apr 27, 2024 at 6:55 AM Imseih (AWS), Sami
wrote:
>
> Hmm, you raise a good point. Isn't this a fundamental problem
> with prepared statements? If there is DDL on the
> relations of the prepared statement query, shouldn't the prepared
> statement be considered invalid at that point and ra
> But simplistic case with a prepared statement shows how the value of
> queryId can be changed if you don't acquire all the objects needed for
> the execution:
> CREATE TABLE test();
> PREPARE name AS SELECT * FROM test;
> EXPLAIN (ANALYSE, VERBOSE, COSTS OFF) EXECUTE name;
> DROP TABLE test;
>
> I am also a bit surprised with the choice of using the first Query
> available in the list for the ID, FWIW.
IIUC, the query trees returned from QueryRewrite
will all have the same queryId, so it appears valid to
use the queryId from the first tree in the list. Right?
Here is an example I was
On 4/23/24 12:49, Michael Paquier wrote:
On Tue, Apr 23, 2024 at 11:42:41AM +0700, Andrei Lepikhov wrote:
On 23/4/2024 11:16, Imseih (AWS), Sami wrote:
+ pgstat_report_query_id(linitial_node(Query, psrc->query_list)->queryId,
true);
set_ps_display("BIND");
@@ -2146,6 +2147,7 @@
On Tue, Apr 23, 2024 at 11:42:41AM +0700, Andrei Lepikhov wrote:
> On 23/4/2024 11:16, Imseih (AWS), Sami wrote:
>> + pgstat_report_query_id(linitial_node(Query,
>> psrc->query_list)->queryId, true);
>> set_ps_display("BIND");
>> @@ -2146,6 +2147,7 @@ exec_execute_message(const char
On 23/4/2024 11:16, Imseih (AWS), Sami wrote:
FWIW, I'd like to think that we could improve the situation, requiring
a mix of calling pgstat_report_query_id() while feeding on some query
IDs retrieved from CachedPlanSource->query_list. I have not in
details looked at how much could be achieved, T
> FWIW, I'd like to think that we could improve the situation, requiring
> a mix of calling pgstat_report_query_id() while feeding on some query
> IDs retrieved from CachedPlanSource->query_list. I have not in
> details looked at how much could be achieved, TBH.
I was dealing with this today and f
>
>
>>
>> FWIW, I'd like to think that we could improve the situation, requiring
>> a mix of calling pgstat_report_query_id() while feeding on some query
>> IDs retrieved from CachedPlanSource->query_list. I have not in
>> details looked at how much could be achieved, TBH.
>>
>
This just cropped u
Thnx.
br
Kaido
On Tue, 13 Jun 2023 at 03:16, Michael Paquier wrote:
> On Mon, Jun 12, 2023 at 09:03:24PM +0300, kaido vaikla wrote:
> > I have noticed, if query comes from PostgreSQL JDBC Driver, then query_id
> > is not present in pg_stat_activity. Erik Wienhold figured out that
> reason
> > c
On Mon, Jun 12, 2023 at 09:03:24PM +0300, kaido vaikla wrote:
> I have noticed, if query comes from PostgreSQL JDBC Driver, then query_id
> is not present in pg_stat_activity. Erik Wienhold figured out that reason
> can be in extended query protocol (
> https://www.postgresql.org/message-id/139161
I have noticed, if query comes from PostgreSQL JDBC Driver, then query_id
is not present in pg_stat_activity. Erik Wienhold figured out that reason
can be in extended query protocol (
https://www.postgresql.org/message-id/1391613709.939460.1684777418...@office.mailbox.org
)
My question is, is it e
55 matches
Mail list logo
