On Tue, Jan 13, 2026 at 6:39 AM John Naylor <[email protected]> wrote: > It looks like it would be helpful if fasthash_accum_cstring just > returned zero when given a NULL string, as in the attached. We could > also do something like add a large number to the hash, but I'm not > sure that's necessary.
I think that pg_plan_advice's requirement are unusual here, so I would suggest not adding a branch to fasthash_accum_cstring. If this were a requirement that almost every caller had, then it would make sense to pay the cost in the common function. But there will probably be a small performance cost to this, hash function are often called in hot paths, and pg_plan_advice is, I think, unusual, so I don't really like doing it given thoe facts. > + /* > + * hashfn_unstable.h recommends using string length as tweak. It's not > + * clear to me what to do if there are multiple strings, so for now I'm > + * just using the total of all of the lengths. > + */ > + return fasthash_final32(&hs, sp_len); > > Sounds reasonable, so the patch also documents that. Some kind of comment change here seems useful to me. I wonder whether it should be generalized even more than this statement. I also wonder if this is really the optimal strategy. But I definitely agree that clarifying this in whatever way makes sense is a good idea. -- Robert Haas EDB: http://www.enterprisedb.com
