Re: Higher level questions around shared memory stats

At Wed, 30 Mar 2022 17:09:44 -0700, Andres Freund <andres@anarazel.de> wrote in 
> Hi,
> 
> On 2022-03-30 16:35:50 -0700, Andres Freund wrote:
> > On 2022-03-29 12:17:27 -0700, Andres Freund wrote:
> > > Separate from the minutia in [1] I'd like to discuss a few questions of more
> > > general interest. I'll post another question or two later.
> >
> > 4) What to do with the stats_temp_directory GUC / PG_STAT_TMP_DIR define /
> >    pg_stats_temp directory?
> >
> >    With shared memory stats patch, the stats system itself doesn't need it
> >    anymore. But pg_stat_statements also uses PG_STAT_TMP_DIR to store
> >    pgss_query_texts.stat. That file can be fairly hot, so there's benefit in
> >    having something like stats_temp_directory.
> >
> >    I'm inclined to just leave the guc / define / directory around, with a
> >    note saying that it's just used by extensions?
> 
> I had searched before on codesearch.debian.net whether there are external
> extensions using it, without finding one (just a copy of pgstat.h). Now I
> searched using https://cs.github.com/ ([1]) and found
> 
> https://github.com/powa-team/pg_sortstats
> https://github.com/uptimejp/sql_firewall
> https://github.com/legrandlegrand/pg_stat_sql_plans
> https://github.com/ossc-db/pg_store_plans
> 
> Which seems to weigh in favor of at least keeping the directory and
> define. They all don't seem to use the guc, but just PG_STAT_TMP_DIR.

The varialbe is not being officially exposed to extensions not even in
the core. That is, it is defined (non-static) in guc.c but does not
appear in header files. I'm not sure why pg_stat_statements decided to
use PG_STAT_TMP_DIR instead of trying to use stats_temp_directory
(also known in the core as pgstast_temp_directory). I guess all
extensions above are just following the pg_stat_statements' practice.
At least pg_store_plans does.

After moving to shared stats, we might want to expose the GUC variable
itself. Then hide/remove the macro PG_STAT_TMP_DIR.  This breaks the
extensions but it is better than keeping using PG_STAT_TMP_DIR for
uncertain reasons. The existence of the macro can be used as the
marker of the feature change.  This is the chance to break the (I
think) bad practice shared among the extensions.  At least I am okay
with that.

#ifdef PG_STAT_TMP_DIR
#define PGSP_TEXT_FILE    PG_STAT_TMP_DIR "/pgsp_plan_texts.stat"
#endif

> We currently have code removing files both in pg_stat and the configured
> pg_stats_temp directory (defaulting to pg_stat_tmp). All files matching
> global.(stat|tmp), db_[0-9]+.(tmp|stat) are removed.
> 
> With the shared memory stats patch there's only a single file, so we don't
> need that anymore. I guess some extension could rely on files being removed
> somehow, but it's hard to believe, because it'd conflict with the stats
> collector's files.

Yes, I intentionally avoided using the file names that are removed by
the logic in pg_store_plans.  But, it is a kind of rare to use such
names inadvertently, though..

> Greetings,
> 
> Andres Freund
> 
> 
> [1]
https://cs.github.com/?scopeName=All+repos&scope=&q=PG_STAT_TMP_DIR+NOT+path%3Afilemap.c+NOT+path%3Apgstat.h+NOT+path%3Abasebackup.c+NOT+path%3Apg_stat_statements.c+NOT+path%3Aguc.c

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center