On 2024-Apr-03, Dilip Kumar wrote:
> Yeah, we missed acquiring the bank lock w.r.t. intervening pages,
> thanks for reporting. Your fix looks correct to me.
Thanks for the quick review! And thanks to Alexander for the report.
Pushed the fix.
--
Álvaro Herrera PostgreSQL Developer —
On Wed, Apr 3, 2024 at 7:40 PM Alvaro Herrera wrote:
>
> Hello,
>
> On 2024-Apr-03, Alexander Lakhin wrote:
>
> > I've managed to trigger an assert added by 53c2a97a9.
> > Please try the following script against a server compiled with
> > -DTEST_SUMMARIZE_SERIAL (initially I observed this failure
Hello,
On 2024-Apr-03, Alexander Lakhin wrote:
> I've managed to trigger an assert added by 53c2a97a9.
> Please try the following script against a server compiled with
> -DTEST_SUMMARIZE_SERIAL (initially I observed this failure without the
> define, it just simplifies reproducing...):
Ah yes,
Hello Alvaro,
27.02.2024 20:33, Alvaro Herrera wrote:
Here's the complete set, with these two names using the singular.
I've managed to trigger an assert added by 53c2a97a9.
Please try the following script against a server compiled with
-DTEST_SUMMARIZE_SERIAL (initially I observed this
On 2024-Mar-04, Tom Lane wrote:
> In hopes of noticing whether there are other similar thinkos,
> I permuted the order of the SlruPageStatus enum values, and
> now I get the expected warnings from gcc:
Thanks for checking! I pushed the fixes.
Maybe we should assign a nonzero value (= 1) to the
I wrote:
> It looks like SLRU_PAGE_EMPTY has (by chance, or deliberately)
> the numeric value of zero, so I guess the majority of our BF
> animals are understanding this as "address != NULL". But that
> doesn't look like a useful test to be making.
In hopes of noticing whether there are other
Alvaro Herrera writes:
> Pushed that way, but we can discuss further wording improvements/changes
> if someone wants to propose any.
I just noticed that drongo is complaining about two lines added
by commit 53c2a97a9:
drongo| 2024-03-04 14:34:52 |
On 2024-Feb-29, Alvaro Herrera wrote:
> On 2024-Feb-29, Kyotaro Horiguchi wrote:
> > Some of them, commit_timestamp_buffers, transaction_buffers,
> > subtransaction_buffers use 0 to mean auto-tuning based on
> > shared-buffer size. I think it's worth adding an extra_desc such as "0
> > to
On 2024-Feb-29, Kyotaro Horiguchi wrote:
> At Tue, 27 Feb 2024 18:33:18 +0100, Alvaro Herrera
> wrote in
> > Here's the complete set, with these two names using the singular.
>
> The commit by the second patch added several GUC descriptions:
>
> > Sets the size of the dedicated buffer pool
At Tue, 27 Feb 2024 18:33:18 +0100, Alvaro Herrera
wrote in
> Here's the complete set, with these two names using the singular.
The commit by the second patch added several GUC descriptions:
> Sets the size of the dedicated buffer pool used for the commit timestamp
> cache.
Some of them,
On Tue, Feb 27, 2024 at 11:41 PM Alvaro Herrera
wrote:
> On 2024-Feb-27, Alvaro Herrera wrote:
>
> > Here's the complete set, with these two names using the singular.
>
> BTW one thing I had not noticed is that before this patch we have
> minimum shmem size that's lower than the lowest you can
On 2024-Feb-27, Alvaro Herrera wrote:
> Here's the complete set, with these two names using the singular.
BTW one thing I had not noticed is that before this patch we have
minimum shmem size that's lower than the lowest you can go with the new
code.
This means Postgres may no longer start when
> On 27 Feb 2024, at 22:33, Alvaro Herrera wrote:
>
>
These patches look amazing!
Best regards, Andrey Borodin.
Here's the complete set, with these two names using the singular.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"
>From 225b2403f7bb9990656d18c8339c452fcd6822c5 Mon Sep 17 00:00:00 2001
On 2024-Feb-27, Andrey M. Borodin wrote:
> Sorry for the late reply, I have one nit. Are you sure that
> multixact_members and multixact_offsets are plural, while transaction
> and commit_timestamp are singular?
> Maybe multixact_members and multixact_offset? Because there are many
> members and
> On 27 Feb 2024, at 21:03, Alvaro Herrera wrote:
>
> On 2024-Feb-27, Dilip Kumar wrote:
>
>>> static const char *const slru_names[] = {
>>>"commit_timestamp",
>>>"multixact_members",
>>>"multixact_offsets",
>>>"notify",
>>>"serializable",
>>>
On 2024-Feb-27, Dilip Kumar wrote:
> > static const char *const slru_names[] = {
> > "commit_timestamp",
> > "multixact_members",
> > "multixact_offsets",
> > "notify",
> > "serializable",
> > "subtransaction",
> > "transaction",
> >
On Mon, Feb 26, 2024 at 9:46 PM Alvaro Herrera
wrote:
> On 2024-Feb-23, Dilip Kumar wrote:
>
> +
> + For each SLRU area that's part of the core server,
> + there is a configuration parameter that controls its size, with the
> suffix
> + _buffers appended. For historical
> + reasons,
On 2024-Feb-23, Dilip Kumar wrote:
> 1.
> + * If no process is already in the list, we're the leader; our first step
> + * is to "close out the group" by resetting the list pointer from
> + * ProcGlobal->clogGroupFirst (this lets other processes set up other
> + * groups later); then we lock the
On 2024-Feb-23, Andrey M. Borodin wrote:
> I'm sure anyone with multiple CPUs should increase, not decrease
> previous default of 128 buffers (with 512MB shared buffers). Having
> more CPUs (the only way to benefit from more locks) implies bigger
> transaction buffers.
Sure.
> IMO making bank
On Fri, Feb 23, 2024 at 1:06 PM Dilip Kumar wrote:
>
> On Fri, Feb 23, 2024 at 1:48 AM Alvaro Herrera
> wrote:
> >
> > On 2024-Feb-07, Dilip Kumar wrote:
> >
> > > On Wed, Feb 7, 2024 at 3:49 PM Alvaro Herrera
> > > wrote:
> >
> > > > Sure, but is that really what we want?
> > >
> > > So your
> On 23 Feb 2024, at 12:36, Dilip Kumar wrote:
>
>> Another thing I've been thinking is that perhaps it would be useful to
>> make the banks smaller, when the total number of buffers is small. For
>> example, if you have 16 or 32 buffers, it's not really clear to me that
>> it makes sense to
On Fri, Feb 23, 2024 at 1:48 AM Alvaro Herrera wrote:
>
> On 2024-Feb-07, Dilip Kumar wrote:
>
> > On Wed, Feb 7, 2024 at 3:49 PM Alvaro Herrera
> > wrote:
>
> > > Sure, but is that really what we want?
> >
> > So your question is do we want these buffers to be in multiple of
> >
On 2024-Feb-07, Dilip Kumar wrote:
> On Wed, Feb 7, 2024 at 3:49 PM Alvaro Herrera wrote:
> > Sure, but is that really what we want?
>
> So your question is do we want these buffers to be in multiple of
> SLRU_BANK_SIZE? Maybe we can have the last bank to be partial, I
> don't think it should
On Wed, Feb 7, 2024 at 3:49 PM Alvaro Herrera wrote:
>
> On 2024-Feb-07, Dilip Kumar wrote:
>
> > On Tue, Feb 6, 2024 at 8:55 PM Alvaro Herrera
> > wrote:
> > >
> > > I made CLOGShmemBuffers, CommitTsShmemBuffers and SUBTRANSShmemBuffers
> > > compute a number that's multiple of SLRU_BANK_SIZE.
On 2024-Feb-07, Dilip Kumar wrote:
> On Tue, Feb 6, 2024 at 8:55 PM Alvaro Herrera wrote:
> >
> > I made CLOGShmemBuffers, CommitTsShmemBuffers and SUBTRANSShmemBuffers
> > compute a number that's multiple of SLRU_BANK_SIZE. But it's a crock,
> > because we don't have that macro at that point,
> On 7 Feb 2024, at 10:58, Dilip Kumar wrote:
>
>> commit_timestamp_slru_buffers
>> transaction_slru_buffers
>> etc
>
> I am not sure we are exposing anything related to SLRU to the user,
I think we already tell something about SLRU to the user. I’d rather consider
if
On Tue, Feb 6, 2024 at 8:55 PM Alvaro Herrera wrote:
>
> Here's the rest of it rebased on top of current master. I think it
> makes sense to have this as one individual commit.
>
> I made CLOGShmemBuffers, CommitTsShmemBuffers and SUBTRANSShmemBuffers
> compute a number that's multiple of
Here's the rest of it rebased on top of current master. I think it
makes sense to have this as one individual commit.
I made CLOGShmemBuffers, CommitTsShmemBuffers and SUBTRANSShmemBuffers
compute a number that's multiple of SLRU_BANK_SIZE. But it's a crock,
because we don't have that macro at
On Tue, Feb 6, 2024 at 4:23 PM Alvaro Herrera wrote:
>
> > > (We also have SimpleLruTruncate, but I think it's not as critical to
> > > have a barrier there anyhow: accessing a slightly outdated page number
> > > could only be a problem if a bug elsewhere causes us to try to truncate
> > > in the
On 2024-Feb-04, Andrey M. Borodin wrote:
> This patch uses wording "banks" in comments before banks start to
> exist. But as far as I understand, it is expected to be committed
> before "banks" patch.
True -- changed that to use ControlLock.
> Besides this patch looks good to me.
Many thanks
On Sun, Feb 4, 2024 at 7:10 PM Alvaro Herrera wrote:
>
> On 2024-Feb-02, Dilip Kumar wrote:
>
> > I have checked the patch and it looks fine to me other than the above
> > question related to memory barrier usage one more question about the
> > same, basically below to instances 1 and 2 look
> On 4 Feb 2024, at 18:38, Alvaro Herrera wrote:
>
> In other words, these barriers are fully useless.
+1. I've tried to understand ideas behind barriers, but latest_page_number is
heuristics that does not need any guarantees at all. It's also is used in
safety check which can fire only
Sorry, brown paper bag bug there. Here's the correct one.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu. Five minutes later I realize that it's also talking
about food"
In short, I propose the attached.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
>From b4ba8135f8044e0077a27fcf6ad18451380cbcb3 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Wed, 31 Jan 2024 12:27:51 +0100
Subject: [PATCH v2] Use atomics for
On 2024-Feb-02, Dilip Kumar wrote:
> I have checked the patch and it looks fine to me other than the above
> question related to memory barrier usage one more question about the
> same, basically below to instances 1 and 2 look similar but in 1 you
> are not using the memory write_barrier whereas
On Thu, Feb 1, 2024 at 4:34 PM Dilip Kumar wrote:
>
> On Thu, Feb 1, 2024 at 4:12 PM Dilip Kumar wrote:
> >
> > On Thu, Feb 1, 2024 at 3:44 PM Alvaro Herrera
> > wrote:
>
> > Okay.
> > >
> > > While I have your attention -- if you could give a look to the 0001
> > > patch I posted, I would
On Thu, Feb 1, 2024 at 4:12 PM Dilip Kumar wrote:
>
> On Thu, Feb 1, 2024 at 3:44 PM Alvaro Herrera wrote:
> Okay.
> >
> > While I have your attention -- if you could give a look to the 0001
> > patch I posted, I would appreciate it.
> >
>
> I will look into it. Thanks.
Some quick
On Thu, Feb 1, 2024 at 3:44 PM Alvaro Herrera wrote:
>
> On 2024-Feb-01, Dilip Kumar wrote:
>
> > On Thu, Feb 1, 2024 at 3:19 PM Alvaro Herrera
> > wrote:
> > >
> > > postgres -c lc_messages=C -c shared_buffers=$((512*17))
> > >
> > > 2024-02-01 10:48:13.548 CET [1535379] FATAL: invalid value
On 2024-Feb-01, Dilip Kumar wrote:
> On Thu, Feb 1, 2024 at 3:19 PM Alvaro Herrera wrote:
> >
> > postgres -c lc_messages=C -c shared_buffers=$((512*17))
> >
> > 2024-02-01 10:48:13.548 CET [1535379] FATAL: invalid value for parameter
> > "transaction_buffers": 17
> > 2024-02-01 10:48:13.548
On Thu, Feb 1, 2024 at 3:19 PM Alvaro Herrera wrote:
>
> Hah:
>
> postgres -c lc_messages=C -c shared_buffers=$((512*17))
>
> 2024-02-01 10:48:13.548 CET [1535379] FATAL: invalid value for parameter
> "transaction_buffers": 17
> 2024-02-01 10:48:13.548 CET [1535379] DETAIL:
Hah:
postgres -c lc_messages=C -c shared_buffers=$((512*17))
2024-02-01 10:48:13.548 CET [1535379] FATAL: invalid value for parameter
"transaction_buffers": 17
2024-02-01 10:48:13.548 CET [1535379] DETAIL: "transaction_buffers" must be a
multiple of 16
--
Álvaro Herrera PostgreSQL
On 2024-Jan-29, Dilip Kumar wrote:
> Thank you for working on this. There is one thing that I feel is
> problematic. We have kept the allowed values for these GUCs to be in
> multiple of SLRU_BANK_SIZE i.e. 16 and that's the reason the min
> values were changed to 16 but in this refactoring
On Fri, Jan 26, 2024 at 11:08 PM Alvaro Herrera wrote:
>
> I've continued to review this and decided that I don't like the mess
> this patch proposes in order to support pg_commit_ts's deletion of all
> files. (Yes, I know that I was the one that proposed this idea. It's
> still a bad one). I'd
On Thu, Jan 25, 2024 at 10:03 PM Alvaro Herrera wrote:
>
> On 2024-Jan-25, Alvaro Herrera wrote:
>
> > Here's a touched-up version of this patch.
>
> > diff --git a/src/backend/storage/lmgr/lwlock.c
> > b/src/backend/storage/lmgr/lwlock.c
> > index 98fa6035cc..4a5e05d5e4 100644
> > ---
> On 26 Jan 2024, at 22:38, Alvaro Herrera wrote:
>
> This is OK because in the
> default compilation each file only has 32 segments, so that requires
> only 32 lwlocks held at once while the file is being deleted.
Do we account somehow that different subsystems do not accumulate
I've continued to review this and decided that I don't like the mess
this patch proposes in order to support pg_commit_ts's deletion of all
files. (Yes, I know that I was the one that proposed this idea. It's
still a bad one). I'd like to change that code by removing the limit
that we can only
On Thu, Jan 25, 2024 at 11:22 AM Alvaro Herrera wrote:
> Still with these auto-tuning GUCs, I noticed that the auto-tuning code
> would continue to grow the buffer sizes with shared_buffers to
> arbitrarily large values. I added an arbitrary maximum of 1024 (8 MB),
> which is much higher than
On 2024-Jan-25, Alvaro Herrera wrote:
> Here's a touched-up version of this patch.
> diff --git a/src/backend/storage/lmgr/lwlock.c
> b/src/backend/storage/lmgr/lwlock.c
> index 98fa6035cc..4a5e05d5e4 100644
> --- a/src/backend/storage/lmgr/lwlock.c
> +++ b/src/backend/storage/lmgr/lwlock.c
>
Here's a touched-up version of this patch.
First, PROC_GLOBAL->clogGroupFirst and SlruCtl->latest_page_number
change from being protected by locks to being atomics, but there's no
mention of considering memory barriers that they should have. Looking
at the code, I think the former doesn't need
On 2024-Jan-08, Dilip Kumar wrote:
> On Mon, Jan 8, 2024 at 4:55 PM Alvaro Herrera wrote:
> >
> > The more I look at TransactionGroupUpdateXidStatus, the more I think
> > it's broken, and while we do have some tests, I don't have confidence
> > that they cover all possible cases.
> >
> > Or, at
On Mon, Jan 8, 2024 at 4:55 PM Alvaro Herrera wrote:
>
> The more I look at TransactionGroupUpdateXidStatus, the more I think
> it's broken, and while we do have some tests, I don't have confidence
> that they cover all possible cases.
>
> Or, at least, if this code is good, then it hasn't been
The more I look at TransactionGroupUpdateXidStatus, the more I think
it's broken, and while we do have some tests, I don't have confidence
that they cover all possible cases.
Or, at least, if this code is good, then it hasn't been sufficiently
explained.
If we have multiple processes trying to
On Wed, Jan 3, 2024 at 12:08 AM Dilip Kumar wrote:
> Yeah, this is indeed an interesting idea. So I think if we are
> interested in working in this direction maybe this can be submitted in
> a different thread, IMHO.
Yeah, that's something quite different from the patch before us.
--
Robert
On Tue, Jan 2, 2024 at 7:53 PM Robert Haas wrote:
>
> On Fri, Dec 22, 2023 at 8:14 AM Andrey M. Borodin
> wrote:
> > Just a side node.
> > It seems like commit log is kind of antipattern of data contention. Even
> > when we build a super-optimized SLRU. Nearby **bits** are written by
> >
On Tue, Jan 2, 2024 at 1:10 PM Andrey M. Borodin wrote:
> > On 2 Jan 2024, at 19:23, Robert Haas wrote:
> >> And it would be even better if page for transaction statuses would be
> >> determined by backend id somehow. Or at least cache line. Can we allocate
> >> a range (sizeof(cacheline)) of
> On 2 Jan 2024, at 19:23, Robert Haas wrote:
>
>>
>> And it would be even better if page for transaction statuses would be
>> determined by backend id somehow. Or at least cache line. Can we allocate a
>> range (sizeof(cacheline)) of xids\subxids\multixacts\whatever for each
>> backend?
On Fri, Dec 22, 2023 at 8:14 AM Andrey M. Borodin wrote:
> Just a side node.
> It seems like commit log is kind of antipattern of data contention. Even when
> we build a super-optimized SLRU. Nearby **bits** are written by different
> CPUs.
> I think that banks and locks are good thing. But
> On 19 Dec 2023, at 10:34, Dilip Kumar wrote:
Just a side node.
It seems like commit log is kind of antipattern of data contention. Even when
we build a super-optimized SLRU. Nearby **bits** are written by different CPUs.
I think that banks and locks are good thing. But also we could
On Mon, Dec 18, 2023 at 11:00 PM Robert Haas wrote:
>
> On Mon, Dec 18, 2023 at 12:04 PM Robert Haas wrote:
> > certain sense they are competing for the same job. However, they do
> > aim to alleviate different TYPES of contention: the group XID update
> > stuff should be most valuable when lots
On Mon, Dec 18, 2023 at 12:53 PM Andrey M. Borodin wrote:
> One page still accommodates 32K transaction statuses under one lock. It feels
> like a lot. About 1 second of transactions on a typical installation.
>
> When the group commit was committed did we have a benchmark to estimate
>
> On 18 Dec 2023, at 22:30, Robert Haas wrote:
>
> On Mon, Dec 18, 2023 at 12:04 PM Robert Haas wrote:
>> certain sense they are competing for the same job. However, they do
>> aim to alleviate different TYPES of contention: the group XID update
>> stuff should be most valuable when lots of
On Mon, Dec 18, 2023 at 12:04 PM Robert Haas wrote:
> certain sense they are competing for the same job. However, they do
> aim to alleviate different TYPES of contention: the group XID update
> stuff should be most valuable when lots of processes are trying to
> update the same page, and the
On Tue, Dec 12, 2023 at 8:29 AM Alvaro Herrera wrote:
> The problem I see is that the group update mechanism is designed around
> contention of the global xact-SLRU control lock; it uses atomics to
> coordinate a single queue when the lock is contended. So if we split up
> the global SLRU
On Thu, Dec 14, 2023 at 4:36 PM Dilip Kumar wrote:
>
> On Wed, Dec 13, 2023 at 5:49 PM Andrey M. Borodin
> wrote:
>
> > > On 12 Dec 2023, at 18:28, Alvaro Herrera wrote:
> > >
> > > Andrey, do you have any stress tests or anything else that you used to
> > > gain confidence in this code?
> >
> On 14 Dec 2023, at 16:32, tender wang wrote:
>
> enable -O2, only one instruction:
> xor eax, eax
This is not fast code. This is how friendly C compiler suggests you that mask
must be 127, not 128.
Best regards, Andrey Borodin.
Andrey M. Borodin 于2023年12月14日周四 17:35写道：
>
>
> > On 14 Dec 2023, at 14:28, tender wang wrote:
> >
> > Now that AND is more faster, Can we replace the '%
> SLRU_MAX_BANKLOCKS' operation in SimpleLruGetBankLock() with '& 127'
>
> unsigned int GetBankno1(unsigned int pageno) {
> return
> On 14 Dec 2023, at 16:06, Dilip Kumar wrote:
>
> I have noticed
> a very good improvement with the addition of patch 0003.
Indeed, a very impressive results! It’s almost x2 of performance on high
contention scenario, on top of previous improvements.
Best regards, Andrey Borodin.
On Wed, Dec 13, 2023 at 5:49 PM Andrey M. Borodin wrote:
> > On 12 Dec 2023, at 18:28, Alvaro Herrera wrote:
> >
> > Andrey, do you have any stress tests or anything else that you used to
> > gain confidence in this code?
>
> We are using only first two steps of the patchset, these steps do not
> On 14 Dec 2023, at 14:28, tender wang wrote:
>
> Now that AND is more faster, Can we replace the '% SLRU_MAX_BANKLOCKS'
> operation in SimpleLruGetBankLock() with '& 127'
unsigned int GetBankno1(unsigned int pageno) {
return pageno & 127;
}
unsigned int GetBankno2(unsigned int
Andrey M. Borodin 于2023年12月14日周四 17:02写道：
>
>
> > On 14 Dec 2023, at 08:12, Amul Sul wrote:
> >
> >
> > + int bankno = pageno & ctl->bank_mask;
> >
> > I am a bit uncomfortable seeing it as a mask, why can't it be simply a
> number
> > of banks (num_banks) and get the bank number through
> On 14 Dec 2023, at 08:12, Amul Sul wrote:
>
>
> + int bankno = pageno & ctl->bank_mask;
>
> I am a bit uncomfortable seeing it as a mask, why can't it be simply a number
> of banks (num_banks) and get the bank number through modulus op (pageno %
> num_banks) instead of bitwise & operation
On Mon, Dec 11, 2023 at 10:42 AM Dilip Kumar wrote:
> On Thu, Nov 30, 2023 at 3:30 PM Dilip Kumar wrote:
> >
> > On Wed, Nov 29, 2023 at 4:58 PM Dilip Kumar
> wrote:
>
> Here is the updated patch based on some comments by tender wang (those
> comments were sent only to me)
>
few nitpicks:
+
> On 12 Dec 2023, at 18:28, Alvaro Herrera wrote:
>
> Andrey, do you have any stress tests or anything else that you used to
> gain confidence in this code?
We are using only first two steps of the patchset, these steps do not touch
locking stuff.
We’ve got some confidence after Yura
On Tue, Dec 12, 2023 at 6:58 PM Alvaro Herrera wrote:
>
> [Added Andrey again in CC, because as I understand they are using this
> code or something like it in production. Please don't randomly remove
> people from CC lists.]
Oh, glad to know that. Yeah, I generally do not remove but I have
[Added Andrey again in CC, because as I understand they are using this
code or something like it in production. Please don't randomly remove
people from CC lists.]
I've been looking at this some more, and I'm not confident in that the
group clog update stuff is correct. I think the injection
On Wed, Nov 29, 2023 at 3:29 PM Alvaro Herrera wrote:
>
> On 2023-Nov-29, tender wang wrote:
>
> > The v8-0001 patch failed to apply in my local repo as below:
> >
> > git apply v8-0001-Make-all-SLRU-buffer-sizes-configurable.patch
> > error: patch failed:
On 2023-Nov-29, tender wang wrote:
> The v8-0001 patch failed to apply in my local repo as below:
>
> git apply v8-0001-Make-all-SLRU-buffer-sizes-configurable.patch
> error: patch failed: src/backend/access/transam/multixact.c:1851
> error: src/backend/access/transam/multixact.c: patch does not
The v8-0001 patch failed to apply in my local repo as below:
git apply v8-0001-Make-all-SLRU-buffer-sizes-configurable.patch
error: patch failed: src/backend/access/transam/multixact.c:1851
error: src/backend/access/transam/multixact.c: patch does not apply
error: patch failed:
On Thu, Nov 23, 2023 at 11:34 AM Dilip Kumar wrote:
>
> Note: With this testing, we have found a bug in the bank-wise
> approach, basically we are clearing a procglobal->clogGroupFirst, even
> before acquiring the bank lock that means in most of the cases there
> will be a single process in each
On Tue, Nov 21, 2023 at 2:03 PM Dilip Kumar wrote:
>
> On Mon, Nov 20, 2023 at 4:42 PM Dilip Kumar wrote:
> >
> > On Mon, Nov 20, 2023 at 2:37 PM Andrey M. Borodin
> > wrote:
> >
> > > > On 20 Nov 2023, at 13:51, Dilip Kumar wrote:
> > > >
> > > > 2) Do we really need one separate lwlock
On Mon, Nov 20, 2023 at 2:37 PM Andrey M. Borodin wrote:
> > On 20 Nov 2023, at 13:51, Dilip Kumar wrote:
> >
> > 2) Do we really need one separate lwlock tranche for each SLRU?
> >
> > IMHO if we use the same lwlock tranche then the wait event will show
> > the same wait event name, right? And
> On 20 Nov 2023, at 13:51, Dilip Kumar wrote:
>
> 2) Do we really need one separate lwlock tranche for each SLRU?
>
> IMHO if we use the same lwlock tranche then the wait event will show
> the same wait event name, right? And that would be confusing for the
> user, whether we are waiting
On Fri, Nov 17, 2023 at 7:28 PM Alvaro Herrera wrote:
>
> On 2023-Nov-17, Dilip Kumar wrote:
I think I need some more clarification for some of the review comments
> > On Thu, Nov 16, 2023 at 3:11 PM Alvaro Herrera
> > wrote:
> > >
> > > I just noticed that 0003 does some changes to
> > >
On Sun, Nov 19, 2023 at 12:39 PM Andrey M. Borodin wrote:
>
> I’ve skimmed through the patch set. Here are some minor notes.
Thanks for the review
>
> 1. Cycles “for (slotno = bankstart; slotno < bankend; slotno++)” in
> SlruSelectLRUPage() and SimpleLruReadPage_ReadOnly() now have identical
>
> On 17 Nov 2023, at 16:11, Dilip Kumar wrote:
>
> On Fri, Nov 17, 2023 at 1:09 PM Dilip Kumar wrote:
>>
>> On Thu, Nov 16, 2023 at 3:11 PM Alvaro Herrera
>> wrote:
>
> PFA, updated patch version, this fixes the comment given by Alvaro and
> also improves some of the comments.
I’ve
On 2023-Nov-18, Dilip Kumar wrote:
> On Fri, Nov 17, 2023 at 6:16 PM Alvaro Herrera
> wrote:
> > I wonder what's the deal with false sharing in the new
> > bank_cur_lru_count array. Maybe instead of using LWLockPadded for
> > bank_locks, we should have a new struct, with both the LWLock and
On Fri, Nov 17, 2023 at 6:16 PM Alvaro Herrera wrote:
Thanks for the review, all comments looks fine to me, replying to
those that need some clarification
> I wonder what's the deal with false sharing in the new
> bank_cur_lru_count array. Maybe instead of using LWLockPadded for
> bank_locks,
On 2023-Nov-17, Dilip Kumar wrote:
> On Thu, Nov 16, 2023 at 3:11 PM Alvaro Herrera
> wrote:
> >
> > I just noticed that 0003 does some changes to
> > TransactionGroupUpdateXidStatus() that haven't been adequately
> > explained AFAICS. How do you know that these changes are safe?
>
> IMHO
In SlruSharedData, a new comment is added that starts:
"Instead of global counter we maintain a bank-wise lru counter because ..."
You don't need to explain what we don't do. Just explain what we do do.
So remove the words "Instead of a global counter" from there, because
they offer no wisdom.
On Thu, Nov 16, 2023 at 3:11 PM Alvaro Herrera wrote:
>
> I just noticed that 0003 does some changes to
> TransactionGroupUpdateXidStatus() that haven't been adequately
> explained AFAICS. How do you know that these changes are safe?
IMHO this is safe as well as logical to do w.r.t.
I just noticed that 0003 does some changes to
TransactionGroupUpdateXidStatus() that haven't been adequately
explained AFAICS. How do you know that these changes are safe?
0001 contains one typo in the docs, "cotents".
I'm not a fan of the fact that some CLOG sizing macros moved to clog.h,
On Fri, Nov 10, 2023 at 10:17:49AM +0530, Dilip Kumar wrote:
> On Thu, Nov 9, 2023 at 4:55 PM Alvaro Herrera wrote:
>> The only point on which we do not have full consensus yet is the need to
>> have one GUC per SLRU, and a lot of effort seems focused on trying to
>> fix the problem without
On Thu, Nov 9, 2023 at 4:55 PM Alvaro Herrera wrote:
>
> IMO the whole area of SLRU buffering is in horrible shape and many users
> are struggling with overall PG performance because of it. An
> improvement doesn't have to be perfect -- it just has to be much better
> than the current situation,
On Thu, Nov 9, 2023 at 9:39 PM Robert Haas wrote:
>
> On Wed, Nov 8, 2023 at 6:41 AM Dilip Kumar wrote:
> > Here is the updated version of the patch, here I have taken the
> > approach suggested by Andrey and I discussed the same with Alvaro
> > offlist and he also agrees with it. So the idea
On Wed, Nov 8, 2023 at 6:41 AM Dilip Kumar wrote:
> Here is the updated version of the patch, here I have taken the
> approach suggested by Andrey and I discussed the same with Alvaro
> offlist and he also agrees with it. So the idea is that we will keep
> the bank size fixed which is 16 buffers
IMO the whole area of SLRU buffering is in horrible shape and many users
are struggling with overall PG performance because of it. An
improvement doesn't have to be perfect -- it just has to be much better
than the current situation, which should be easy enough. We can
continue to improve later,
> On 8 Nov 2023, at 14:17, Ants Aasma wrote:
>
> Is there a particular reason why lock partitions need to be bigger? We have
> one lock per buffer anyway, bankwise locks will increase the number of locks
> < 10%.
The problem was not attracting much attention for some years. So my reasoning
On Sat, 4 Nov 2023 at 22:08, Andrey M. Borodin wrote:
> On 30 Oct 2023, at 09:20, Dilip Kumar wrote:
>
> changed the logic of SlruAdjustNSlots() in 0002, such that now it
> starts with the next power of 2 value of the configured slots and
> keeps doubling the number of banks until we reach the
On Wed, Nov 8, 2023 at 10:52 AM Amul Sul wrote:
Thanks for review Amul,
> Here are some minor comments:
>
> + * By default, we'll use 1MB of for every 1GB of shared buffers, up to the
> + * maximum value that slru.c will allow, but always at least 16 buffers.
> */
> Size
>
1 - 100 of 118 matches
Mail list logo
