public inbox for [email protected]  

help / color / mirror / Atom feed

From: Andres Freund <[email protected]>
To: Drouvot, Bertrand <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
Date: Fri, 10 Feb 2023 13:46:19 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

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!

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.


> 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.


>  	/* 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?


>  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.


> +		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().


> +		/* 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?


> 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.

Greetings,

Andres Freund

view thread (5+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox