BUG #15039: some question about hash index code

Started by PG Bug reporting formalmost 8 years ago7 messages

noreply@postgresql.org

almost 8 years ago

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.

Amit Kapila

amit.kapila16@gmail.com

almost 8 years ago

In reply to: PG Bug reporting form (#1)

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

自己

zoulx1982@163.com

almost 8 years ago

In reply to: Amit Kapila (#2)

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

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:

Show quoted text

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

Amit Kapila

amit.kapila16@gmail.com

almost 8 years ago

In reply to: 自己 (#3)

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

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

Amit Kapila

amit.kapila16@gmail.com

almost 8 years ago

In reply to: Amit Kapila (#4)

2 attachment(s)

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

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

Attachments:

fix_markbufdirty_hash_index_v1.patchapplication/octet-stream; name=fix_markbufdirty_hash_index_v1.patchDownload

diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c
index c9de128..2033b2f 100644
--- a/src/backend/access/hash/hashovfl.c
+++ b/src/backend/access/hash/hashovfl.c
@@ -341,9 +341,10 @@ found:
 			metap->hashm_mapp[metap->hashm_nmaps] = BufferGetBlockNumber(newmapbuf);
 			metap->hashm_nmaps++;
 			metap->hashm_spares[splitnum]++;
-			MarkBufferDirty(metabuf);
 		}
 
+		MarkBufferDirty(metabuf);
+
 		/*
 		 * for new overflow page, we don't need to explicitly set the bit in
 		 * bitmap page, as by default that will be set to "in use".

fix_markbufdirty_hash_index_v1.1.patchapplication/octet-stream; name=fix_markbufdirty_hash_index_v1.1.patchDownload

diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c
index c9de128..0e23d71 100644
--- a/src/backend/access/hash/hashovfl.c
+++ b/src/backend/access/hash/hashovfl.c
@@ -131,6 +131,7 @@ _hash_addovflpage(Relation rel, Buffer metabuf, Buffer buf, bool retain_pin)
 	uint32		i,
 				j;
 	bool		page_found = false;
+	bool		metap_dirty = false;
 
 	/*
 	 * Write-lock the tail page.  Here, we need to maintain locking order such
@@ -341,9 +342,10 @@ found:
 			metap->hashm_mapp[metap->hashm_nmaps] = BufferGetBlockNumber(newmapbuf);
 			metap->hashm_nmaps++;
 			metap->hashm_spares[splitnum]++;
-			MarkBufferDirty(metabuf);
 		}
 
+		metap_dirty = true;
+
 		/*
 		 * for new overflow page, we don't need to explicitly set the bit in
 		 * bitmap page, as by default that will be set to "in use".
@@ -357,9 +359,12 @@ found:
 	if (metap->hashm_firstfree == orig_firstfree)
 	{
 		metap->hashm_firstfree = bit + 1;
-		MarkBufferDirty(metabuf);
+		metap_dirty = true;
 	}
 
+	if (metap_dirty)
+		MarkBufferDirty(metabuf);
+
 	/* initialize new overflow page */
 	ovflpage = BufferGetPage(ovflbuf);
 	ovflopaque = (HashPageOpaque) PageGetSpecialPointer(ovflpage);

Robert Haas

robertmhaas@gmail.com

almost 8 years ago

In reply to: Amit Kapila (#5)

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

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

Amit Kapila

amit.kapila16@gmail.com

almost 8 years ago

In reply to: Robert Haas (#6)

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

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