Horton

Thread

Commits

  1. Fix possible failure to mark hash metapage dirty.

  1. BUG #15039: some question about hash index code

    The Post Office <noreply@postgresql.org> — 2018-01-31T11:34:46Z

    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.

  2. Re: BUG #15039: some question about hash index code

    Amit Kapila <amit.kapila16@gmail.com> — 2018-01-31T12:06:22Z

    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

  3. Re:Re: BUG #15039: some question about hash index code

    zoulx1982 <zoulx1982@163.com> — 2018-01-31T12:27:26Z

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

  4. Re: Re: BUG #15039: some question about hash index code

    Amit Kapila <amit.kapila16@gmail.com> — 2018-01-31T12:44:51Z

    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

  5. Re: Re: BUG #15039: some question about hash index code

    Amit Kapila <amit.kapila16@gmail.com> — 2018-02-01T06:51:53Z

    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

  6. Re: Re: BUG #15039: some question about hash index code

    Robert Haas <robertmhaas@gmail.com> — 2018-02-01T20:33:49Z

    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

  7. Re: Re: BUG #15039: some question about hash index code

    Amit Kapila <amit.kapila16@gmail.com> — 2018-02-02T02:46:52Z

    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