Thread: BUG #15039: some question about hash index code

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?

thanks.

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

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 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 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 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

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