On Wed, Feb 27, 2019 at 11:07 AM John Naylor
<john.naylor@2ndquadrant.com> wrote:
>
> On Wed, Feb 27, 2019 at 5:06 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > I have tried this test many times (more than 1000 times) by varying
> > thread count, but couldn't reproduce it. My colleague, Kuntal has
> > tried a similar test overnight, but the issue didn't reproduce which
> > is not surprising to me seeing the nature of the problem. As I could
> > reproduce it via the debugger, I think we can go ahead with the fix.
> > I have improved the comments in the attached patch and I have also
> > tried to address Tom's concern w.r.t comments by adding additional
> > comments atop of data-structure used to maintain the local map.
>
> The flaw in my thinking was treating extension too similarly too
> finding an existing block. With your patch clearing the local map in
> the correct place, it seems the call at hio.c:682 is now superfluous?
>
What if get some valid block from the first call to
GetPageWithFreeSpace via local map which has required space? I think
in that case we will need the call at hio.c:682. Am I missing
something?
> It wasn't sufficient before, so should this call be removed so as not
> to mislead? Or maybe keep it to be safe, with a comment like "This
> should already be cleared by now, but make sure it is."
>
> + * To maintain good performance, we only mark every other page as available
> + * in this map.
>
> I think this implementation detail is not needed to comment on the
> struct. I think the pointer to the README is sufficient.
>
Fair enough, I will remove that part of a comment once we agree on the
above point.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com