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.jp
Attachments:
README.barrier.difftext/x-patch; charset=us-asciiDownload
diff --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
Attachments:
README.barrier_v2.difftext/x-patch; charset=us-asciiDownload
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
Attachments:
README.barrier_v3.patchapplication/octet-stream; name=README.barrier_v3.patchDownload
diff --git a/src/backend/storage/lmgr/README.barrier b/src/backend/storage/lmgr/README.barrier
index e73d6799ab..f78e5ac8f6 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. Such a case could be made safe by using an
+atomic variable and an atomic add. See port/atomics.h.
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 either serialize access
+with a lock or use an atomic variable.
3. No ordering guarantees. While memory barriers ensure that any given
process performs loads and stores to shared memory in order, they don't
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