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
