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

Thanks for the new version.

At Mon, 13 Feb 2023 09:58:52 +0100, "Drouvot, Bertrand" <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote in
> Hi,
>
> On 2/13/23 8:40 AM, Kyotaro Horiguchi wrote:
> > At Mon, 13 Feb 2023 08:09:50 +0100, "Drouvot, Bertrand"
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote in
> >>> 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.
> > Agreed.
> >
> >> Removing the pfrees in V2 attached.
> > Ah, that sound good.
> > if (!entry_ref)
> > + {
> > entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION,
> > InvalidOid, rel_id);
> > + return tablestatus;
> > + }
> > We should return something if the call returns a non-null value?
>
> What we do is: if entry_ref is NULL then we return NULL (so that the
> caller returns 0).
>
> If entry_ref is not NULL then we return a copy of entry_ref->pending
> (with or without subtrans).

Isn't it ignoring the second call to pgstat_fetch_pending_entry?

What the code did is: if entry_ref is NULL for MyDatabaseId then we
*retry* fetching an global (not database-wise) entry. If any global
entry is found, return it (correctly entry_ref->pending) to the caller.

The current patch returns NULL when a glboal entry is found.

I thought that we might be able to return entry_ref->pending since the
callers don't call pfree on the returned pointer, but it is not great
that we don't inform the callers if the returned memory can be safely
pfreed or not.

Thus what I have in mind is the following.

> if (!entry_ref)
> + {
> entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION,
> InvalidOid, rel_id);
> + if (!entry_ref)
> + return NULL;
> + }

> > So, since we want to hide the internal from pgstatfuncs, the
> > additional flag should be gone.
>
> I think there is pros and cons for both but I don't have a strong
> opinion about that.
>
> So also proposing V3 attached without the flag in case this is the
> preferred approach.

That part looks good to me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center