BUG #1502: hash_seq_search might return removed entry

"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