At Wed, 30 Mar 2022 17:09:44 -0700, Andres Freund <and...@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
