Обсуждение: Typo in README.barrier
I think there is a typo in src/backend/storage/lmgr/README.barrier. Attached patch should fix it. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jpdiff --git a/src/backend/storage/lmgr/README.barrier b/src/backend/storage/lmgr/README.barrier index e73d6799ab..710ad2cca7 100644 --- a/src/backend/storage/lmgr/README.barrier +++ b/src/backend/storage/lmgr/README.barrier @@ -103,7 +103,7 @@ performed before the barrier, and vice-versa. Although this code will work, it is needlessly inefficient. On systems with strong memory ordering (such as x86), the CPU never reorders loads with other -loads, nor stores with other stores. It can, however, allow a load to +loads, nor stores with other stores. It can, however, allow a load to be performed before a subsequent store. To avoid emitting unnecessary memory instructions, we provide two additional primitives: pg_read_barrier(), and pg_write_barrier(). When a memory barrier is being used to separate two
On Mon, 17 May 2021 at 00:11, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > I think there is a typo in src/backend/storage/lmgr/README.barrier. > Attached patch should fix it. Yeah looks like a typo to me. I wonder if we also need to fix this part: > either one does their writes. Eventually we might be able to use an atomic > fetch-and-add instruction for this specific case on architectures that support > it, but we can't rely on that being available everywhere, and we currently > have no support for it at all. Use a lock. That seems to have been written at a time before we got atomics. The following also might want to mention atomics too: > 2. Eight-byte loads and stores aren't necessarily atomic. We assume in > various places in the source code that an aligned four-byte load or store is > atomic, and that other processes therefore won't see a half-set value. > Sadly, the same can't be said for eight-byte value: on some platforms, an > aligned eight-byte load or store will generate two four-byte operations. If > you need an atomic eight-byte read or write, you must make it atomic with a > lock. David
> Yeah looks like a typo to me. Ok. > I wonder if we also need to fix this part: > >> either one does their writes. Eventually we might be able to use an atomic >> fetch-and-add instruction for this specific case on architectures that support >> it, but we can't rely on that being available everywhere, and we currently >> have no support for it at all. Use a lock. > > That seems to have been written at a time before we got atomics. > > The following also might want to mention atomics too: > >> 2. Eight-byte loads and stores aren't necessarily atomic. We assume in >> various places in the source code that an aligned four-byte load or store is >> atomic, and that other processes therefore won't see a half-set value. >> Sadly, the same can't be said for eight-byte value: on some platforms, an >> aligned eight-byte load or store will generate two four-byte operations. If >> you need an atomic eight-byte read or write, you must make it atomic with a >> lock. Yes, we'd better to fix them. Attached is a propsal for these. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp diff --git a/src/backend/storage/lmgr/README.barrier b/src/backend/storage/lmgr/README.barrier index e73d6799ab..ba5ebb844b 100644 --- a/src/backend/storage/lmgr/README.barrier +++ b/src/backend/storage/lmgr/README.barrier @@ -103,7 +103,7 @@ performed before the barrier, and vice-versa. Although this code will work, it is needlessly inefficient. On systems with strong memory ordering (such as x86), the CPU never reorders loads with other -loads, nor stores with other stores. It can, however, allow a load to +loads, nor stores with other stores. It can, however, allow a load to be performed before a subsequent store. To avoid emitting unnecessary memory instructions, we provide two additional primitives: pg_read_barrier(), and pg_write_barrier(). When a memory barrier is being used to separate two @@ -155,18 +155,16 @@ Although this may compile down to a single machine-language instruction, the CPU will execute that instruction by reading the current value of foo, adding one to it, and then storing the result back to the original address. If two CPUs try to do this simultaneously, both may do their reads before -either one does their writes. Eventually we might be able to use an atomic -fetch-and-add instruction for this specific case on architectures that support -it, but we can't rely on that being available everywhere, and we currently -have no support for it at all. Use a lock. +either one does their writes. In this case we can use pg_atomic_fetch_add_u32() +or pg_atomic_fetch_add_u64() depending on the byte length of the variable. 2. Eight-byte loads and stores aren't necessarily atomic. We assume in various places in the source code that an aligned four-byte load or store is atomic, and that other processes therefore won't see a half-set value. Sadly, the same can't be said for eight-byte value: on some platforms, an aligned eight-byte load or store will generate two four-byte operations. If -you need an atomic eight-byte read or write, you must make it atomic with a -lock. +you need an atomic eight-byte read or write, you must make it atomic by using +pg_atomic_*_u64(). 3. No ordering guarantees. While memory barriers ensure that any given process performs loads and stores to shared memory in order, they don't
On Mon, 17 May 2021 at 01:29, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > Yes, we'd better to fix them. Attached is a propsal for these. Thanks for working on that. I had a look and wondered if it might be better to go into slightly less details about the exact atomic function to use. The wording there might lead you to believe you can just call the atomic function on the non-atomic variable. It might be best just to leave the details about how exactly to use atomics by just referencing port/atomics.h. Maybe something like the attached? I'm also a bit on the fence if this should be backpatched or not. The reasons though maybe not is that it seems unlikely maybe people would not be working in master if they're developing something new. On the other side of the argument, 0ccebe779, which adjusts another README was backpatched. I'm leaning towards backpatching. David
Вложения
> Thanks for working on that. I had a look and wondered if it might be > better to go into slightly less details about the exact atomic > function to use. The wording there might lead you to believe you can > just call the atomic function on the non-atomic variable. > > It might be best just to leave the details about how exactly to use > atomics by just referencing port/atomics.h. > > Maybe something like the attached? Thanks. Agreed and your patch looks good to me. > I'm also a bit on the fence if this should be backpatched or not. The > reasons though maybe not is that it seems unlikely maybe people would > not be working in master if they're developing something new. On the > other side of the argument, 0ccebe779, which adjusts another README > was backpatched. I'm leaning towards backpatching. Me too. Let's backpatch. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
David, >> Thanks for working on that. I had a look and wondered if it might be >> better to go into slightly less details about the exact atomic >> function to use. The wording there might lead you to believe you can >> just call the atomic function on the non-atomic variable. >> >> It might be best just to leave the details about how exactly to use >> atomics by just referencing port/atomics.h. >> >> Maybe something like the attached? > > Thanks. Agreed and your patch looks good to me. > >> I'm also a bit on the fence if this should be backpatched or not. The >> reasons though maybe not is that it seems unlikely maybe people would >> not be working in master if they're developing something new. On the >> other side of the argument, 0ccebe779, which adjusts another README >> was backpatched. I'm leaning towards backpatching. > > Me too. Let's backpatch. Would you like to push the patch? -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Mon, 17 May 2021 at 16:45, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > Would you like to push the patch? Yeah, I can. I was just letting it sit for a while to see if anyone else had an opinion about backpatching. David
> On Mon, 17 May 2021 at 16:45, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: >> Would you like to push the patch? > > Yeah, I can. I was just letting it sit for a while to see if anyone > else had an opinion about backpatching. Ok. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Mon, May 17, 2021 at 09:33:27AM +0900, Tatsuo Ishii wrote: > Me too. Let's backpatch. A README is not directly user-facing, it is here for developers, so I would not really bother with a backpatch. Now it is not a big deal to do so either, so that's not a -1 from me, more a +0, for "please feel free to do what you think is most adapted". You may want to hold on until 14beta1 is tagged, though. -- Michael
Вложения
> On Mon, May 17, 2021 at 09:33:27AM +0900, Tatsuo Ishii wrote: >> Me too. Let's backpatch. > > A README is not directly user-facing, it is here for developers, so I > would not really bother with a backpatch. Now it is not a big deal to > do so either, so that's not a -1 from me, more a +0, for "please feel > free to do what you think is most adapted". I think README is similar to code comments. If a code comment is wrong, we usually fix to back branches. Why can't we do the same thing for README? > You may want to hold on until 14beta1 is tagged, though. Of course we can wait till that day but I wonder why. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Mon, 17 May 2021 at 17:18, Tatsuo Ishii <ishii@sraoss.co.jp> wrote: > > > On Mon, May 17, 2021 at 09:33:27AM +0900, Tatsuo Ishii wrote: > >> Me too. Let's backpatch. > > > > A README is not directly user-facing, it is here for developers, so I > > would not really bother with a backpatch. Now it is not a big deal to > > do so either, so that's not a -1 from me, more a +0, for "please feel > > free to do what you think is most adapted". > > I think README is similar to code comments. If a code comment is > wrong, we usually fix to back branches. Why can't we do the same thing > for README? Thanks for the votes. Since Michael was on the fence and I was just leaning over it and Ishii-san was pro-backpatch, I backpatched it. > > You may want to hold on until 14beta1 is tagged, though. > > Of course we can wait till that day but I wonder why. I imagined that would be a good idea for more risky patches so we don't break something before a good round of buildfarm testing. However, since this is just a README, I didn't think it would have mattered. Maybe there's another reason I'm overlooking? David
David Rowley <dgrowleyml@gmail.com> writes:
>>> You may want to hold on until 14beta1 is tagged, though.
>> Of course we can wait till that day but I wonder why.
> I imagined that would be a good idea for more risky patches so we
> don't break something before a good round of buildfarm testing.
> However, since this is just a README, I didn't think it would have
> mattered. Maybe there's another reason I'm overlooking?
Generally it's considered poor form to push any inessential patches
during a release window (which I'd define roughly as 48 hours before
the wrap till after the tag is applied). It complicates the picture
for the final round of buildfarm testing, and if we have to do a
re-wrap then we're faced with the question of whether to back out
the patch.
In this case, it being just a README, I agree there's no harm done.
But we've been burnt by "low risk" patches before, so I'd tend to
err on the side of caution during a release window.
regards, tom lane
On Mon, May 17, 2021 at 06:37:56PM -0400, Tom Lane wrote: > Generally it's considered poor form to push any inessential patches > during a release window (which I'd define roughly as 48 hours before > the wrap till after the tag is applied). It complicates the picture > for the final round of buildfarm testing, and if we have to do a > re-wrap then we're faced with the question of whether to back out > the patch. > > In this case, it being just a README, I agree there's no harm done. > But we've been burnt by "low risk" patches before, so I'd tend to > err on the side of caution during a release window. Yes, I've had this experience once in the past. So I tend to just wait until the tag is pushed as long as it is not critical for the release. -- Michael