On Sat, Mar 06, 2021 at 06:56:49PM +0100, Magnus Hagander wrote: > On Sun, Dec 27, 2020 at 9:39 AM Julien Rouhaud <rjuju...@gmail.com> wrote: > > > - * > - * Right now, this structure contains no padding. If you add any, make sure > - * to teach pgss_store() to zero the padding bytes. Otherwise, things will > - * break, because pgss_hash is created using HASH_BLOBS, and thus tag_hash > - * is used to hash this. > > I don't think removing this comment completely is a good idea. Right > now it ends up there, yes, but eventually it might reach the same > state again. I think it's better to reword it based on the current > situation while keeping the part about it having to be zeroed for > padding. And maybe along with a comment at the actual memset() sites > as well?
Agreed, I'll take care of that. > AFAICT, it's going to store two copies of the query in the query text > file (pgss_query_texts.stat)? Can we find a way around that? Maybe by > looking up the entry with the flag set to the other value, and then > reusing that? Yes I was a bit worried about the possible extra text entry. I kept things simple until now as the general opinion was that entries existing for both top level and nested level should be very rare so adding extra code and overhead to spare a few query texts wouldn't be worth it. I think that using a flag might be a bit expensive, as we would have to make sure that at least one of the possible two entries has it. So if there are 2 entries and the one with the flag is evicted, we would have to transfer the flag to the other one, and check the existence of the flag when allocatin a new entry. And all of that has to be done holding an exclusive lock on pgss->lock. Maybe having a new hash table (without the toplevel flag) for those query text might be better, or maybe pgss performance is already so terrible when you have to regularly evict entries that it wouldn't make any real difference. Should I try to add some extra code to make sure that we only store the query text once, or should I document that there might be duplicate, but we expect that to be very rare?
