Thread: BUG #1502: hash_seq_search might return removed entry
The following bug has been logged online: Bug reference: 1502 Logged by: Thomas Email address: thhal@mailblocks.com PostgreSQL version: 8.0.1 Operating system: N/A Description: hash_seq_search might return removed entry Details: The hash_seq_search keeps track of what element that it should return next in a HASH_SEQ_STATUS struct when it peruses a bucket. Removing that element from the table won't change anything since the struct remains unaffected. It still holds onto that element and hence, will return it on next iteration.
"Thomas" <thhal@mailblocks.com> writes:
> The hash_seq_search keeps track of what element that it should return next
> in a HASH_SEQ_STATUS struct when it peruses a bucket. Removing that element
> from the table won't change anything since the struct remains unaffected. It
> still holds onto that element and hence, will return it on next iteration.
This isn't a bug; it's the designed way for it to work. It's up to
callers to avoid causing a problem.
regards, tom lane
Tom Lane wrote: >"Thomas" <thhal@mailblocks.com> writes: > > >>The hash_seq_search keeps track of what element that it should return next >>in a HASH_SEQ_STATUS struct when it peruses a bucket. Removing that element >>from the table won't change anything since the struct remains unaffected. It >>still holds onto that element and hence, will return it on next iteration. >> >> > >This isn't a bug; it's the designed way for it to work. It's up to >callers to avoid causing a problem. > > This report origins from the hackers thread "SPI_finish and RegisterExprContextCallback" where you indicated that my report did not properly describe a problem. Since my report was correct and since you stated that "hash_seq_search() shouldn't return any already-dropped entries." I was led me to believe that it was *not* designed to do that. Anyway, the AtCommit_Portals doesn't avoid this problem and the end result for me is the same regardless of where the error is. I can't drop portals using a ExprContextCallback and I'd like to know the best way to fix it. I can contribute a patch but I want you to decide what needs to be fixed. Regards, Thomas Hallgren
Thomas Hallgren wrote: > Tom Lane wrote: > > >"Thomas" <thhal@mailblocks.com> writes: > > > > > >>The hash_seq_search keeps track of what element that it should return next > >>in a HASH_SEQ_STATUS struct when it peruses a bucket. Removing that element > >>from the table won't change anything since the struct remains unaffected. It > >>still holds onto that element and hence, will return it on next iteration. > >> > >> > > > >This isn't a bug; it's the designed way for it to work. It's up to > >callers to avoid causing a problem. > > > > > This report origins from the hackers thread "SPI_finish and > RegisterExprContextCallback" where you indicated that my report did not > properly describe a problem. Since my report was correct and since you > stated that "hash_seq_search() shouldn't return any already-dropped > entries." I was led me to believe that it was *not* designed to do that. > > Anyway, the AtCommit_Portals doesn't avoid this problem and the end > result for me is the same regardless of where the error is. I can't drop > portals using a ExprContextCallback and I'd like to know the best way to > fix it. I can contribute a patch but I want you to decide what needs to > be fixed. Does this need a C comment addition? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian wrote: > Thomas Hallgren wrote: > >>Tom Lane wrote: >> >> >>>"Thomas" <thhal@mailblocks.com> writes: >>> >>> >>> >>>>The hash_seq_search keeps track of what element that it should return next >>>>in a HASH_SEQ_STATUS struct when it peruses a bucket. Removing that element >>> >>>>from the table won't change anything since the struct remains unaffected. It >>> >>>>still holds onto that element and hence, will return it on next iteration. >>>> >>>> >>> >>>This isn't a bug; it's the designed way for it to work. It's up to >>>callers to avoid causing a problem. >>> >>> >> >>This report origins from the hackers thread "SPI_finish and >>RegisterExprContextCallback" where you indicated that my report did not >>properly describe a problem. Since my report was correct and since you >>stated that "hash_seq_search() shouldn't return any already-dropped >>entries." I was led me to believe that it was *not* designed to do that. >> >>Anyway, the AtCommit_Portals doesn't avoid this problem and the end >>result for me is the same regardless of where the error is. I can't drop >>portals using a ExprContextCallback and I'd like to know the best way to >>fix it. I can contribute a patch but I want you to decide what needs to >>be fixed. > > > Does this need a C comment addition? > I'm thinking of providing a remedy for my problem that does the following. 1. The AtCommit_Portals will start with building a fixed size array of pointers to portals that is the result of an iteration of the hash table. 2. Next, it will iterate over this array and for each pointer check if that pointer is NULL. If not, do what's done during the iteration today. 3. The method that deletes a portal from the hash-table will also sequentially scan the pointer array and set the pointer to NULL if found. The remedy is based on the fact that it's unlikely that you have a large amount of portals in the hash table. Would such a patch be accepted? Regards, Thomas Hallgren
Thomas Hallgren <thhal@mailblocks.com> writes:
> Would such a patch be accepted?
Seems like a brute-force solution. I'd look first at whether
AtCommit_Portals could just restart its hashtable scan after
each deletion; if that seems too inefficient, modify the hash
table entries themselves to carry a "portal already deleted"
flag.
regards, tom lane
Tom Lane wrote: >Thomas Hallgren <thhal@mailblocks.com> writes: > > >>Would such a patch be accepted? >> >> > >Seems like a brute-force solution. I'd look first at whether >AtCommit_Portals could just restart its hashtable scan after >each deletion; if that seems too inefficient, modify the hash >table entries themselves to carry a "portal already deleted" >flag. > > Yes, a static flag indicating that a deletion has occured will work fine since all portals that has not been perused but not dropped now has an InvalidSubTransactionId. I'll do it that way then. Regards, Thomas Hallgren