Skip to content

Conversation

SeleznevPavel
Copy link

The size of the key used in the hash table is sizeof(pgspHashKey) = 24.

The hash is calculated for 24 bytes, but the key consists of Oid + Oid + queryid_t + uint32 = 4 + 4 + 8 + 4 = 20 bytes. Memory in pgsp_store is allocated on the stack, uninitialized and filled for 20 bytes, respectively, garbage remains in the remaining 4 bytes.

This leads to the fact that different hashes were obtained from the same key, so there was no query aggregation - each time a new entry was added to the hash table.

@SeleznevPavel
Copy link
Author

@rjuju can i ask you to do a review?

@rjuju rjuju self-assigned this Jan 17, 2025
@rjuju rjuju self-requested a review January 17, 2025 19:27
Copy link
Collaborator

@rjuju rjuju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I agree with the bugfix, just a couple of minor modifications for consistency.


Assert(plan != NULL && queryId != PGSP_NO_QUERYID);

memset(&key, 0, sizeof(pgspHashKey));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ifdef'd for pg11+ as until pg10 it was still a uint32, since the rest of the code correctly handle the version change.

if (plan_storage == PLAN_STORAGE_SHMEM)
memcpy(SHMEM_PLAN_PTR(entry), shorten_plan, plan_len + 1);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could just remove this line as there is another newline just after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants