On Tue, 29 Nov 2022 at 20:46, Tom Lane wrote:
>
> I wrote:
> > That seems like a fairly bad idea: it will add extra contention
> > on ProcArrayLock, and I see no real strong argument that the path
> > can't get traversed often enough for that to matter. It would
> > likely be better for
Michail Nikolaev writes:
> The small thing I was thinking to add in KnownAssignedXidsCompress is
> the assertion like
> Assert(MyBackendType == B_STARTUP);
Mmm ... given where the call sites are, we have got lots more problems
than this if some non-startup process reaches them. I'm not sure
Hello, Tom.
> Since we're running out of time in the current commitfest,
> I went ahead and changed that, and made the cosmetic fixes
> I wanted, and pushed.
Great, thanks!
The small thing I was thinking to add in KnownAssignedXidsCompress is
the assertion like
Assert(MyBackendType ==
I wrote:
> That seems like a fairly bad idea: it will add extra contention
> on ProcArrayLock, and I see no real strong argument that the path
> can't get traversed often enough for that to matter. It would
> likely be better for KnownAssignedXidsCompress to obtain the lock
> for itself, only
Michail Nikolaev writes:
>> * when idle - since we have time to do it when that happens, which
>> happens often since most workloads are bursty
> I have added getting of ProcArrayLock for this case.
That seems like a fairly bad idea: it will add extra contention
on ProcArrayLock, and I see no
Hello, everyone.
I have tried to put it all together.
> In the absence of that approach, falling back to a counter that
> compresses every N xids would be best, in addition to the two new
> forced compression events.
Done.
> Also, if we add more forced compressions, it seems like we should
On Tue, 22 Nov 2022 at 16:53, Tom Lane wrote:
>
> Simon Riggs writes:
> > On Tue, 22 Nov 2022 at 16:28, Tom Lane wrote:
> >> If we do those things, do we need a wasted-work counter at all?
>
> > The wasted work counter works well to respond to heavy read-only
> > traffic and also avoids wasted
Simon Riggs writes:
> On Tue, 22 Nov 2022 at 16:28, Tom Lane wrote:
>> If we do those things, do we need a wasted-work counter at all?
> The wasted work counter works well to respond to heavy read-only
> traffic and also avoids wasted compressions for write-heavy workloads.
> So I still like it
On Tue, 22 Nov 2022 at 16:28, Tom Lane wrote:
>
> Simon Riggs writes:
> > We seem to have replaced one magic constant with another, so not sure
> > if this is autotuning, but I like it much better than what we had
> > before (i.e. better than my prev patch).
>
> Yeah, the magic constant is still
Simon Riggs writes:
> We seem to have replaced one magic constant with another, so not sure
> if this is autotuning, but I like it much better than what we had
> before (i.e. better than my prev patch).
Yeah, the magic constant is still magic, even if it looks like it's
not terribly sensitive to
On Sun, 20 Nov 2022 at 13:45, Michail Nikolaev
wrote:
> If such approach looks committable - I could do more careful
> performance testing to find the best value for
> WASTED_SNAPSHOT_WORK_LIMIT_TO_COMPRESS.
Nice patch.
We seem to have replaced one magic constant with another, so not sure
if
Oh, seems like it is not my day :) The image fixed again.
Oops, wrong image, this is correct one. But is 1-run tests, so it
shows only basic correlation,
Hello.
On Wed, Nov 16, 2022 at 3:44 AM Andres Freund wrote:
> Approach 1:
> We could have an atomic variable in ProcArrayStruct that counts the amount of
> wasted effort and have processes update it whenever they've wasted a
> meaningful amount of effort. Something like counting the skipped
Hello everyone.
> However ... I tried to reproduce the original complaint, and
> failed entirely. I do see KnownAssignedXidsGetAndSetXmin
> eating a bit of time in the standby backends, but it's under 1%
> and doesn't seem to be rising over time. Perhaps we've already
> applied some
On Wed, 16 Nov 2022 at 00:15, Tom Lane wrote:
>
> Andres Freund writes:
> > On 2022-11-15 23:14:42 +, Simon Riggs wrote:
> >> Hence more frequent compression is effective at reducing the overhead.
> >> But too frequent compression slows down the startup process, which
> >> can't then keep
Hi,
On 2022-11-15 19:46:26 -0500, Tom Lane wrote:
> Andres Freund writes:
> > To me it sounds like known_assigned_xids_lck is pointless and the talk about
> > memory barriers a red herring, since all modifications have to happen with
> > ProcArrayLock held exlusively and all reads with
Andres Freund writes:
> To me it sounds like known_assigned_xids_lck is pointless and the talk about
> memory barriers a red herring, since all modifications have to happen with
> ProcArrayLock held exlusively and all reads with ProcArrayLock held in share
> mode. It can't be legal to modify
Hi,
On 2022-11-15 19:15:15 -0500, Tom Lane wrote:
> Andres Freund writes:
> > On 2022-11-15 23:14:42 +, Simon Riggs wrote:
> >> Hence more frequent compression is effective at reducing the overhead.
> >> But too frequent compression slows down the startup process, which
> >> can't then keep
Hi,
On 2022-11-15 23:14:42 +, Simon Riggs wrote:
> On Tue, 15 Nov 2022 at 23:06, Tom Lane wrote:
> >
> > BTW, while nosing around this code I came across this statement
> > (procarray.c, about line 4550 in HEAD):
> >
> > * ... To add XIDs to the array, we just insert
> > * them into slots
Simon Riggs writes:
> but that is not related to the main issues:
> * COMMITs: xids are removed from the array by performing a binary
> search - this gets more and more expensive as the array gets wider
> * SNAPSHOTs: scanning the array for snapshots becomes more expensive
> as the array gets
Andres Freund writes:
> On 2022-11-15 23:14:42 +, Simon Riggs wrote:
>> Hence more frequent compression is effective at reducing the overhead.
>> But too frequent compression slows down the startup process, which
>> can't then keep up.
>> So we're just looking for an optimal frequency of
Hi,
On 2022-11-15 23:14:42 +, Simon Riggs wrote:
> * COMMITs: xids are removed from the array by performing a binary
> search - this gets more and more expensive as the array gets wider
> * SNAPSHOTs: scanning the array for snapshots becomes more expensive
> as the array gets wider
>
> Hence
On Tue, 15 Nov 2022 at 23:06, Tom Lane wrote:
>
> BTW, while nosing around this code I came across this statement
> (procarray.c, about line 4550 in HEAD):
>
> * ... To add XIDs to the array, we just insert
> * them into slots to the right of the head pointer and then advance the head
> *
BTW, while nosing around this code I came across this statement
(procarray.c, about line 4550 in HEAD):
* ... To add XIDs to the array, we just insert
* them into slots to the right of the head pointer and then advance the head
* pointer. This wouldn't require any lock at all, except that on
Simon Riggs writes:
> I've cleaned up the comments and used a #define for the constant.
> IMHO this is ready for commit. I will add it to the next CF.
I looked at this a little. It's a simple enough patch, and if it
solves the problem then I sure like it better than the previous
ideas in this
On Wed, Nov 9, 2022 at 1:54 PM Andres Freund wrote:
> On 2022-11-09 11:42:36 +1300, Thomas Munro wrote:
> > On Sat, Sep 17, 2022 at 6:27 PM Simon Riggs
> > wrote:
> > > I've cleaned up the comments and used a #define for the constant.
> > >
> > > IMHO this is ready for commit. I will add it to
Hi,
On 2022-11-09 11:42:36 +1300, Thomas Munro wrote:
> On Sat, Sep 17, 2022 at 6:27 PM Simon Riggs
> wrote:
> > I've cleaned up the comments and used a #define for the constant.
> >
> > IMHO this is ready for commit. I will add it to the next CF.
>
> FYI This had many successful cfbot runs but
On Sat, Sep 17, 2022 at 6:27 PM Simon Riggs
wrote:
> I've cleaned up the comments and used a #define for the constant.
>
> IMHO this is ready for commit. I will add it to the next CF.
FYI This had many successful cfbot runs but today it blew up on
Windows when the assertion
On Fri, 16 Sept 2022 at 17:08, Michail Nikolaev
wrote:
>
> Hello everyone.
>
> To find the best frequency for calling KnownAssignedXidsCompress in
> Simon's patch, I made a set of benchmarks. It looks like each 8th xid
> is a little bit often.
>
> Setup and method is the same as previous (1).
> On 2 Aug 2022, at 20:18, Simon Riggs wrote:
>
> On Tue, 2 Aug 2022 at 12:32, Andrey Borodin wrote:
>
>> KnownAssignedXidsRemoveTree() only compress with probability 1/8, but it is
>> still O(N*N).
>
> Currently it is O(NlogS), not quite as bad as O(N^2).
Consider workload when we have a
At Tue, 2 Aug 2022 16:18:44 +0100, Simon Riggs
wrote in
> On Tue, 2 Aug 2022 at 12:32, Andrey Borodin wrote:
>
> > KnownAssignedXidsRemoveTree() only compress with probability 1/8, but it is
> > still O(N*N).
>
> Currently it is O(NlogS), not quite as bad as O(N^2).
>
> Since each xid in
On Tue, 2 Aug 2022 at 12:32, Andrey Borodin wrote:
> KnownAssignedXidsRemoveTree() only compress with probability 1/8, but it is
> still O(N*N).
Currently it is O(NlogS), not quite as bad as O(N^2).
Since each xid in the tree is always stored to the right, it should be
possible to make that
> On 29 Jul 2022, at 20:08, Simon Riggs wrote:
>
> A simple patch like this seems to hit the main concern, aiming to keep
> the array from spreading out and impacting snapshot performance for
> SELECTs, yet not doing it so often that the startup process has a
> higher burden of work.
The
On Fri, 29 Jul 2022 at 18:24, Michail Nikolaev
wrote:
> > A simple patch like this seems to hit the main concern, aiming to keep
> > the array from spreading out and impacting snapshot performance for
> > SELECTs, yet not doing it so often that the startup process has a
> > higher burden of
Hello.
Thanks to everyone for the review.
> It seems to me storing the index itself is simpler and maybe faster by
> the cycles to perform addition.
Yes, first version used 1-byte for offset with maximum value of 255.
Agreed, looks like there is no sense to store offsets now.
> A simple patch
On Wed, 27 Jul 2022 at 08:08, Kyotaro Horiguchi wrote:
>
> At Tue, 26 Jul 2022 23:09:16 +0500, Andrey Borodin
> wrote in
> >
> >
> > > On 20 Jul 2022, at 02:12, Michail Nikolaev
> > > wrote:
> > >
> > >> I've looked into v5.
> > > Thanks!
> > >
> > > Patch is updated accordingly your remarks.
At Tue, 26 Jul 2022 23:09:16 +0500, Andrey Borodin wrote
in
>
>
> > On 20 Jul 2022, at 02:12, Michail Nikolaev
> > wrote:
> >
> >> I've looked into v5.
> > Thanks!
> >
> > Patch is updated accordingly your remarks.
>
> The patch seems Ready for Committer from my POV.
+ * is s updated
> On 20 Jul 2022, at 02:12, Michail Nikolaev wrote:
>
>> I've looked into v5.
> Thanks!
>
> Patch is updated accordingly your remarks.
The patch seems Ready for Committer from my POV.
Thanks!
Best regards, Andrey Borodin.
Hello, Andrey.
> I've looked into v5.
Thanks!
Patch is updated accordingly your remarks.
Best regards,
Michail.
From 1301a262dea7f541c11092851e82f10932150ee3 Mon Sep 17 00:00:00 2001
From: Michail Nikolaev
Date: Tue, 19 Jul 2022 23:50:53 +0300
Subject: [PATCH v6] Currently
> On 1 Apr 2022, at 04:18, Michail Nikolaev wrote:
>
> Hello.
>
> Just an updated commit message.
I've looked into v5.
IMO the purpose of KnownAssignedXidsNext would be slightly more obvious if it
was named KnownAssignedXidsNextOffset.
Also please consider some editorialisation:
s/high
Hello, Simon.
Sorry for calling you directly, but you know the subject better than
anyone else. It is related to your work from 2010 - replacing
KnownAssignedXidsHash with the KnownAssignedXids array.
I have added additional optimization to the data structure you
implemented. Initially, it was
Hello.
Just an updated commit message.
Thanks,
Michail.
From 934d649a18c66f8b448463e57375865b33ce53e7 Mon Sep 17 00:00:00 2001
From: nkey
Date: Fri, 1 Apr 2022 02:17:55 +0300
Subject: [PATCH v5] Optimize KnownAssignedXidsGetAndSetXmin by maintaining
offset to next valid xid.
MIME-Version: 1.0
Hello, Andrey.
Thanks for your efforts.
> Patch on barrier seems too complicated to me right now. I’d propose to focus
> on KnowAssignedXidsNext patch: it’s clean, simple and effective.
I'll extract it to the separated patch later.
> I’ve rebased the patch so that it does not depend on
> On 22 Nov 2021, at 14:05, Michail Nikolaev wrote:
>
>> Write barrier must be issued after write, not before.
>> Don't we need to issue read barrier too?
>
> The write barrier is issued after the changes to KnownAssignedXidsNext
> and KnownAssignedXidsValid arrays and before the update of
>
Hello, Andrey.
> Write barrier must be issued after write, not before.
> Don't we need to issue read barrier too?
The write barrier is issued after the changes to KnownAssignedXidsNext
and KnownAssignedXidsValid arrays and before the update of
headKnownAssignedXids.
So, it seems to be correct.
> 21 нояб. 2021 г., в 23:58, Michail Nikolaev
> написал(а):
>
>
Write barrier must be issued after write, not before.
Don't we need to issue read barrier too?
Best regards, Andrey Borodin.
Hello, Alexander.
Thanks for your review.
> It looks like KnownAssignedXidsNext doesn't have to be
> pg_atomic_uint32. I see it only gets read with pg_atomic_read_u32()
> and written with pg_atomic_write_u32(). Existing code believes that
> read/write of 32-bit values is atomic. So, you can
On Wed, Nov 10, 2021 at 12:16 AM Michail Nikolaev
wrote:
> I updated the patch a little. KnownAssignedXidsGetAndSetXmin now
> causes fewer cache misses because some values are stored in variables
> (registers). I think it is better to not lean on the compiler here
> because of `volatile` args.
>
Hello, Andrey.
Thanks for your feedback.
> Current patch addresses another problem. In presence of old enough
> transaction enumeration of KnownAssignedXids with shared lock prevents adding
> new transactions with exclusive lock. And recovery effectively pauses.
Actually, I see two problems
Sorry for so late reply. I've been thinking about possible approaches.
KnownAssignedXids over hashtable in fact was implemented long before and
rejected [0].
> 3 авг. 2021 г., в 22:35, Andres Freund написал(а):
>
> On 2021-08-03 10:33:50 +0500, Andrey Borodin wrote:
>>> 3 авг. 2021 г., в
Hello, Andres.
Could you please clarify how to better deal with the situation?
According to your previous letter, I think there was some
misunderstanding regarding the latest patch version (but I am not
sure). Because as far as understand provided optimization (lazily
calculated optional offset
On Tue, Aug 10, 2021 at 12:45:17AM +0300, Michail Nikolaev wrote:
> Thanks for the feedback again.
From what I can see, there has been some feedback from Andres here,
and the thread is idle for six weeks now, so I have marked this patch
as RwF in the CF app.
--
Michael
signature.asc
Hello, Andres.
Thanks for the feedback again.
> The problem is that we don't want to add a lot of work
> KnownAssignedXidsAdd/Remove, because very often nobody will build a snapshot
> for that moment and building a sorted, gap-free, linear array of xids isn't
> cheap. In my experience it's more
Hi,
On 2021-08-03 22:23:58 +0300, Michail Nikolaev wrote:
> > I'm not sure that we need to care as much about the cost of
> > KnownAssignedXidsGetAndSetXmin() - for one, the caching we now have makes
> > that
> > less frequent.
>
> It is still about 5-7% of CPU for a typical workload, a
Hello, Andres.
Thanks for your feedback.
>> Maybe use a hashtable of running transactions? It will be slightly faster
>> when adding\removing single transactions. But much worse when doing
>> KnownAssignedXidsRemove().
> Why would it be worse for KnownAssignedXidsRemove()? Were you intending to
Hi,
On 2021-08-03 10:33:50 +0500, Andrey Borodin wrote:
> > 3 авг. 2021 г., в 03:01, Andres Freund написал(а):
> > On 2021-08-03 00:07:23 +0300, Michail Nikolaev wrote:
> >> The main idea is simple optimistic optimization - store offset to next
> >> valid entry. So, in most cases, we could just
Hi,
> 3 авг. 2021 г., в 03:01, Andres Freund написал(а):
>
> Hi,
>
> On 2021-08-03 00:07:23 +0300, Michail Nikolaev wrote:
>> The main idea is simple optimistic optimization - store offset to next
>> valid entry. So, in most cases, we could just skip all the gaps.
>> Of course, it adds some
Hi,
On 2021-08-03 00:07:23 +0300, Michail Nikolaev wrote:
> The main idea is simple optimistic optimization - store offset to next
> valid entry. So, in most cases, we could just skip all the gaps.
> Of course, it adds some additional impact for workloads without long
> (few seconds) transactions
Hello.
> I have tried such an approach but looks like it is not effective,
> probably because of CPU caching issues.
It was a mistake by me. I have repeated the approach and got good
results with small and a non-invasive patch.
The main idea is simple optimistic optimization - store offset to
Hello, Kirill.
> Also, maybe it is better to reduce the invasivity by using a more
> simple approach. For example, use the first bit to mark xid as valid
> and the last 7 bit (128 values) as an optimistic offset to the next
> valid xid (jump by 127 steps in the worse scenario).
> What do you
)Hello.
> I recently ran into a problem in one of our production postgresql cluster.
> I had noticed lock contention on procarray lock on standby, which causes WAL
> replay lag growth.
Yes, I saw the same issue on my production cluster.
> 1) set max_connections to big number, like 10
I
sorry, forgot to add a patch to the letter
чт, 20 мая 2021 г. в 13:52, Кирилл Решке :
> Hi,
> I recently ran into a problem in one of our production postgresql cluster.
> I had noticed lock contention on procarray lock on standby, which causes
> WAL replay lag growth.
> To reproduce this, you
Hi,
I recently ran into a problem in one of our production postgresql cluster.
I had noticed lock contention on procarray lock on standby, which causes
WAL replay lag growth.
To reproduce this, you can do the following:
1) set max_connections to big number, like 10
2) begin a transaction on
64 matches
Mail list logo
