Horton

Thread

Commits

  1. Fix typo and outdated information in README.barrier

  1. Typo in README.barrier

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2021-05-16T12:11:33Z

    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

  2. Re: Typo in README.barrier

    David Rowley <dgrowleyml@gmail.com> — 2021-05-16T12:51:50Z

    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

  3. Re: Typo in README.barrier

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2021-05-16T13:29:30Z

    > 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

  4. Re: Typo in README.barrier

    David Rowley <dgrowleyml@gmail.com> — 2021-05-17T00:19:41Z

    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

  5. Re: Typo in README.barrier

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2021-05-17T00:33:27Z

    > 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

  6. Re: Typo in README.barrier

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2021-05-17T04:45:09Z

    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

  7. Re: Typo in README.barrier

    David Rowley <dgrowleyml@gmail.com> — 2021-05-17T04:46:29Z

    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

  8. Re: Typo in README.barrier

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2021-05-17T04:48:31Z

    > 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

  9. Re: Typo in README.barrier

    Michael Paquier <michael@paquier.xyz> — 2021-05-17T04:52:54Z

    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

  10. Re: Typo in README.barrier

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2021-05-17T05:18:41Z

    > 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

  11. Re: Typo in README.barrier

    David Rowley <dgrowleyml@gmail.com> — 2021-05-17T22:07:30Z

    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

  12. Re: Typo in README.barrier

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-17T22:37:56Z

    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

  13. Re: Typo in README.barrier

    Michael Paquier <michael@paquier.xyz> — 2021-05-18T00:04:11Z

    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