> From this discussion, there is desire for a sleep function that:
> 1/ Sleeps for the full duration of the requested time
> 2/ Continues to handle important interrupts during the sleep
>
> While something like CF 5118 will take care of point #1,
> I'm not sure, even with CF entry 5118, nanoslee
Hi,
On Tue, Aug 20, 2024 at 02:25:19PM -0500, Sami Imseih wrote:
> > As it looks like we have a consensus that reducing the number of interrupts
> > also
> > makes sense, I just provided a rebase version of the 1 Hz version (see [0],
> > that
> > also makes clear in the doc that the new field m
> As it looks like we have a consensus that reducing the number of interrupts
> also
> makes sense, I just provided a rebase version of the 1 Hz version (see [0],
> that
> also makes clear in the doc that the new field might show slightly old
> values).
That makes sense. However, I suspect the
Hi,
On Tue, Aug 13, 2024 at 11:40:27AM -0500, Nathan Bossart wrote:
> On Tue, Aug 13, 2024 at 11:07:46AM -0500, Imseih (AWS), Sami wrote:
> > Having to add special handling to space out instrumentation
> > directly in vacuum_delay_point seems very odd to me. I don't
> > think vacuum_delay_point sh
Hi,
On Thu, Aug 15, 2024 at 04:13:29PM -0500, Nathan Bossart wrote:
> On Wed, Aug 14, 2024 at 06:00:06AM +, Bertrand Drouvot wrote:
> > I gave it more thoughts and I don't think we have to choose between the two.
> > The 1 Hz approach reduces the number of interrupts and Sami's patch
> > prov
On Sun, Aug 18, 2024 at 11:12 AM Thomas Munro wrote:
> I guess both of these issues go away in practice if
> CF #5118 goes in.
To be more precise, if you just keep doing pg_usleep() the issue goes
away, and likewise for posix_fallocate() it goes away... But if you
switch to WaitLatchUs() so you
On Wed, Aug 14, 2024 at 9:30 AM Nathan Bossart wrote:
> Another concern is the huge number of PqMsg_Progress messages sent by
> parallel workers with that approach. In Bertrand's tests, he was seeing
> nearly 350K interrupts for a ~19 minute vacuum (~300 interrupts per
> second). That seems a bi
> time. I wouldn't say I'm diametrically opposed to this patch, but I do
> think we need to carefully consider whether it's worth the extra code.
FWIW, besides the patch that Bertrand is proposing [1], there is another
parallel
vacuum case being discussed to allow for parallel heap scan [2].
Be
On Wed, Aug 14, 2024 at 06:00:06AM +, Bertrand Drouvot wrote:
> I gave it more thoughts and I don't think we have to choose between the two.
> The 1 Hz approach reduces the number of interrupts and Sami's patch provides a
> way to get "accurate" delay in case of interrupts. I think both have th
Hi,
On Tue, Aug 13, 2024 at 04:30:46PM -0500, Nathan Bossart wrote:
> On Tue, Aug 13, 2024 at 01:12:30PM -0500, Imseih (AWS), Sami wrote:
> >> None of this seems intractable to me. 1 Hz seems like an entirely
> >> reasonable place to start, and it is very easy to change (or to even make
> >> conf
On Tue, Aug 13, 2024 at 4:30 PM Nathan Bossart
wrote:
> On Tue, Aug 13, 2024 at 01:12:30PM -0500, Imseih (AWS), Sami wrote:
> >> None of this seems intractable to me. 1 Hz seems like an entirely
> >> reasonable place to start, and it is very easy to change (or to even
> make
> >> configurable).
On Tue, Aug 13, 2024 at 01:12:30PM -0500, Imseih (AWS), Sami wrote:
>> None of this seems intractable to me. 1 Hz seems like an entirely
>> reasonable place to start, and it is very easy to change (or to even make
>> configurable). pg_stat_progress_vacuum might show slightly old values in
>> this
None of this seems intractable to me. 1 Hz seems like an entirely
reasonable place to start, and it is very easy to change (or to even make
configurable). pg_stat_progress_vacuum might show slightly old values in
this column, but that should be easy enough to explain in the docs if we
are really
On Tue, Aug 13, 2024 at 11:07:46AM -0500, Imseih (AWS), Sami wrote:
> Having to add special handling to space out instrumentation
> directly in vacuum_delay_point seems very odd to me. I don't
> think vacuum_delay_point should have to worry about this.
>
> Also,
> 1/ what is an appropriate interva
Please disregards this point from the last reply:
"""
2/ What if there are other callers in the future that wish
to instrument parallel vacuum workers? they will need to implement
similar logic.
"""
I misspoke about this and this point does not matter since only
vacuum sleep matters for this
On 8/13/24 10:57 AM, Nathan Bossart wrote:
On Tue, Aug 13, 2024 at 10:47:51AM -0500, Imseih (AWS), Sami wrote:
On 8/13/24 10:09 AM, Nathan Bossart wrote:
On Mon, Aug 12, 2024 at 05:35:08PM -0500, Imseih (AWS), Sami wrote:
Skimming the last few messages of that thread [0], it looks like Bertr
On Tue, Aug 13, 2024 at 10:47:51AM -0500, Imseih (AWS), Sami wrote:
> On 8/13/24 10:09 AM, Nathan Bossart wrote:
>> On Mon, Aug 12, 2024 at 05:35:08PM -0500, Imseih (AWS), Sami wrote:
>> > > Skimming the last few messages of that thread [0], it looks like Bertrand
>> > > is exploring ways to avoid
On 8/13/24 10:09 AM, Nathan Bossart wrote:
On Mon, Aug 12, 2024 at 05:35:08PM -0500, Imseih (AWS), Sami wrote:
Skimming the last few messages of that thread [0], it looks like Bertrand
is exploring ways to avoid so many interrupts. I guess the unavoidable
question is whether this work is stil
On Mon, Aug 12, 2024 at 05:35:08PM -0500, Imseih (AWS), Sami wrote:
>> Skimming the last few messages of that thread [0], it looks like Bertrand
>> is exploring ways to avoid so many interrupts. I guess the unavoidable
>> question is whether this work is still worthwhile given that improvement.
>
Hi,
On Mon, Aug 12, 2024 at 05:04:02PM -0500, Nathan Bossart wrote:
> On Tue, Aug 13, 2024 at 12:04:28AM +0300, Heikki Linnakangas wrote:
> >> In the case of the patch being proposed by Bertrand, the number of
> >> interrupts > will be much more frequent as parallel workers would send a
> >> messa
Skimming the last few messages of that thread [0], it looks like Bertrand
is exploring ways to avoid so many interrupts. I guess the unavoidable
question is whether this work is still worthwhile given that improvement.
The way the instrumentation in [0] dealt with interrupts was too complex,
On Tue, Aug 13, 2024 at 12:04:28AM +0300, Heikki Linnakangas wrote:
>> In the case of the patch being proposed by Bertrand, the number of
>> interrupts > will be much more frequent as parallel workers would send a
>> message
> to the leader
>> to update the vacuum delay counters every vacuum_delay_
I'm trying to understand what the point of this patch is, so I went to
read this thread from the beginning:
In the proposal by Bertrand [1] to implement vacuum cost delay tracking
in pg_stat_progress_vacuum, it was discovered that the vacuum cost delay
ends early on the leader process of a para
As cfbot points out [0], this is missing a Windows/frontend implementation.
[0] https://cirrus-ci.com/task/6393555868975104
Looks like the pgsleep implementation is missing the
#ifndef WIN32
I followed what is done in pg_usleep.
v11 should now build on Windows, hopefully.
Strangely, v10 buil
On Mon, Aug 12, 2024 at 03:56:18PM +, Bertrand Drouvot wrote:
> Thanks, v10 LGTM.
As cfbot points out [0], this is missing a Windows/frontend implementation.
[0] https://cirrus-ci.com/task/6393555868975104
--
nathan
Hi,
On Mon, Aug 12, 2024 at 10:19:56AM -0500, Sami Imseih wrote:
> v10 attached in the previous message addresses
> Bertrands last comment and replaces “This” with “this"
>
Thanks, v10 LGTM.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Servic
My email client attached the last response for
some reason :(
v10 attached in the previous message addresses
Bertrands last comment and replaces “This” with “this"
Regards,
Sami
>
> + * Unlike pg_usleep, This function continues
>
> s/This/this/?
>
> Apart from the above, LGTM.
>
v10-0001-vaccum_delay-with-absolute-time-nanosleep.patch
Description: Binary data
Thanks for this catch. Uploaded v10.
Regards,
Sami
Hi,
On Sat, Aug 10, 2024 at 08:27:56AM -0500, Sami Imseih wrote:
>
>
> v9 has some has some minor corrections to the comments.
>
Thanks!
1 ===
+ * Unlike pg_usleep, This function continues
s/This/this/?
Apart from the above, LGTM.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Te
v9-0001-vaccum_delay-with-absolute-time-nanosleep.patch
Description: Binary data
v9 has some has some minor corrections to the comments.
Regards,
Sami
v8-0001-vaccum_delay-with-absolute-time-nanosleep.patch
Description: Binary data
>
> Yeah, I had the same thought in [1], so +1.
>
> [1]:
> https://www.postgresql.org/message-id/ZpDhS4nFX66ItAze%40ip-10-97-1-34.eu-west-3.compute.internal
>
The intention ( see start of the thread ) was to ma
Hi,
On Fri, Aug 09, 2024 at 02:03:55PM -0500, Nathan Bossart wrote:
> On Thu, Aug 08, 2024 at 03:01:20PM -0500, Nathan Bossart wrote:
> > Thanks. This one looks pretty good to me, and so I plan to commit it in
> > the near future unless anyone voices concerns about the approach.
>
> As I am prep
On Thu, Aug 08, 2024 at 03:01:20PM -0500, Nathan Bossart wrote:
> Thanks. This one looks pretty good to me, and so I plan to commit it in
> the near future unless anyone voices concerns about the approach.
As I am preparing this for commit, I'm wondering whether it makes sense to
name the new fun
On Wed, Aug 07, 2024 at 04:22:22PM -0500, Sami Imseih wrote:
> Please see v7.
Thanks. This one looks pretty good to me, and so I plan to commit it in
the near future unless anyone voices concerns about the approach.
--
nathan
On Thu, Aug 08, 2024 at 06:49:27AM +, Bertrand Drouvot wrote:
> 2.2 CLOCK_REALTIME: Its time represents seconds and nanoseconds since the
> Epoch.
> It means that we´re currently about 237 years away from the limit. So even,
> if we were to say add 2 "recents" of them we are still about 183 y
Hi,
On Wed, Aug 07, 2024 at 09:36:59AM -0500, Nathan Bossart wrote:
> Also, do we need to worry about overflow here? It looks like the rest of
> instr_time.h is oblivious about overflow, so maybe this is better discussed
> in a separate thread...
Yeah, a separate thread would be better.
FWIW an
v7-0001-vaccum_delay-with-absolute-time-nanosleep.patch
Description: Binary data
>
> On Wed, Aug 07, 2024 at 06:00:53AM +, Bertrand Drouvot wrote:
>> +SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);
>
> I think this deserves a comment.
>
Done
>> +#define INS
On Wed, Aug 07, 2024 at 06:00:53AM +, Bertrand Drouvot wrote:
> + SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);
I think this deserves a comment.
> +#define INSTR_TIME_ADD_MICROSEC(x,t) \
> + ((x).ticks += t * NS_PER_US)
I'd add parentheses around "t" to ensu
Hi,
On Wed, Aug 07, 2024 at 09:11:19AM -0500, Sami Imseih wrote:
>
>
> > On Aug 7, 2024, at 1:00 AM, Bertrand Drouvot
> > wrote:
> >
> > add t (in microseconds) to x”
>
>
> I was attempting to be more verbose in the comment,
> but what you have above matches the format of
> the other commen
> On Aug 7, 2024, at 1:00 AM, Bertrand Drouvot
> wrote:
>
> add t (in microseconds) to x”
I was attempting to be more verbose in the comment,
but what you have above matches the format of
the other comments. I am ok with your revision.
Regards.
Sami
Hi,
On Tue, Aug 06, 2024 at 12:36:44PM -0500, Sami Imseih wrote:
>
>
> v5 takes care of the latest comments by Bertrand.
>
Thanks!
Please find attached a rebase version (due to 39a138fbef) and in passing I
changed
one comment:
"add t in microseconds to a instr_time" -> "add t (in microseco
v5-0001-vaccum_delay-with-absolute-time-nanosleep.patch
Description: Binary data
v5 takes care of the latest comments by Bertrand.
Regards,
Sami
Hi,
On Mon, Aug 05, 2024 at 03:07:34PM -0500, Sami Imseih wrote:
>
> > yeah, we already have a few macros that access the .ticks, so maybe we
> > could add
> > 2 new ones, say:
> >
> > 1. INSTR_TIME_ADD_MS(t1, msec)
> > 2. INSTR_TIME_IS_GREATER(t1, t2)
> >
> > I think the less operations is d
v4-0001-vaccum_delay-with-absolute-time-nanosleep.patch
Description: Binary data
> yeah, we already have a few macros that access the .ticks, so maybe we could
> add
> 2 new ones, say:
>
> 1. INSTR_TIME_ADD_MS(t1, msec)
> 2. INSTR_TIME_IS_GREATER(t1, t2)
>
> I think the less operations is don
Hi,
On Mon, Jul 29, 2024 at 06:15:49PM -0500, Sami Imseih wrote:
> > On Jul 26, 2024, at 3:27 AM, Bertrand Drouvot
> > wrote:
> > 3 ===
> >
> > I gave more thoughts and I think it can be simplified a bit to reduce the
> > number of operations in the while loop.
> >
> > What about relying on a
> On Jul 26, 2024, at 3:27 AM, Bertrand Drouvot
> wrote:
>
> Hi,
>
> On Thu, Jul 25, 2024 at 05:27:15PM -0500, Sami Imseih wrote:
>> I am attaching v3 of the patch which addresses the comments made
>> earlier by Bertrand about the comment in the patch [1].
>
> Thanks!
>
> Looking at it:
>
Hi,
On Thu, Jul 25, 2024 at 05:27:15PM -0500, Sami Imseih wrote:
> I am attaching v3 of the patch which addresses the comments made
> earlier by Bertrand about the comment in the patch [1].
Thanks!
Looking at it:
1 ===
+ struct instr_time start_time;
I think we can get rid of the "struc
v3-0001-vaccum_delay-with-absolute-time-nanosleep.patch
Description: Binary data
attaching the patch again. Something is strange with my email client.
Regards,
Sami
I am attaching v3 of the patch which addresses the comments madeearlier by Bertrand about the comment in the patch [1]. Also I will stick withvacuum_sleep as the name as the function will be inside vacuum.c. I am notsure we should make this function available outside of vacuum, but I would liketo h
Hi,
On Mon, Jul 15, 2024 at 12:20:29PM -0500, Nathan Bossart wrote:
> On Fri, Jul 12, 2024 at 03:39:57PM -0500, Sami Imseih wrote:
> > but Bertrand found long drifts [2[ which I could not reproduce.
> > To safeguard the long drifts, continue to use the &remain time with an
> > additional safeguar
On Fri, Jul 12, 2024 at 03:39:57PM -0500, Sami Imseih wrote:
> but Bertrand found long drifts [2[ which I could not reproduce.
> To safeguard the long drifts, continue to use the &remain time with an
> additional safeguard to make sure the actual sleep does not exceed the
> requested sleep time.
> What does your testing show when you don't have
> the extra check, i.e.,
>
> struct timespec delay;
> struct timespec remain;
>
> delay.tv_sec = microsec / 100L;
> delay.tv_nsec = (microsec % 100L) * 1000;
>
> while (nanosleep(&delay, &remain) == -1 && er
On Fri, Jul 12, 2024 at 12:14:56PM -0500, Sami Imseih wrote:
> 1/ TimestampDifference has a dependency on gettimeofday,
> while my proposal utilizes clock_gettime. There are old discussions
> that did not reach a conclusion comparing both mechanisms.
> My main conclusion from these hacker discuss
>
> I'm imagining something like this:
>
>struct timespec delay;
>TimestampTz end_time;
>
>end_time = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), msec);
>
>do
>{
>longsecs;
>int microsecs;
>
>TimestampDifference(GetCurrentTimes
On 2024-Jul-11, Nathan Bossart wrote:
> I'm imagining something like this:
>
> struct timespec delay;
> TimestampTz end_time;
>
> end_time = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), msec);
>
> do
> {
> longsecs;
> int microsecs;
>
>
Hi,
On Thu, Jul 11, 2024 at 10:15:41AM -0500, Sami Imseih wrote:
>
> > I did a few tests with the patch and did not see any "large" drifts like the
> > ones observed above.
>
> Thanks for testing.
>
> > I think it could be "simplified" by making use of instr_time instead of
> > timespec
> > fo
On Thu, Jul 11, 2024 at 01:10:25PM -0500, Sami Imseih wrote:
>> I'm curious why we wouldn't just subtract "elapsed_time" from "delay" at
>> the bottom of the while loop to avoid needing this extra check.
>
> Can you elaborate further? I am not sure how this will work since delay is a
> timespec
>
> I'm curious why we wouldn't just subtract "elapsed_time" from "delay" at
> the bottom of the while loop to avoid needing this extra check.
Can you elaborate further? I am not sure how this will work since delay is a
timespec
and elapsed time is an instr_time.
Also, in every iteration
+ /*
+* We allow nanosleep to handle interrupts and retry with the
remaining time.
+* However, since nanosleep is susceptible to time drift when
interrupted
+* frequently, we add a safeguard to break out of the nanosleep
whenever the
>
> Correct I attached a v2 of this patch that uses instr_time to check the
> elapsed
> time and break out of the loop. It needs some more benchmarking.
My email client has an issue sending attachments it seems. Reattaching
v2-0001-vaccum_delay-with-absolute-time-nanosleep.patch
Description:
I did a few tests with the patch and did not see any "large" drifts like theones observed above.Thanks for testing.I think it could be "simplified" by making use of instr_time instead of timespecfor current and absolute. Then I think it would be enough to compare theirticks.Correct I attached a v2
Hi,
On Fri, Jul 05, 2024 at 11:49:45AM -0500, Sami Imseih wrote:
>
> > With 50 indexes and 10 parallel workers I can see things like:
> >
> > 2024-07-02 08:22:23.789 UTC [2189616] LOG: expected 1.00, actual
> > 239.378368
> > 2024-07-02 08:22:24.575 UTC [2189616] LOG: expected 0.10, a
>
> A more portable approach which could be to continue using nanosleep and
> add checks to ensure that nanosleep exists whenever
> it goes past an absolute time. This was suggested by Bertrand in an offline
> conversation. I am not yet fully convinced of this idea, but posting the patch
> that im
With 50 indexes and 10 parallel workers I can see things like:2024-07-02 08:22:23.789 UTC [2189616] LOG: expected 1.00, actual 239.3783682024-07-02 08:22:24.575 UTC [2189616] LOG: expected 0.10, actual 224.3317372024-07-02 08:22:25.363 UTC [2189616] LOG: expected 1.30, actual 230.462
Hi,
On Fri, Jun 28, 2024 at 05:50:20PM -0500, Sami Imseih wrote:
>
> Thanks for the feedback!
>
> > On Jun 28, 2024, at 4:34 PM, Tom Lane wrote:
> >
> > Sami Imseih writes:
> >> Reattaching the patch.
> >
> > I feel like this is fundamentally a wrong solution, for the reasons
> > cited in th
>
>> Therefore, rather than "improving" pg_usleep (and uglifying its API),
>> the right answer is to fix parallel vacuum leaders to not depend on
>> pg_usleep in the first place. A better idea might be to use
>> pg_sleep() or equivalent code.
>
> Yes, that is a good idea to explore and it will
Thanks for the feedback!
> On Jun 28, 2024, at 4:34 PM, Tom Lane wrote:
>
> Sami Imseih writes:
>> Reattaching the patch.
>
> I feel like this is fundamentally a wrong solution, for the reasons
> cited in the comment for pg_usleep: long sleeps are a bad idea
> because of the resulting uncerta
On Sat, Jun 29, 2024 at 9:34 AM Tom Lane wrote:
> Therefore, rather than "improving" pg_usleep (and uglifying its API),
> the right answer is to fix parallel vacuum leaders to not depend on
> pg_usleep in the first place. A better idea might be to use
> pg_sleep() or equivalent code.
In case it'
Sami Imseih writes:
> Reattaching the patch.
I feel like this is fundamentally a wrong solution, for the reasons
cited in the comment for pg_usleep: long sleeps are a bad idea
because of the resulting uncertainty about whether we'll respond to
interrupts and such promptly. An example here is tha
> We want patches to be in the pgsql-hackers archives, not temporarily
> accessible via some link.
>
> …Robert
>
Moving to another email going forward.
Reattaching the patch.
0001-Handle-Sleep-interrupts.patch
Description: Binary data
Regards,
Sami Imseih
Amazon Web Services
> I think you need to find a way to disable this "Attachment protected
> by Amazon" thing:
Yes, I am looking into that. I only noticed after sending the email.
Sorry about that.
Sami
Hi,
I think you need to find a way to disable this "Attachment protected
by Amazon" thing:
http://postgr.es/m/01000190606e3d2a-116ead16-84d2-4449-8d18-5053da66b1f4-000...@email.amazonses.com
We want patches to be in the pgsql-hackers archives, not temporarily
accessible via some link.
...Robert
Attachment protected by Amazon:
[0001-Handle-Sleep-interrupts.patch]
https://us-east-1.secure-attach.amazon.com/fcdc82ce-7887-4aa1-af9e-c1161a6b1d6f/bc81fa24-41de-48f9-a767-a6d15801754b
Amazon has replaced attachment with download link. Downloads will be available
until July 28, 2024, 19:59 (UTC
73 matches
Mail list logo
