Hi,
On 11/16/22 9:12 PM, Andres Freund wrote:
> Hi,
>
> On 2022-11-15 10:48:40 +0100, Drouvot, Bertrand wrote:
>> diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
>> index ae3365d917..be7f175bf1 100644
>> --- a/src/backend/utils/adt/pgstatfuncs.c
>> +++ b/src/backend/utils/adt/pgstatfuncs.c
>> @@ -36,24 +36,34 @@
>>
>> #define HAS_PGSTAT_PERMISSIONS(role) (has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS) ||
has_privs_of_role(GetUserId(),role))
>>
>> +#define PGSTAT_FETCH_STAT_ENTRY(entry, stat_name) ((entry == NULL) ? 0 : (int64) (entry->stat_name))
>> +
>> Datum
>> -pg_stat_get_numscans(PG_FUNCTION_ARGS)
>> +pg_stat_get_index_numscans(PG_FUNCTION_ARGS)
>> {
>> Oid relid = PG_GETARG_OID(0);
>> int64 result;
>> - PgStat_StatTabEntry *tabentry;
>> + PgStat_StatIndEntry *indentry = pgstat_fetch_stat_indentry(relid);
>>
>> - if ((tabentry = pgstat_fetch_stat_tabentry(relid)) == NULL)
>> - result = 0;
>> - else
>> - result = (int64) (tabentry->numscans);
>> + result = PGSTAT_FETCH_STAT_ENTRY(indentry, numscans);
>>
>> PG_RETURN_INT64(result);
>> }
>
> This still leaves a fair bit of boilerplate. ISTM that the function body
> really should just be a single line.
>
> Might even be worth defining the whole function via a macro. Perhaps something like
>
> PGSTAT_DEFINE_REL_FIELD_ACCESSOR(PGSTAT_KIND_INDEX, pg_stat_get_index, numscans);
Thanks for the feedback!
Right, what about something like the following?
"
#define PGSTAT_FETCH_STAT_ENTRY(pgstat_entry_kind, pgstat_fetch_stat_function, relid, stat_name) \
do { \
pgstat_entry_kind *entry = pgstat_fetch_stat_function(relid); \
PG_RETURN_INT64(entry == NULL ? 0 : (int64) (entry->stat_name)); \
} while (0)
Datum
pg_stat_get_index_numscans(PG_FUNCTION_ARGS)
{
PGSTAT_FETCH_STAT_ENTRY(PgStat_StatIndEntry, pgstat_fetch_stat_indentry, PG_GETARG_OID(0), numscans);
}
"
>
> I think the logic to infer which DB oid to use for a stats entry could be
> shared between different kinds of stats. We don't need to duplicate it.
>
Agree, will provide a new patch version once [1] is committed.
> Is there any reason to not replace the "double lookup" in
> pgstat_fetch_stat_tabentry() with IsSharedRelation()?
>
>
Thanks for the suggestion!
> This should probably be done in a preparatory commit.
Proposal submitted in [1].
[1]:
https://www.postgresql.org/message-id/flat/2e4a0ae1-2696-9f0c-301c-2330e447133f%40gmail.com#e47bf5d2902121461b61ed47413628fc
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
