On Wed, Feb 27, 2019 at 11:07 AM John Naylor <john.nay...@2ndquadrant.com> wrote: > > On Wed, Feb 27, 2019 at 5:06 AM Amit Kapila <amit.kapil...@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
