BUG #15039: some question about hash index code

On Wed, Jan 31, 2018 at 5:04 PM, PG Bug reporting form
<noreply@postgresql.org> wrote:

The following bug has been logged on the website:

Bug reference: 15039
Logged by: lixian zou
Email address: zoulx1982@163.com
PostgreSQL version: 10.0
Operating system: source code
Description:

hi,
i read hash index code , and found in _hash_addovflpage function, there is
such code :
if (metap->hashm_firstfree == orig_firstfree)
{
metap->hashm_firstfree = bit + 1;
MarkBufferDirty(metabuf);
}

i found no any chang for metap,metap->hashm_firstfree,and initial the two
variable is equal, so maybe the if statement is redundant?

The hashm_firstfree can be changed by a concurrent session as we
release and reacquire the lock on a metapage while trying to get the
bitmap page. See, below code:

_hash_addovflpage
{
..
..

for (;;)
{
..
/* Release exclusive lock on metapage while reading bitmap page */
LockBuffer(metabuf, BUFFER_LOCK_UNLOCK);
..

/* Reacquire exclusive lock on the meta page */
LockBuffer(metabuf, BUFFER_LOCK_EXCLUSIVE);
}
..

if (metap->hashm_firstfree == orig_firstfree)
{
metap->hashm_firstfree = bit + 1;
MarkBufferDirty(metabuf);
}
..
}

I think pgsql-hackers is the better place to get clarifications related to code.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

thank you for your quick reply.and i have another question, for the following code, whether exist such scene : page_found is false andnewmapbuf is invalid, if so, may be the statement MarkBufferDirty(metabuf); should be placed outside the if statement ?because metap->hashm_spares is changed when page_found is false, buf not mark dirty.

if (page_found)
{
......
}
else
{
/* update the count to indicate new overflow page is added */
metap->hashm_spares[splitnum]++;

if (BufferIsValid(newmapbuf))
{
......

/* add the new bitmap page to the metapage's list of bitmaps */
metap->hashm_mapp[metap->hashm_nmaps] = BufferGetBlockNumber(newmapbuf);
metap->hashm_nmaps++;
metap->hashm_spares[splitnum]++;
MarkBufferDirty(metabuf);
}
}

At 2018-01-31 20:06:22, "Amit Kapila" <amit.kapila16@gmail.com> wrote:

On Wed, Jan 31, 2018 at 5:57 PM, 自己 <zoulx1982@163.com> wrote:

thank you for your quick reply.
and i have another question, for the following code, whether exist such
scene : page_found is false and
newmapbuf is invalid, if so, may be the statement MarkBufferDirty(metabuf);
should be placed outside the if statement ?

On a quick look, your observation seems to be right and I think in
this function we might call markbufferdirty twice for meta page which
doesn't seem to be required. In any case, tomorrow I will look at
this code again and can send a patch to fix it unless you want to send
a patch to fix it.

Thanks for reporting the problem.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jan 31, 2018 at 6:14 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Jan 31, 2018 at 5:57 PM, 自己 <zoulx1982@163.com> wrote:

thank you for your quick reply.
and i have another question, for the following code, whether exist such
scene : page_found is false and
newmapbuf is invalid, if so, may be the statement MarkBufferDirty(metabuf);
should be placed outside the if statement ?

On a quick look, your observation seems to be right and I think in
this function we might call markbufferdirty twice for meta page which
doesn't seem to be required.

Attached patch fix_markbufdirty_hash_index_v1.patch fixes the problem
by calling MarkBufferDirty at the appropriate place in the code.
However, I noticed that we might end up calling MarkBufferDirty twice
for metapage in _hash_addovflpage. I think we can easily avoid that
as is done in patch fix_markbufdirty_hash_index_v1.1.patch.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Feb 1, 2018 at 1:51 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

Attached patch fix_markbufdirty_hash_index_v1.patch fixes the problem
by calling MarkBufferDirty at the appropriate place in the code.
However, I noticed that we might end up calling MarkBufferDirty twice
for metapage in _hash_addovflpage. I think we can easily avoid that
as is done in patch fix_markbufdirty_hash_index_v1.1.patch.

IIUC, the v1 patch is fixing a bug, but the v1.1 patch is a minor
optimization. I'm going to take the easy way out and push v1 to v10
and master.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Feb 2, 2018 at 2:03 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Feb 1, 2018 at 1:51 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

Attached patch fix_markbufdirty_hash_index_v1.patch fixes the problem
by calling MarkBufferDirty at the appropriate place in the code.
However, I noticed that we might end up calling MarkBufferDirty twice
for metapage in _hash_addovflpage. I think we can easily avoid that
as is done in patch fix_markbufdirty_hash_index_v1.1.patch.

IIUC, the v1 patch is fixing a bug, but the v1.1 patch is a minor
optimization.

Yes.

I'm going to take the easy way out and push v1 to v10
and master.

Okay.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com