Re: Enhancing Memory Context Statistics Reporting

On 2025-02-03 21:47, Rahila Syed wrote:
> Hi,
> 
>>> 
>>> Just idea; as an another option, how about blocking new
>> requests to
>>> the target process (e.g., causing them to fail with an error
>> or
>>> returning NULL with a warning) if a previous request is still
>> pending?
>>> Users can simply retry the request if it fails. IMO failing
>> quickly
>>> seems preferable to getting stuck for a while in cases with
>> concurrent
>>> requests.
>>> 
>>> Thank you for the suggestion. I agree that it is better to fail
>>> early and avoid waiting for a timeout in such cases. I will add a
>>> "pending request" tracker for this in shared memory. This approach
>> 
>>> will help prevent sending a concurrent request if a request for
>> the
>>> same backend is still being processed.
>>> 
> 
> Please find attached a patch that adds a request_pending field in
> shared memory. This allows us to detect concurrent requests early
> and return a WARNING message immediately, avoiding unnecessary
> waiting and potential timeouts. This is added in v12-0002* patch.

Thanks for updating the patch!

The below comments would be a bit too detailed at this stage, but I’d 
like to share the points I noticed.


>  76 +        arguments: PID and a boolean, get_summary. The function 
> can send

Since get_summary is a parameter, should we enclose it in <parameter> 
tags, like <parameter>get_summary</parameter>?

> 387 + * The shared memory buffer has a limited size - it the process 
> has too many
> 388 + * memory contexts,

Should 'it' be 'if'?

> 320  * By default, only superusers are allowed to signal to return the 
> memory
> 321  * contexts because allowing any users to issue this request at an 
> unbounded
> 322  * rate would cause lots of requests to be sent and which can lead 
> to denial of
> 323  * service. Additional roles can be permitted with GRANT.

This comment seems to contradict the following code:

> 360      * Only superusers or users with pg_read_all_stats privileges 
> can view the
> 361      * memory context statistics of another process
> 362      */
> 363     if (!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
> 364         ereport(ERROR,
> 365                 (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> 366                  errmsg("memory context statistics privilege 
> error")));


> 485 +   if (memCtxState[procNumber].memstats_dsa_handle == 
> DSA_HANDLE_INVALID)
> 486 +   {
> 487 +
> 488 +       LWLockRelease(&memCtxState[procNumber].lw_lock);

> 505 +   else
> 506 +   {
> 507 +       LWLockRelease(&memCtxState[procNumber].lw_lock);

The LWLockRelease() function appears in both the if and else branches. 
Can we move it outside the conditional block to avoid duplication?

  > 486 +   {
  > 487 +
  > 488 +       LWLockRelease(&memCtxState[procNumber].lw_lock);

The blank line at 487 seems unnecessary. Should we remove it?

> 534         {
> 535             ereport(LOG,
> 536                     (errmsg("Wait for %d process to publish stats 
> timed out, trying again",
> 537                             pid)));
> 538             if (num_retries > MAX_RETRIES)
> 539                 goto end;
> 540             num_retries = num_retries + 1;
> 541         }

If the target process remains unresponsive, the logs will repeatedly 
show:

   LOG:  Wait for xxxx process to publish stats timed out, trying again
   LOG:  Wait for xxxx process to publish stats timed out, trying again
   ...
   LOG:  Wait for xxxx process to publish stats timed out, trying again

However, the final log message is misleading because it does not 
actually try again. Should we adjust the last log message to reflect the 
correct behavior?

> 541         }
> 542
> 543     }

The blank line at 542 seems unnecessary. Should we remove it?

> 874 +   context_id_lookup = 
> hash_create("pg_get_remote_backend_memory_contexts",

Should 'pg_get_remote_backend_memory_contexts' be renamed to 
'pg_get_process_memory_contexts' now?


> 899 +    * Allocate memory in this process's dsa for storing statistics 
> of the the

'the the' is a duplicate.


-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.