>> This makes me wonder then if all it takes is just adding this to PortalDrop
>> (proposed earlier in the thread by Frédéric):
> One thing I did not like about that approach is that we will need to
> save the current debug_query_string inside PortalDrop before
> temporarily setting it to the one
> The "everything" also includes the query_text.
>
> This makes me wonder then if all it takes is just adding this to PortalDrop
> (proposed earlier in the thread by Frédéric):
One thing I did not like about that approach is that we will need to
save the current debug_query_string inside PortalDr
Hi,
On 18/09/2025 18:06, Sami Imseih wrote:
The "everything" also includes the query_text.
This makes me wonder then if all it takes is just adding this to
PortalDrop (proposed earlier in the thread by Frédéric):
if (portal->queryDesc)
debug_query_string = portal->queryDesc->sourceText;
On 16/09/2025 21:13, Sami Imseih wrote:
They can be committed together if there is agreement that tests are worth it.
One argument for keeping the tests would be that they nicely bring
together the side-effect (logging) and these use-cases, whereas in the
code they're pretty distant, making t
On 9/18/25 18:06, Sami Imseih wrote:
I also realized that the initial tests we've been working with included:
```
+log_statement = all
+log_min_duration_statement = 0
```
Yes, you added these two in your v8, together with a useless
log_line_prefix.
which caused additional noise and did
> Now, if I apply patch v11-0002 that only adds the tests but not
> v11-0001, the tests pass.
Ok. It looks like my regexp was wrong. v12-0002 fixes this by looking
for the correct STATEMENT: after the temp file logging.
I also realized that the initial tests we've been working with included:
```
Sorry for the late reply.
On 8/29/25 20:27, Sami Imseih wrote:
This version needs another rebase, but I don't think this is a proper solution
yet. It's dropping the portal in an area I don't think we should be concerned
with,
You might be right, I don't know... my understanding of the code isn
On Tue, Sep 16, 2025 at 02:13:39PM -0500, Sami Imseih wrote:
> Also, the tests should be checking that we are logging "temporary file: "
> before the next statement is logged.
>
> I split up the actual fix and the corrected tests into separate patches.
> They can be committed together if there is
On Wed, Sep 17, 2025 at 10:52:31AM -0500, Sami Imseih wrote:
> > One argument for keeping the tests would be that they nicely bring together
> > the side-effect (logging) and these use-cases, whereas in the code they're
> > pretty distant, making the connection not obvious.
>
> Another argument
> One argument for keeping the tests would be that they nicely bring together
> the side-effect (logging) and these use-cases, whereas in the code they're
> pretty distant, making the connection not obvious.
Another argument is that we have no coverage on temp logging, which
is why this bug was
> Just my 2c while looking at this particular part of the thread. Now
> to the main patch proposed, v8 or v10..
I have been thinking about whether test coverage is worth it for temp
file logging. I think it is, but others may disagree. However, I also
don't think the current tests are correct.
F
On Thu, Sep 11, 2025 at 10:28:50AM -0500, Sami Imseih wrote:
> > Only question is if we should avoid the extra portal hashtable lookup as
> > well, or leave that for a separate patch.
>
> I prefer a separate thread for this, as it's an optimization of the
> existing behavior.
- por
> Only question is if we should avoid the extra portal hashtable lookup as
> well, or leave that for a separate patch.
I prefer a separate thread for this, as it's an optimization of the
existing behavior.
--
Sami Imseih
Amazon Web Services (AWS)
Hi,
On 11/09/2025 16:28, Sami Imseih wrote:
I prefer a separate thread for this, as it's an optimization of the
existing behavior.
OK, I removed my changes to CreatePortal. I left the test description
fixes and added a proposed commit message.
If no objections I will set it to ready for com
Hi,
On 29/08/2025 19:27, Sami Imseih wrote:
Thoughts on v8?
I tried v8 with the Java file in the original email and it works (I
don't see the wrong query in the logs).
Small fix needed in the test descriptions: used to logged -> used to log.
Only question is if we should avoid the extra po
> On 8/21/25 14:02, Frédéric Yhuel wrote:
> > This v6 patch includes the TAP test that I sent in my previous email,
> > with some enhancements.
>
> The meson test system was overlooked by this patch, and the attached v7
> fixes that.
This version needs another rebase, but I don't think this is a p
On 8/21/25 14:02, Frédéric Yhuel wrote:
This v6 patch includes the TAP test that I sent in my previous email,
with some enhancements.
The meson test system was overlooked by this patch, and the attached v7
fixes that.From 554e8e62675574cbde0fd35b0a10f6ae56e5f0d6 Mon Sep 17 00:00:00 2001
From
On 4/26/25 20:57, Sami Imseih wrote:
I found several issues with v4. It does not deal correctly with pipelining,
and we should only really be concerned with dropping an unnamed
portal only.
So, v5 now moves the DropPortal call after the unnamed portal was
executed to completion ( as v4 was doi
On 4/26/25 20:57, Sami Imseih wrote:
I found several issues with v4. It does not deal correctly with pipelining,
and we should only really be concerned with dropping an unnamed
portal only.
So, v5 now moves the DropPortal call after the unnamed portal was
executed to completion ( as v4 was doi
I found several issues with v4. It does not deal correctly with pipelining,
and we should only really be concerned with dropping an unnamed
portal only.
So, v5 now moves the DropPortal call after the unnamed portal was
executed to completion ( as v4 was doing ), but does so only in the
case in whi
Thanks for testing. I also tested it a bit more today with other
patterns like different fetch sizes, named portal, etc. and I can't
find an issue with this, but I could be missing something.
I will go ahead and attach this change in patch form.
--
Sami Imseih
Amazon Web Services (AWS)
v4-Corre
On 4/23/25 18:13, Sami Imseih wrote:
Also, another strange behavior of the way portal cleanup occurs is that
in extended-query-protocol and within a transaction, ExecutorEnd for the
last query is not actually called until the next command. This just seems
odd to me especially for extensions th
> Yes, I think I had misunderstood what Tom said. Thank you for pointing
> that out.
>
> However, is it really unsafe?
I have not been able to find a case where this is unsafe, but
the documentation in mmgr/README does indicate that the
query string may live shorter than the portal in some cases.
On 4/23/25 09:41, Frédéric Yhuel wrote:
On 4/22/25 19:37, Sami Imseih wrote:
the patch relies on looking up queryDesc->sourceText inside DropPortal,
which Tom raised concerns about earlier in the thread [0]
Yes, I think I had misunderstood what Tom said. Thank you for pointing
that out.
On 4/22/25 19:37, Sami Imseih wrote:
the patch relies on looking up queryDesc->sourceText inside DropPortal,
which Tom raised concerns about earlier in the thread [0]
Yes, I think I had misunderstood what Tom said. Thank you for pointing
that out.
However, is it really unsafe?
In exec_bi
> So, It seems to me we are better off just setting debug_query_string
> to NULL in DropPortal, or alternatively why not just log the statement
> automatically at the start of execution whenever we have log_temp_files > 0.
> That will allow us to safely capture the statement to blame for the
> temp
> In any case, my v3 patch seems to fix all these cases.
>
> (I'm not saying it's good enough to be committed as is. I think I should
> at least add some comments. Anything else?)
the patch relies on looking up queryDesc->sourceText inside DropPortal,
which Tom raised concerns about earlier in the
On 4/21/25 07:46, Frédéric Yhuel wrote:
On 4/20/25 00:42, Sami Imseih wrote:
(In my testing, the "temporary file:" message comes
out without any attached STATEMENT most of the time already, so this
isn't losing much as far as that's concerned.)
Indeed, this happens when using autocommit /
On 4/21/25 07:46, Frédéric Yhuel wrote:
I can try to implement Tom's idea if we have a consensus.
v3 attached. Would that do?From 07d331ba0f91b999fcefd12696bfc1eda7e8f20f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Yhuel?=
Date: Fri, 18 Apr 2025 13:20:52 +0200
Subject: [PA
On 4/20/25 00:42, Sami Imseih wrote:
(In my testing, the "temporary file:" message comes
out without any attached STATEMENT most of the time already, so this
isn't losing much as far as that's concerned.)
Indeed, this happens when using autocommit / implicit transactions.
But if you disabl
Hi,
On 20/04/2025 00:42, Sami Imseih wrote:
[...]
I'm frankly inclined to do nothing, but if we must do something,
the way to fix it here would be to transiently set debug_query_string
to NULL so that the actions of PortalDrop aren't blamed on any
particular query.
I think this is better, bec
> Sami Imseih writes:
> > I think the solution proposed by Frédéric seems reasonable: to switch
> > the debug_query_string
> > inside PortalDrop. However, I do not like the way the
> > debug_query_string changes values
> > after the CreatePortal call inside exec_bind_message; that seems incorrect.
Sami Imseih writes:
> I think the solution proposed by Frédéric seems reasonable: to switch
> the debug_query_string
> inside PortalDrop. However, I do not like the way the
> debug_query_string changes values
> after the CreatePortal call inside exec_bind_message; that seems incorrect.
> So, I bel
I looked into this a bit more.
What occurs during exec_bind_message is that the debug_query_string of the
query is set early on. Later in that routine, a new portal is created
via CreatePortal,
which also drops the existing unnamed portal from the previous
execution and which
also logs the temp fi
> [...] LOG: temporary file: path "base/pgsql_tmp/pgsql_tmp525566.0",
> size 2416640
> [..] STATEMENT: SELECT 1
> but it should be:
> [...] LOG: temporary file: path "base/pgsql_tmp/pgsql_tmp538230.0",
> size 2416640
> [...] STATEMENT: SELECT * FROM foo ORDER BY a OFFSET $1 LIMIT 2
hmm, loo
On 4/18/25 10:49, Frédéric Yhuel wrote:
Hi,
It seems there's a bug in the logging of temporary file usage when the
extended protocol is used with unnamed portals.
FWIW, the attached patch seems to fix the problem.
Best regards,
FrédéricFrom afb228f07f847c467ba05dbe204861e7be2ffc32 Mon Se
Hi,
It seems there's a bug in the logging of temporary file usage when the
extended protocol is used with unnamed portals.
For example, with the attached Java / pgJDBC programs, we get the
following logs:
[...] LOG: temporary file: path "base/pgsql_tmp/pgsql_tmp525566.0",
size 2416640
[
37 matches
Mail list logo
