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.