Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry

Hi,

On 2/10/23 10:46 PM, Andres Freund wrote:
> Hi,
>
> On 2023-02-09 11:38:18 +0100, Drouvot, Bertrand wrote:
>> Please find attached a patch proposal for $SUBJECT.
>>
>> The idea has been raised in [1] by Andres: it would allow to simplify even more the work done to
>> generate pg_stat_get_xact*() functions with Macros.
>
> Thanks!
>

Thanks for looking at it!

> I think this is useful beyond being able to generate those functions with
> macros. The fact that we had to deal with transactional code in pgstatfuncs.c
> meant that a lot of the relevant itnernals had to be exposed "outside" pgstat,
> which seems architecturally not great.
>

Right, good point.

>> Indeed, with the reconciliation done in find_tabstat_entry() then all the pg_stat_get_xact*() functions
>> (making use of find_tabstat_entry()) now "look the same" (should they take into account live subtransactions or not).
>
> I'm not bothered by making all of pg_stat_get_xact* functions more expensive,
> they're not a hot code path. But if we need to, we could just add a parameter
> to find_tabstat_entry() indicating whether we need to reconcile or not.
>

I think that's a good idea to avoid doing extra work if not needed.
V2 adds such a bool.

>> /* save stats for this function, later used to compensate for recursion */
>> - fcu->save_f_total_time = pending->f_counts.f_total_time;
>> + fcu->save_f_total_time = pending->f_total_time;
>>
>> /* save current backend-wide total time */
>> fcu->save_total = total_func_time;
>
> The diff is noisy due to all the mechanical changes like the above. Could that
> be split into a separate commit?
>

Fully agree, the PgStat_BackendFunctionEntry stuff will be done in a separate patch.

>
>> find_tabstat_entry(Oid rel_id)
>> {
>> PgStat_EntryRef *entry_ref;
>> + PgStat_TableStatus *tablestatus = NULL;
>>
>> entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
>> if (!entry_ref)
>> entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
>>
>> if (entry_ref)
>> - return entry_ref->pending;
>> - return NULL;
>> + {
>> + PgStat_TableStatus *tabentry = (PgStat_TableStatus *) entry_ref->pending;
>
> I'd add an early return for the !entry_ref case, that way you don't need to
> indent the bulk of the function.
>

Good point, done in V2.

>
>> + PgStat_TableXactStatus *trans;
>> +
>> + tablestatus = palloc(sizeof(PgStat_TableStatus));
>> + memcpy(tablestatus, tabentry, sizeof(PgStat_TableStatus));
>
> For things like this I'd use
> *tablestatus = *tabentry;
>
> that way the compiler will warn you about mismatching types, and you don't
> need the sizeof().
>
>

Good point, done in V2.

>> + /* live subtransactions' counts aren't in t_counts yet */
>> + for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
>> + {
>> + tablestatus->t_counts.t_tuples_inserted += trans->tuples_inserted;
>> + tablestatus->t_counts.t_tuples_updated += trans->tuples_updated;
>> + tablestatus->t_counts.t_tuples_deleted += trans->tuples_deleted;
>> + }
>> + }
>
> Hm, why do we end uup with t_counts still being used here, but removed other
> places?
>

t_counts are not removed, maybe you are confused with the "f_counts" that were removed
in V1 due to the PgStat_BackendFunctionEntry related changes?

>
>> diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
>> index 6737493402..40a6fbf871 100644
>> --- a/src/backend/utils/adt/pgstatfuncs.c
>> +++ b/src/backend/utils/adt/pgstatfuncs.c
>> @@ -1366,7 +1366,10 @@ pg_stat_get_xact_numscans(PG_FUNCTION_ARGS)
>> if ((tabentry = find_tabstat_entry(relid)) == NULL)
>> result = 0;
>> else
>> + {
>> result = (int64) (tabentry->t_counts.t_numscans);
>> + pfree(tabentry);
>> + }
>>
>> PG_RETURN_INT64(result);
>> }
>
> I don't think we need to bother with individual pfrees in this path. The
> caller will call the function in a dedicated memory context, that'll be reset
> very soon after this.

Oh right, the palloc is done in the ExprContext memory context that is reset soon after.
Removing the pfrees in V2 attached.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com