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