public inbox for [email protected]help / color / mirror / Atom feed
Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc? 7+ messages / 4 participants [nested] [flat]
* Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc? @ 2023-08-30 08:57 Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Daniel Gustafsson @ 2023-08-30 08:57 UTC (permalink / raw) To: Tom Lane <[email protected]>; Andres Freund <[email protected]>; +Cc: pgsql-hackers; Thomas Munro <[email protected]>; Justin Pryzby <[email protected]>; Nazir Bilal Yavuz <[email protected]> > On 28 Aug 2023, at 14:32, Daniel Gustafsson <[email protected]> wrote: > Attached is a patch with a quick PoC for using PQPing instead of using psql for > connection checks in pg_regress. The attached v2 fixes a silly mistake which led to a compiler warning. -- Daniel Gustafsson Attachments: [application/octet-stream] v2-0001-Speed-up-pg_regress-server-testing.patch (10.2K, ../../[email protected]/2-v2-0001-Speed-up-pg_regress-server-testing.patch) download | inline diff: From 8b4fb9ea81b7e4eed8d0269962ed5aeb744f5684 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson <[email protected]> Date: Mon, 28 Aug 2023 10:54:25 +0200 Subject: [PATCH v2] Speed up pg_regress server testing Instead of connecting to the server with psql to check if it is ready for running tests, pg_regress now use PQPing which avoids performing expensive system() calls on Windows. The frequency of tests is also increased in order to connect to the server faster. This patch is part of a larger effort to make testing consume fewer resources in order to be able to fit more tests into the available CI constraints. Discussion: https://postgr.es/m/[email protected] --- src/interfaces/ecpg/test/Makefile | 3 +- src/interfaces/ecpg/test/meson.build | 2 +- src/test/isolation/Makefile | 2 +- src/test/isolation/meson.build | 2 +- src/test/regress/GNUmakefile | 4 +- src/test/regress/meson.build | 2 +- src/test/regress/pg_regress.c | 88 ++++++++++++++++++++-------- 7 files changed, 71 insertions(+), 32 deletions(-) diff --git a/src/interfaces/ecpg/test/Makefile b/src/interfaces/ecpg/test/Makefile index cf841a3a5b..ba6ca837b3 100644 --- a/src/interfaces/ecpg/test/Makefile +++ b/src/interfaces/ecpg/test/Makefile @@ -10,6 +10,7 @@ include $(top_builddir)/src/Makefile.global override CPPFLAGS := \ '-I$(top_builddir)/src/port' \ '-I$(top_srcdir)/src/test/regress' \ + '-I$(libpq_srcdir)' \ '-DHOST_TUPLE="$(host_tuple)"' \ '-DSHELLPROG="$(SHELL)"' \ $(CPPFLAGS) @@ -45,7 +46,7 @@ clean distclean maintainer-clean: all: pg_regress$(X) pg_regress$(X): pg_regress_ecpg.o $(WIN32RES) $(top_builddir)/src/test/regress/pg_regress.o - $(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@ + $(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@ $(top_builddir)/src/test/regress/pg_regress.o: $(MAKE) -C $(dir $@) $(notdir $@) diff --git a/src/interfaces/ecpg/test/meson.build b/src/interfaces/ecpg/test/meson.build index 04c6819a79..b7a3fb4e0e 100644 --- a/src/interfaces/ecpg/test/meson.build +++ b/src/interfaces/ecpg/test/meson.build @@ -18,7 +18,7 @@ pg_regress_ecpg = executable('pg_regress_ecpg', pg_regress_ecpg_sources, c_args: pg_regress_cflags, include_directories: [pg_regress_inc, include_directories('.')], - dependencies: [frontend_code], + dependencies: [frontend_code, libpq], kwargs: default_bin_args + { 'install': false }, diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile index b8738b7c1b..e99602ae52 100644 --- a/src/test/isolation/Makefile +++ b/src/test/isolation/Makefile @@ -38,7 +38,7 @@ pg_regress.o: | submake-regress rm -f $@ && $(LN_S) $(top_builddir)/src/test/regress/pg_regress.o . pg_isolation_regress$(X): isolation_main.o pg_regress.o $(WIN32RES) - $(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@ + $(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@ isolationtester$(X): $(OBJS) | submake-libpq submake-libpgport $(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@ diff --git a/src/test/isolation/meson.build b/src/test/isolation/meson.build index a4439e8ad0..e6ebe62c1e 100644 --- a/src/test/isolation/meson.build +++ b/src/test/isolation/meson.build @@ -35,7 +35,7 @@ pg_isolation_regress = executable('pg_isolation_regress', isolation_sources, c_args: pg_regress_cflags, include_directories: pg_regress_inc, - dependencies: frontend_code, + dependencies: [frontend_code, libpq], kwargs: default_bin_args + { 'install_dir': dir_pgxs / 'src/test/isolation', }, diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile index 38c3a1f85b..db0687f867 100644 --- a/src/test/regress/GNUmakefile +++ b/src/test/regress/GNUmakefile @@ -36,11 +36,11 @@ EXTRADEFS = '-DHOST_TUPLE="$(host_tuple)"' \ all: pg_regress$(X) pg_regress$(X): pg_regress.o pg_regress_main.o $(WIN32RES) | submake-libpgport - $(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@ + $(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@ # dependencies ensure that path changes propagate pg_regress.o: pg_regress.c $(top_builddir)/src/port/pg_config_paths.h -pg_regress.o: override CPPFLAGS += -I$(top_builddir)/src/port $(EXTRADEFS) +pg_regress.o: override CPPFLAGS += -I$(top_builddir)/src/port -I$(libpq_srcdir) $(EXTRADEFS) # note: because of the submake dependency, this rule's action is really a no-op $(top_builddir)/src/port/pg_config_paths.h: | submake-libpgport diff --git a/src/test/regress/meson.build b/src/test/regress/meson.build index a045c00c1f..f0dfd85591 100644 --- a/src/test/regress/meson.build +++ b/src/test/regress/meson.build @@ -30,7 +30,7 @@ endif pg_regress = executable('pg_regress', regress_sources, c_args: pg_regress_cflags, - dependencies: [frontend_code], + dependencies: [frontend_code, libpq], kwargs: default_bin_args + { 'install_dir': dir_pgxs / 'src/test/regress', }, diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index ec67588cf5..48df02156c 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -32,6 +32,7 @@ #include "common/username.h" #include "getopt_long.h" #include "lib/stringinfo.h" +#include "libpq-fe.h" #include "libpq/pqcomm.h" /* needed for UNIXSOCK_PATH() */ #include "pg_config_paths.h" #include "pg_regress.h" @@ -75,6 +76,12 @@ const char *pretty_diff_opts = "-w -U3"; */ #define TESTNAME_WIDTH 36 +/* + * The number times per second that pg_regress checks to see if the test + * instance server has started and is available for connection. + */ +#define WAIT_TICKS_PER_SECOND 20 + typedef enum TAPtype { DIAG = 0, @@ -107,6 +114,7 @@ static bool nolocale = false; static bool use_existing = false; static char *hostname = NULL; static int port = -1; +static char portstr[16]; static bool port_specified_by_user = false; static char *dlpath = PKGLIBDIR; static char *user = NULL; @@ -2107,7 +2115,10 @@ regression_main(int argc, char *argv[], int i; int option_index; char buf[MAXPGPATH * 4]; - char buf2[MAXPGPATH * 4]; + instr_time starttime; + instr_time stoptime; + + INSTR_TIME_SET_CURRENT(starttime); pg_logging_init(argv[0]); progname = get_progname(argv[0]); @@ -2296,6 +2307,9 @@ regression_main(int argc, char *argv[], const char *env_wait; int wait_seconds; const char *initdb_template_dir; + const char *keywords[4]; + const char *values[4]; + PGPing rv; /* * Prepare the temp instance @@ -2435,22 +2449,30 @@ regression_main(int argc, char *argv[], } #endif + sprintf(portstr, "%d", port); + /* - * Check if there is a postmaster running already. + * Prepare the connection params for checking the state of the server + * before starting the tests. */ - snprintf(buf2, sizeof(buf2), - "\"%s%spsql\" -X postgres <%s 2>%s", - bindir ? bindir : "", - bindir ? "/" : "", - DEVNULL, DEVNULL); + keywords[0] = "dbname"; + values[0] = "postgres"; + keywords[1] = "port"; + values[1] = portstr; + keywords[2] = "host"; + values[2] = hostname ? hostname : sockdir; + keywords[3] = NULL; + values[3] = NULL; + /* + * Check if there is a postmaster running already. + */ for (i = 0; i < 16; i++) { - fflush(NULL); - if (system(buf2) == 0) - { - char s[16]; + rv = PQpingParams(keywords, values, 1); + if (rv == PQPING_OK) + { if (port_specified_by_user || i == 15) { note("port %d apparently in use", port); @@ -2461,8 +2483,8 @@ regression_main(int argc, char *argv[], note("port %d apparently in use, trying %d", port, port + 1); port++; - sprintf(s, "%d", port); - setenv("PGPORT", s, 1); + sprintf(portstr, "%d", port); + setenv("PGPORT", portstr, 1); } else break; @@ -2485,11 +2507,11 @@ regression_main(int argc, char *argv[], bail("could not spawn postmaster: %s", strerror(errno)); /* - * Wait till postmaster is able to accept connections; normally this - * is only a second or so, but Cygwin is reportedly *much* slower, and - * test builds using Valgrind or similar tools might be too. Hence, - * allow the default timeout of 60 seconds to be overridden from the - * PGCTLTIMEOUT environment variable. + * Wait till postmaster is able to accept connections; normally takes + * only a fraction of a second or so, but Cygwin is reportedly *much* + * slower, and test builds using Valgrind or similar tools might be + * too. Hence, allow the default timeout of 60 seconds to be + * overridden from the PGCTLTIMEOUT environment variable. */ env_wait = getenv("PGCTLTIMEOUT"); if (env_wait != NULL) @@ -2501,13 +2523,24 @@ regression_main(int argc, char *argv[], else wait_seconds = 60; - for (i = 0; i < wait_seconds; i++) + for (i = 0; i < wait_seconds * WAIT_TICKS_PER_SECOND; i++) { - /* Done if psql succeeds */ - fflush(NULL); - if (system(buf2) == 0) + /* + * It's fairly unlikely that the server is responding immediately + * so we start with sleeping before checking instead of the other + * way around. + */ + pg_usleep(1000000L / WAIT_TICKS_PER_SECOND); + + rv = PQpingParams(keywords, values, 1); + + /* Done if the server is running and accepts connections */ + if (rv == PQPING_OK) break; + if (rv == PQPING_NO_ATTEMPT) + bail("attempting to connect to postmaster failed"); + /* * Fail immediately if postmaster has exited */ @@ -2520,10 +2553,8 @@ regression_main(int argc, char *argv[], bail("postmaster failed, examine \"%s/log/postmaster.log\" for the reason", outputdir); } - - pg_usleep(1000000L); } - if (i >= wait_seconds) + if (i >= wait_seconds * WAIT_TICKS_PER_SECOND) { diag("postmaster did not respond within %d seconds, examine \"%s/log/postmaster.log\" for the reason", wait_seconds, outputdir); @@ -2582,6 +2613,13 @@ regression_main(int argc, char *argv[], create_role(sl->str, dblist); } + /* + * Report how much time we spent during instance setup. + */ + INSTR_TIME_SET_CURRENT(stoptime); + INSTR_TIME_SUBTRACT(stoptime, starttime); + diag("Time to first test: %.0f ms", INSTR_TIME_GET_MILLISEC(stoptime)); + /* * Ready to run the tests */ -- 2.32.1 (Apple Git-133) ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc? @ 2023-09-12 23:49 Andres Freund <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 2 replies; 7+ messages in thread From: Andres Freund @ 2023-09-12 23:49 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Justin Pryzby <[email protected]>; Nazir Bilal Yavuz <[email protected]> Hi, On 2023-08-30 10:57:10 +0200, Daniel Gustafsson wrote: > > On 28 Aug 2023, at 14:32, Daniel Gustafsson <[email protected]> wrote: > > > Attached is a patch with a quick PoC for using PQPing instead of using psql for > > connection checks in pg_regress. > > The attached v2 fixes a silly mistake which led to a compiler warning. Still seems like a good idea to me. To see what impact it has, I measured the time running the pg_regress tests that take less than 6s on my machine - I excluded the slower ones (like the main regression tests) because they'd hide any overall difference. ninja && m test --suite setup --no-rebuild && tests=$(m test --no-rebuild --list|grep -E '/regress'|grep -vE '(regress|postgres_fdw|test_integerset|intarray|amcheck|test_decoding)/regress'|cut -d' ' -f 3) && time m test --no-rebuild $tests Time for: master: cassert: real 0m5.265s user 0m8.422s sys 0m8.381s optimized: real 0m4.926s user 0m6.356s sys 0m8.263s my patch (probing every 100ms with psql): cassert: real 0m3.465s user 0m8.827s sys 0m8.579s optimized: real 0m2.932s user 0m6.596s sys 0m8.458s Daniel's (probing every 50ms with PQping()): cassert: real 0m3.347s user 0m8.373s sys 0m8.354s optimized: real 0m2.527s user 0m6.156s sys 0m8.315s My patch increased user/sys time a bit (likely due to a higher number of futile psql forks), but Daniel's doesn't. And it does show a nice overall wall clock time saving. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc? @ 2023-09-18 13:12 Daniel Gustafsson <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 0 replies; 7+ messages in thread From: Daniel Gustafsson @ 2023-09-18 13:12 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Justin Pryzby <[email protected]>; Nazir Bilal Yavuz <[email protected]> > On 13 Sep 2023, at 01:49, Andres Freund <[email protected]> wrote: > On 2023-08-30 10:57:10 +0200, Daniel Gustafsson wrote: >>> On 28 Aug 2023, at 14:32, Daniel Gustafsson <[email protected]> wrote: >> >>> Attached is a patch with a quick PoC for using PQPing instead of using psql for >>> connection checks in pg_regress. >> >> The attached v2 fixes a silly mistake which led to a compiler warning. > > Still seems like a good idea to me. To see what impact it has, I measured the > time running the pg_regress tests that take less than 6s on my machine - I > excluded the slower ones (like the main regression tests) because they'd hide > any overall difference. > My patch increased user/sys time a bit (likely due to a higher number of > futile psql forks), but Daniel's doesn't. And it does show a nice overall wall > clock time saving. While it does add a lib dependency I think it's worth doing, so I propose we go ahead with this for master. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc? @ 2023-10-24 20:25 Daniel Gustafsson <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Daniel Gustafsson @ 2023-10-24 20:25 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Justin Pryzby <[email protected]>; Nazir Bilal Yavuz <[email protected]> > On 13 Sep 2023, at 01:49, Andres Freund <[email protected]> wrote: > My patch increased user/sys time a bit (likely due to a higher number of > futile psql forks), but Daniel's doesn't. And it does show a nice overall wall > clock time saving. I went ahead and applied this on master, thanks for review! Now to see if there will be any noticeable difference in resource usage. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc? @ 2023-10-24 20:34 Tom Lane <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Tom Lane @ 2023-10-24 20:34 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Justin Pryzby <[email protected]>; Nazir Bilal Yavuz <[email protected]> Daniel Gustafsson <[email protected]> writes: > I went ahead and applied this on master, thanks for review! Now to see if > there will be any noticeable difference in resource usage. I think that tools like Coverity are likely to whine about your use of sprintf instead of snprintf. Sure, it's perfectly safe, but that won't stop the no-sprintf-ever crowd from complaining. regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc? @ 2023-10-24 20:45 Daniel Gustafsson <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Daniel Gustafsson @ 2023-10-24 20:45 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Justin Pryzby <[email protected]>; Nazir Bilal Yavuz <[email protected]> > On 24 Oct 2023, at 22:34, Tom Lane <[email protected]> wrote: > > Daniel Gustafsson <[email protected]> writes: >> I went ahead and applied this on master, thanks for review! Now to see if >> there will be any noticeable difference in resource usage. > > I think that tools like Coverity are likely to whine about your > use of sprintf instead of snprintf. Sure, it's perfectly safe, > but that won't stop the no-sprintf-ever crowd from complaining. Fair point, that's probably quite likely to happen. I can apply an snprintf() conversion change like this in the two places introduced by this: - sprintf(s, "%d", port); + sprintf(s, sizeof(s), "%d", port); -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v16 2/2] handle relation statistics correctly during rewrites @ 2026-05-18 15:20 Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Bertrand Drouvot @ 2026-05-18 15:20 UTC (permalink / raw) This patch ensures that relation statistics are preserved correctly when a relation is rewritten (e.g., during CLUSTER, VACUUM FULL, ALTER TABLE operations that require a table rewrite, or TRUNCATE). Since patch 0001 keys PGSTAT_KIND_RELATION by the physical RelFileLocator rather than the relation OID, a rewrite (which assigns a new relfilenode) would lose the accumulated statistics if not handled specially. The approach is: 1. When a rewrite occurs (RelationSetNewRelfilenumber or swap_relation_files), record the old and new locators via pgstat_mark_rewrite(). 2. During pgstat_assoc_relation(), if the relation's new locator matches a pending rewrite, continue using the stats entry associated with the old locator. This ensures stats accumulated after the rewrite are still tracked in the original entry. 3. At transaction commit (AtEOXact_PgStat_Relations), process pending rewrites: flush any pending stats for the old locator, copy them to the new locator's stats entry, and drop the old entry. 4. At subtransaction abort, remove rewrites from the aborted nesting level. 5. For two-phase transactions, the rewrite information is recorded in the TwoPhasePgStatRecord and replayed at commit/abort time. This also handles chained rewrites (multiple rewrites of the same relation in a single transaction) by tracking the original locator through the chain. --- src/backend/catalog/index.c | 2 +- src/backend/commands/repack.c | 5 + src/backend/commands/tablecmds.c | 6 + src/backend/utils/activity/pgstat_relation.c | 392 ++++++++++++++++++- src/backend/utils/activity/pgstat_xact.c | 25 +- src/backend/utils/cache/relcache.c | 6 + src/include/pgstat.h | 5 +- src/tools/pgindent/typedefs.list | 1 + 8 files changed, 425 insertions(+), 17 deletions(-) 92.8% src/backend/utils/activity/ 4.8% src/backend/ diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 9407c357f27..335ff1e50f6 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1813,7 +1813,7 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName) changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId); /* copy over statistics from old to new index */ - pgstat_copy_relation_stats(newClassRel, oldClassRel); + pgstat_copy_relation_stats(newClassRel->rd_locator, oldClassRel->rd_locator, false); /* Copy data of pg_statistic from the old index to the new one */ CopyStatistics(oldIndexId, newIndexId); diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 351a3cc32e8..eda4135bcd1 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -1633,6 +1633,11 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, rel1 = relation_open(r1, NoLock); rel2 = relation_open(r2, NoLock); + + /* Mark that a rewrite happened */ + if (RELKIND_HAS_STORAGE(rel1->rd_rel->relkind)) + pgstat_mark_rewrite(rel1->rd_locator, rel2->rd_locator); + rel2->rd_createSubid = rel1->rd_createSubid; rel2->rd_newRelfilelocatorSubid = rel1->rd_newRelfilelocatorSubid; rel2->rd_firstRelfilelocatorSubid = rel1->rd_firstRelfilelocatorSubid; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 92b0f38c353..b553b9a4970 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -17129,6 +17129,7 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode) Oid reltoastrelid; RelFileNumber newrelfilenumber; RelFileLocator newrlocator; + RelFileLocator oldrlocator; List *reltoastidxids = NIL; ListCell *lc; @@ -17167,6 +17168,7 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode) newrlocator = rel->rd_locator; newrlocator.relNumber = newrelfilenumber; newrlocator.spcOid = newTableSpace; + oldrlocator = rel->rd_locator; /* hand off to AM to actually create new rel storage and copy the data */ if (rel->rd_rel->relkind == RELKIND_INDEX) @@ -17179,6 +17181,10 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode) table_relation_copy_data(rel, &newrlocator); } + /* mark that a rewrite happened */ + if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind)) + pgstat_mark_rewrite(oldrlocator, newrlocator); + /* * Update the pg_class row. * diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c index 7fcda7f7518..18d1b9fc59d 100644 --- a/src/backend/utils/activity/pgstat_relation.c +++ b/src/backend/utils/activity/pgstat_relation.c @@ -30,6 +30,19 @@ #include "utils/syscache.h" #include "utils/timestamp.h" +/* Pending rewrite operations for stats copying */ +typedef struct PgStat_PendingRewrite +{ + RelFileLocator old_locator; + RelFileLocator new_locator; + RelFileLocator original_locator; + int nest_level; /* Transaction nesting level where rewrite + * occurred */ + struct PgStat_PendingRewrite *next; +} PgStat_PendingRewrite; + +/* The pending rewrites list for current transaction */ +static PgStat_PendingRewrite *pending_rewrites = NULL; /* Record that's written to 2PC state file when pgstat state is persisted */ typedef struct TwoPhasePgStatRecord @@ -43,6 +56,8 @@ typedef struct TwoPhasePgStatRecord PgStat_Counter deleted_pre_truncdrop; RelFileLocator locator; /* table's rd_locator */ bool truncdropped; /* was the relation truncated/dropped? */ + RelFileLocator rewrite_old_locator; + int rewrite_nest_level; } TwoPhasePgStatRecord; @@ -54,27 +69,71 @@ static void restore_truncdrop_counters(PgStat_TableXactStatus *trans); /* - * Copy stats between relations. This is used for things like REINDEX + * Copy stats between RelFileLocator. This is used for things like REINDEX * CONCURRENTLY. */ void -pgstat_copy_relation_stats(Relation dst, Relation src) +pgstat_copy_relation_stats(RelFileLocator dst, RelFileLocator src, bool increment) { PgStat_StatTabEntry *srcstats; PgStatShared_Relation *dstshstats; PgStat_EntryRef *dst_ref; - srcstats = pgstat_fetch_stat_tabentry_ext(RelationGetRelid(src), NULL); + srcstats = (PgStat_StatTabEntry *) pgstat_fetch_entry(PGSTAT_KIND_RELATION, + src.dbOid, + RelFileLocatorToPgStatObjid(src), + NULL); if (!srcstats) return; dst_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_RELATION, - dst->rd_rel->relisshared ? InvalidOid : MyDatabaseId, - RelationGetRelid(dst), + dst.dbOid, + RelFileLocatorToPgStatObjid(dst), false); dstshstats = (PgStatShared_Relation *) dst_ref->shared_stats; - dstshstats->stats = *srcstats; + + if (!increment) + dstshstats->stats = *srcstats; + else + { + /* Increment those statistics */ +#define RELFSTAT_ACC(fld, stats_to_add) \ + (dstshstats->stats.fld += stats_to_add->fld) + RELFSTAT_ACC(numscans, srcstats); + RELFSTAT_ACC(tuples_returned, srcstats); + RELFSTAT_ACC(tuples_fetched, srcstats); + RELFSTAT_ACC(tuples_inserted, srcstats); + RELFSTAT_ACC(tuples_updated, srcstats); + RELFSTAT_ACC(tuples_deleted, srcstats); + RELFSTAT_ACC(tuples_hot_updated, srcstats); + RELFSTAT_ACC(tuples_newpage_updated, srcstats); + RELFSTAT_ACC(live_tuples, srcstats); + RELFSTAT_ACC(dead_tuples, srcstats); + RELFSTAT_ACC(mod_since_analyze, srcstats); + RELFSTAT_ACC(ins_since_vacuum, srcstats); + RELFSTAT_ACC(blocks_fetched, srcstats); + RELFSTAT_ACC(blocks_hit, srcstats); + RELFSTAT_ACC(vacuum_count, srcstats); + RELFSTAT_ACC(autovacuum_count, srcstats); + RELFSTAT_ACC(analyze_count, srcstats); + RELFSTAT_ACC(autoanalyze_count, srcstats); + RELFSTAT_ACC(total_vacuum_time, srcstats); + RELFSTAT_ACC(total_autovacuum_time, srcstats); + RELFSTAT_ACC(total_analyze_time, srcstats); + RELFSTAT_ACC(total_autoanalyze_time, srcstats); +#undef RELFSTAT_ACC + + /* Replace those statistics */ +#define RELFSTAT_REP(fld, stats_to_rep) \ + (dstshstats->stats.fld = stats_to_rep->fld) + RELFSTAT_REP(lastscan, srcstats); + RELFSTAT_REP(last_vacuum_time, srcstats); + RELFSTAT_REP(last_autovacuum_time, srcstats); + RELFSTAT_REP(last_analyze_time, srcstats); + RELFSTAT_REP(last_autoanalyze_time, srcstats); +#undef RELFSTAT_REP + } pgstat_unlock_entry(dst_ref); } @@ -136,6 +195,7 @@ void pgstat_assoc_relation(Relation rel) { RelFileLocator locator; + PgStat_TableStatus *pgstat_info; Assert(rel->pgstat_enabled); Assert(rel->pgstat_info == NULL); @@ -164,14 +224,54 @@ pgstat_assoc_relation(Relation rel) locator.relNumber = rel->rd_id; } + /* + * If this relation was rewritten during the current transaction we may be + * reopening it with its new RelFileLocator. In that case, continue using + * the stats entry associated with the old locator rather than creating a + * new one. This ensures all stats from before and after the rewrite are + * tracked in a single entry which will be properly copied to the new + * locator at transaction commit. + */ + if (pending_rewrites != NULL) + { + PgStat_PendingRewrite *rewrite; + + for (rewrite = pending_rewrites; rewrite != NULL; rewrite = rewrite->next) + { + if (locator.dbOid == rewrite->new_locator.dbOid && + locator.spcOid == rewrite->new_locator.spcOid && + locator.relNumber == rewrite->new_locator.relNumber) + { + pgstat_info = pgstat_prep_relation_pending(rewrite->old_locator); + goto found_entry; + } + } + } + /* Else find or make the PgStat_TableStatus entry, and update link */ - rel->pgstat_info = pgstat_prep_relation_pending(locator); + pgstat_info = pgstat_prep_relation_pending(locator); + +found_entry: + rel->pgstat_info = pgstat_info; + + /* + * For relations stats, we key by physical file location, not by relation + * OID. This means during operations like ALTER TYPE it's possible that + * the relation OID changes but the relfilenode stays the same (no actual + * rewrite needed). Unlink the old relation first. + */ + if (pgstat_info->relation != NULL && + pgstat_info->relation != rel) + { + pgstat_info->relation->pgstat_info = NULL; + pgstat_info->relation = NULL; + } /* don't allow link a stats to multiple relcache entries */ - Assert(rel->pgstat_info->relation == NULL); + Assert(pgstat_info->relation == NULL); /* mark this relation as the owner */ - rel->pgstat_info->relation = rel; + pgstat_info->relation = rel; } /* @@ -214,14 +314,37 @@ pgstat_drop_relation(Relation rel) { int nest_level = GetCurrentTransactionNestLevel(); PgStat_TableStatus *pgstat_info; + bool skip_transactional_drop = false; /* don't track stats for relations without storage */ if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind)) return; - pgstat_drop_transactional(PGSTAT_KIND_RELATION, - rel->rd_locator.dbOid, - RelFileLocatorToPgStatObjid(rel->rd_locator)); + /* Check if this drop is part of a pending rewrite */ + if (pending_rewrites != NULL) + { + PgStat_PendingRewrite *rewrite; + + for (rewrite = pending_rewrites; rewrite != NULL; rewrite = rewrite->next) + { + if (rel->rd_locator.dbOid == rewrite->old_locator.dbOid && + rel->rd_locator.spcOid == rewrite->old_locator.spcOid && + rel->rd_locator.relNumber == rewrite->old_locator.relNumber) + { + skip_transactional_drop = true; + break; + } + } + } + + /* + * If it is part of a rewrite, drop its stats later, for example in + * AtEOXact_PgStat_Relations(), so skip it here. + */ + if (!skip_transactional_drop) + pgstat_drop_transactional(PGSTAT_KIND_RELATION, + rel->rd_locator.dbOid, + RelFileLocatorToPgStatObjid(rel->rd_locator)); if (!pgstat_should_count_relation(rel)) return; @@ -668,6 +791,48 @@ AtEOXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit) } tabstat->trans = NULL; } + + /* preserve the stats in case of rewrite */ + if (isCommit && pending_rewrites != NULL) + { + PgStat_PendingRewrite *rewrite; + PgStat_PendingRewrite *prev = NULL; + PgStat_PendingRewrite *current = pending_rewrites; + PgStat_PendingRewrite *next; + + /* reverse the rewrites list to process in chronological order */ + while (current != NULL) + { + next = current->next; + current->next = prev; + prev = current; + current = next; + } + + /* now process rewrites in chronological order */ + for (rewrite = prev; rewrite != NULL; rewrite = rewrite->next) + { + PgStat_EntryRef *old_entry_ref; + + old_entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, + rewrite->old_locator.dbOid, + RelFileLocatorToPgStatObjid(rewrite->old_locator)); + + if (old_entry_ref && old_entry_ref->pending) + pgstat_relation_flush_cb(old_entry_ref, false); + + pgstat_copy_relation_stats(rewrite->new_locator, + rewrite->old_locator, true); + + /* drop old locator's stats */ + if (!pgstat_drop_entry(PGSTAT_KIND_RELATION, + rewrite->old_locator.dbOid, + RelFileLocatorToPgStatObjid(rewrite->old_locator))) + pgstat_request_entry_refs_gc(); + } + } + + pending_rewrites = NULL; } /* @@ -683,6 +848,30 @@ AtEOSubXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit, in PgStat_TableXactStatus *trans; PgStat_TableXactStatus *next_trans; + /* + * If we don't commit then remove the associated rewrites if any, to keep + * the rewrite chain in sync with what will be eventually committed. + */ + if (!isCommit) + { + PgStat_PendingRewrite **rewrite_ptr = &pending_rewrites; + + while (*rewrite_ptr != NULL) + { + if ((*rewrite_ptr)->nest_level >= nestDepth) + { + PgStat_PendingRewrite *to_remove = *rewrite_ptr; + + *rewrite_ptr = (*rewrite_ptr)->next; + pfree(to_remove); + } + else + { + rewrite_ptr = &((*rewrite_ptr)->next); + } + } + } + for (trans = xact_state->first; trans != NULL; trans = next_trans) { PgStat_TableStatus *tabstat; @@ -762,11 +951,19 @@ void AtPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state) { PgStat_TableXactStatus *trans; + PgStat_PendingRewrite *rewrite; + /* + * For each tabstat, find its matching rewrite and remove it from the + * pending rewrites list. This way, after processing all tabstats, pending + * rewrites will only contain rewrite only transactions. + */ for (trans = xact_state->first; trans != NULL; trans = trans->next) { PgStat_TableStatus *tabstat PG_USED_FOR_ASSERTS_ONLY; TwoPhasePgStatRecord record; + PgStat_PendingRewrite **rewrite_ptr; + bool found_rewrite = false; Assert(trans->nest_level == 1); Assert(trans->upper == NULL); @@ -786,10 +983,83 @@ AtPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state) record.locator = tabstat->locator; record.truncdropped = trans->truncdropped; + record.rewrite_nest_level = 0; + + /* + * Look for a matching rewrite and remove it from pending rewrites. We + * check three possible matches: + * + * The new_locator when stats have been added after the rewrite. The + * old_locator when stats have been added before the rewrite but not + * after. The original_locator when this tabstat is part of a rewrite + * chain. + */ + rewrite_ptr = &pending_rewrites; + while (*rewrite_ptr != NULL) + { + rewrite = *rewrite_ptr; + + if ((record.locator.dbOid == rewrite->new_locator.dbOid && + record.locator.spcOid == rewrite->new_locator.spcOid && + record.locator.relNumber == rewrite->new_locator.relNumber) || + (tabstat->locator.dbOid == rewrite->old_locator.dbOid && + tabstat->locator.spcOid == rewrite->old_locator.spcOid && + tabstat->locator.relNumber == rewrite->old_locator.relNumber) || + (tabstat->locator.dbOid == rewrite->original_locator.dbOid && + tabstat->locator.spcOid == rewrite->original_locator.spcOid && + tabstat->locator.relNumber == rewrite->original_locator.relNumber)) + { + /* + * Found matching rewrite. Record the rewrite information and + * remove this rewrite from the list since it's now handled. + */ + record.rewrite_old_locator = rewrite->original_locator; + record.rewrite_nest_level = rewrite->nest_level; + record.locator = rewrite->new_locator; + found_rewrite = true; + + /* Remove from pending_rewrites list */ + *rewrite_ptr = rewrite->next; + pfree(rewrite); + break; + } + else + { + /* Move to next rewrite in the list */ + rewrite_ptr = &(rewrite->next); + } + } + + /* If no rewrite found, clear the rewrite fields */ + if (!found_rewrite) + { + memset(&record.rewrite_old_locator, 0, sizeof(RelFileLocator)); + } + + RegisterTwoPhaseRecord(TWOPHASE_RM_PGSTAT_ID, 0, + &record, sizeof(TwoPhasePgStatRecord)); + } + + /* + * Now process any rewrites still pending. These are rewrite only + * transactions. We need to preserve their stats even though there's no + * tabstat entry for them. + */ + for (rewrite = pending_rewrites; rewrite != NULL; rewrite = rewrite->next) + { + TwoPhasePgStatRecord record; + + memset(&record, 0, sizeof(TwoPhasePgStatRecord)); + record.locator = rewrite->new_locator; + record.rewrite_old_locator = rewrite->original_locator; + record.rewrite_nest_level = rewrite->nest_level; + record.truncdropped = false; RegisterTwoPhaseRecord(TWOPHASE_RM_PGSTAT_ID, 0, &record, sizeof(TwoPhasePgStatRecord)); } + + pending_rewrites = NULL; } /* @@ -812,6 +1082,8 @@ PostPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state) tabstat = trans->parent; tabstat->trans = NULL; } + + pending_rewrites = NULL; } /* @@ -847,6 +1119,29 @@ pgstat_twophase_postcommit(FullTransactionId fxid, uint16 info, pgstat_info->counts.changed_tuples += rec->tuples_inserted + rec->tuples_updated + rec->tuples_deleted; + + if (rec->rewrite_nest_level > 0) + { + PgStat_EntryRef *old_entry_ref; + + /* Flush any pending stats for old locator first */ + old_entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, + rec->rewrite_old_locator.dbOid, + RelFileLocatorToPgStatObjid(rec->rewrite_old_locator)); + + if (old_entry_ref && old_entry_ref->pending) + pgstat_relation_flush_cb(old_entry_ref, false); + + /* Copy stats from old to new locator */ + pgstat_copy_relation_stats(rec->locator, rec->rewrite_old_locator, + true); + + /* Drop old locator's stats */ + if (!pgstat_drop_entry(PGSTAT_KIND_RELATION, + rec->rewrite_old_locator.dbOid, + RelFileLocatorToPgStatObjid(rec->rewrite_old_locator))) + pgstat_request_entry_refs_gc(); + } } /* @@ -861,9 +1156,26 @@ pgstat_twophase_postabort(FullTransactionId fxid, uint16 info, { TwoPhasePgStatRecord *rec = (TwoPhasePgStatRecord *) recdata; PgStat_TableStatus *pgstat_info; + RelFileLocator target_locator; + + /* + * For aborted transactions with rewrites (like TRUNCATE), we need to + * restore stats to the old locator, not the new one. The new locator + * should be dropped since the rewrite is being rolled back. + */ + if (rec->rewrite_nest_level > 0) + { + /* Use the old locator */ + target_locator = rec->rewrite_old_locator; + } + else + { + /* No rewrite, use the original locator */ + target_locator = rec->locator; + } /* Find or create a tabstat entry for the target locator */ - pgstat_info = pgstat_prep_relation_pending(rec->locator); + pgstat_info = pgstat_prep_relation_pending(target_locator); /* Same math as in AtEOXact_PgStat, abort case */ if (rec->truncdropped) @@ -918,7 +1230,17 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) tabentry->numscans += lstats->counts.numscans; if (lstats->counts.numscans) { - TimestampTz t = GetCurrentTransactionStopTimestamp(); + TimestampTz t; + + /* + * Checking the transaction state due to the flush call in + * pgstat_twophase_postcommit() that would break the assertion on the + * state in GetCurrentTransactionStopTimestamp(). + */ + if (!IsTransactionState()) + t = GetCurrentTransactionStopTimestamp(); + else + t = GetCurrentTimestamp(); if (t > tabentry->lastscan) tabentry->lastscan = t; @@ -1169,3 +1491,45 @@ pgstat_reloid_to_relfilelocator(Oid reloid, RelFileLocator *locator) ReleaseSysCache(tuple); return result; } + +/* + * Mark that a relation rewrite has occurred, preserving the original locator + * so stats can be copied at transaction commit. + */ +void +pgstat_mark_rewrite(RelFileLocator old_locator, RelFileLocator new_locator) +{ + PgStat_PendingRewrite *rewrite; + PgStat_PendingRewrite *existing; + RelFileLocator original_locator = old_locator; + + for (existing = pending_rewrites; existing != NULL; existing = existing->next) + { + if (old_locator.dbOid == existing->new_locator.dbOid && + old_locator.spcOid == existing->new_locator.spcOid && + old_locator.relNumber == existing->new_locator.relNumber) + { + original_locator = existing->original_locator; + break; + } + } + + /* Allocate in TopTransactionContext memory context */ + rewrite = MemoryContextAlloc(TopTransactionContext, + sizeof(PgStat_PendingRewrite)); + + rewrite->old_locator = old_locator; + rewrite->new_locator = new_locator; + rewrite->original_locator = original_locator; + rewrite->nest_level = GetCurrentTransactionNestLevel(); + + /* Add to the list */ + rewrite->next = pending_rewrites; + pending_rewrites = rewrite; +} + +void +pgstat_clear_rewrite(void) +{ + pending_rewrites = NULL; +} diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/pgstat_xact.c index 5e2d69e6297..8ed8f5317f3 100644 --- a/src/backend/utils/activity/pgstat_xact.c +++ b/src/backend/utils/activity/pgstat_xact.c @@ -55,6 +55,8 @@ AtEOXact_PgStat(bool isCommit, bool parallel) } pgStatXactStack = NULL; + pgstat_clear_rewrite(); + /* Make sure any stats snapshot is thrown away */ pgstat_clear_snapshot(); } @@ -360,8 +362,29 @@ create_drop_transactional_internal(PgStat_Kind kind, Oid dboid, uint64 objid, bo void pgstat_create_transactional(PgStat_Kind kind, Oid dboid, uint64 objid) { - if (pgstat_get_entry_ref(kind, dboid, objid, false, NULL)) + PgStat_EntryRef *entry_ref; + + entry_ref = pgstat_get_entry_ref(kind, dboid, objid, false, NULL); + + if (entry_ref) { + /* + * For relations stats, we key by physical file location, not by + * relation OID. This means during operations like ALTER TYPE where + * the relation OID changes but the relfilenode stays the same (no + * actual rewrite needed), we'll find an existing entry. + * + * This is expected behavior, we want to preserve stats across the + * catalog change. Simply reset and recreate the entry for the new + * relation OID without warning. + */ + if (kind == PGSTAT_KIND_RELATION) + { + pgstat_reset(kind, dboid, objid); + create_drop_transactional_internal(kind, dboid, objid, true); + return; + } + ereport(WARNING, errmsg("resetting existing statistics for kind %s, db=%u, oid=%" PRIu64, (pgstat_get_kind_info(kind))->name, dboid, diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 0572ab424e7..d84058db102 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -87,6 +87,7 @@ #include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" +#include "utils/pgstat_internal.h" #include "utils/relmapper.h" #include "utils/resowner.h" #include "utils/snapmgr.h" @@ -3782,6 +3783,7 @@ RelationSetNewRelfilenumber(Relation relation, char persistence) MultiXactId minmulti = InvalidMultiXactId; TransactionId freezeXid = InvalidTransactionId; RelFileLocator newrlocator; + RelFileLocator oldrlocator = relation->rd_locator; if (!IsBinaryUpgrade) { @@ -3953,6 +3955,10 @@ RelationSetNewRelfilenumber(Relation relation, char persistence) table_close(pg_class, RowExclusiveLock); + /* Mark that a rewrite happened */ + if (RELKIND_HAS_STORAGE(relation->rd_rel->relkind)) + pgstat_mark_rewrite(oldrlocator, newrlocator); + /* * Make the pg_class row change or relation map change visible. This will * cause the relcache entry to get updated, too. diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 941a39bd16e..5344ece9ecf 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -697,7 +697,7 @@ extern PgStat_FunctionCounts *find_funcstat_entry(Oid func_id); extern void pgstat_create_relation(Relation rel); extern void pgstat_drop_relation(Relation rel); -extern void pgstat_copy_relation_stats(Relation dst, Relation src); +extern void pgstat_copy_relation_stats(RelFileLocator dst, RelFileLocator src, bool increment); extern void pgstat_init_relation(Relation rel); extern void pgstat_assoc_relation(Relation rel); @@ -709,6 +709,9 @@ extern void pgstat_report_vacuum(Relation rel, PgStat_Counter livetuples, extern void pgstat_report_analyze(Relation rel, PgStat_Counter livetuples, PgStat_Counter deadtuples, bool resetcounter, TimestampTz starttime); +extern void pgstat_mark_rewrite(RelFileLocator old_locator, + RelFileLocator new_locator); +extern void pgstat_clear_rewrite(void); /* * If stats are enabled, but pending data hasn't been prepared yet, call diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 8cf40c87043..1544f54e918 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2349,6 +2349,7 @@ PgStat_LockEntry PgStat_PendingDroppedStatsItem PgStat_PendingIO PgStat_PendingLock +PgStat_PendingRewrite PgStat_SLRUStats PgStat_ShmemControl PgStat_Snapshot -- 2.34.1 --Uc+t0mng6H5VV2zk-- ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-05-18 15:20 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-08-30 08:57 Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc? Daniel Gustafsson <[email protected]> 2023-09-12 23:49 ` Andres Freund <[email protected]> 2023-09-18 13:12 ` Daniel Gustafsson <[email protected]> 2023-10-24 20:25 ` Daniel Gustafsson <[email protected]> 2023-10-24 20:34 ` Tom Lane <[email protected]> 2023-10-24 20:45 ` Daniel Gustafsson <[email protected]> 2026-05-18 15:20 [PATCH v16 2/2] handle relation statistics correctly during rewrites Bertrand Drouvot <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox