> On 26 Mar 2025, at 11:34, Rahila Syed <rahilasyed90@gmail.com> wrote:
> + ereport(LOG,
> + errmsg("hash table corrupted, can't construct path value"));
> I know you switched from elog(LOG.. to ereport(LOG.. but I still think a LOG
> entry stating corruption isn't helpful, it's not actionable for the user.
> Given that it's a case that shouldn't happen I wonder if we should downgrade it
> to an Assert(false) and potentially a DEBUG1?
>
> How about changing it to ERROR, in accordance with current occurrences of the
> same message? I did it in the attached version, however I am open to changing
> it to an Assert(false) and DEBUG1.
In the attached I moved it to an elog() as it's an internal error, and spending
translation effort on it seems fruitless.
> 1. Eliminated the unnecessary creation of an extra memory context before calling hash_create.
> The hash_create function already generates a memory context containing the hash table,
> enabling easy memory deallocation by simply deleting the context via hash_destroy.
> Therefore, the patch relies on hash_destroy for memory management instead of manual freeing.
Nice
> 2. Optimized memory usage by storing the path as an array of integers rather than as an array of
> Datums.
> This approach conserves DSA memory allocated for storing this information.
Ah yes, much better.
The attached v21 has a few improvements:
* The function documentation didn't specify the return type, only the fact that
it's setof record. I've added all output columns.
* Some general cleaups of the docs with better markup, improved xref linking
and various rewording.
* Comment cleanups and language alignment
* Added a missing_ok parameter to ContextTypeToString(). While all callers are
fine with unknown context types, if we introduce an API for this it seems
prudent to not place that burden on callers but to take it on in the function.
* Renamed get_summary to just summary, and num_of_tries to retries which feels
more in line with the naming convention in other functions
* Deferred calling InitMaterializedSRF() until after the PID has been checked
for validity.
* Pulled back the timeout to 500msec from 1 second. In running congested
pgbench simulations I saw better performance and improved results in getting stats.
* Replaced strncpy with strlcpy and consistently used idlen to keep all length
calculations equal.
* Fixed misspelled param name in pg_proc.dat
* Pulled back maximum memory usage from 8Mb to 1Mb. 8Mb for the duration of a
process (once allocated) is a lot for a niche feature and I while I'm still not
sure 1Mb is the right value I think from experimentation that it's closer.
I think this version is close to a committable state, will spend a little more
time testing, polishing and rewriting the commit message. I will also play
around with placement within the memory context code files to keep it from
making backpatch issues.
--
Daniel Gustafsson
