agora inbox for [email protected]help / color / mirror / Atom feed
[PATCH v1 1/2] Allow IndexScanDesc allocation in caller-specified context 38+ messages / 8 participants [nested] [flat]
* [PATCH v1 1/2] Allow IndexScanDesc allocation in caller-specified context @ 2018-10-30 07:56 amit <[email protected]> 0 siblings, 0 replies; 38+ messages in thread From: amit @ 2018-10-30 07:56 UTC (permalink / raw) --- src/backend/access/index/genam.c | 6 ++++-- src/backend/access/index/indexam.c | 21 +++++++++++++++------ src/backend/commands/cluster.c | 3 ++- src/backend/executor/execIndexing.c | 3 ++- src/backend/executor/execReplication.c | 2 +- src/backend/executor/nodeIndexonlyscan.c | 3 ++- src/backend/executor/nodeIndexscan.c | 6 ++++-- src/backend/utils/adt/selfuncs.c | 4 ++-- src/include/access/genam.h | 3 ++- src/include/access/relscan.h | 6 ++++++ 10 files changed, 40 insertions(+), 17 deletions(-) diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c index 9d08775687..0744b00d15 100644 --- a/src/backend/access/index/genam.c +++ b/src/backend/access/index/genam.c @@ -370,7 +370,8 @@ systable_beginscan(Relation heapRelation, } sysscan->iscan = index_beginscan(heapRelation, irel, - snapshot, nkeys, 0); + snapshot, nkeys, 0, + CurrentMemoryContext); index_rescan(sysscan->iscan, key, nkeys, NULL, 0); sysscan->scan = NULL; } @@ -572,7 +573,8 @@ systable_beginscan_ordered(Relation heapRelation, } sysscan->iscan = index_beginscan(heapRelation, indexRelation, - snapshot, nkeys, 0); + snapshot, nkeys, 0, + CurrentMemoryContext); index_rescan(sysscan->iscan, key, nkeys, NULL, 0); sysscan->scan = NULL; diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c index eade540ef5..b17ad9417b 100644 --- a/src/backend/access/index/indexam.c +++ b/src/backend/access/index/indexam.c @@ -125,7 +125,8 @@ do { \ static IndexScanDesc index_beginscan_internal(Relation indexRelation, int nkeys, int norderbys, Snapshot snapshot, - ParallelIndexScanDesc pscan, bool temp_snap); + ParallelIndexScanDesc pscan, bool temp_snap, + MemoryContext scan_cxt); /* ---------------------------------------------------------------- @@ -222,11 +223,13 @@ IndexScanDesc index_beginscan(Relation heapRelation, Relation indexRelation, Snapshot snapshot, - int nkeys, int norderbys) + int nkeys, int norderbys, + MemoryContext scan_cxt) { IndexScanDesc scan; - scan = index_beginscan_internal(indexRelation, nkeys, norderbys, snapshot, NULL, false); + scan = index_beginscan_internal(indexRelation, nkeys, norderbys, snapshot, + NULL, false, scan_cxt); /* * Save additional parameters into the scandesc. Everything else was set @@ -251,7 +254,8 @@ index_beginscan_bitmap(Relation indexRelation, { IndexScanDesc scan; - scan = index_beginscan_internal(indexRelation, nkeys, 0, snapshot, NULL, false); + scan = index_beginscan_internal(indexRelation, nkeys, 0, snapshot, + NULL, false, CurrentMemoryContext); /* * Save additional parameters into the scandesc. Everything else was set @@ -268,9 +272,11 @@ index_beginscan_bitmap(Relation indexRelation, static IndexScanDesc index_beginscan_internal(Relation indexRelation, int nkeys, int norderbys, Snapshot snapshot, - ParallelIndexScanDesc pscan, bool temp_snap) + ParallelIndexScanDesc pscan, bool temp_snap, + MemoryContext scan_cxt) { IndexScanDesc scan; + MemoryContext oldcxt; RELATION_CHECKS; CHECK_REL_PROCEDURE(ambeginscan); @@ -286,11 +292,14 @@ index_beginscan_internal(Relation indexRelation, /* * Tell the AM to open a scan. */ + oldcxt = MemoryContextSwitchTo(scan_cxt); scan = indexRelation->rd_amroutine->ambeginscan(indexRelation, nkeys, norderbys); /* Initialize information for parallel scan. */ scan->parallel_scan = pscan; scan->xs_temp_snap = temp_snap; + scan->xs_scan_cxt = scan_cxt; + MemoryContextSwitchTo(oldcxt); return scan; } @@ -504,7 +513,7 @@ index_beginscan_parallel(Relation heaprel, Relation indexrel, int nkeys, snapshot = RestoreSnapshot(pscan->ps_snapshot_data); RegisterSnapshot(snapshot); scan = index_beginscan_internal(indexrel, nkeys, norderbys, snapshot, - pscan, true); + pscan, true, CurrentMemoryContext); /* * Save additional parameters into the scandesc. Everything else was set diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 68be470977..2c5c23a145 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -926,7 +926,8 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose, if (OldIndex != NULL && !use_sort) { heapScan = NULL; - indexScan = index_beginscan(OldHeap, OldIndex, SnapshotAny, 0, 0); + indexScan = index_beginscan(OldHeap, OldIndex, SnapshotAny, 0, 0, + CurrentMemoryContext); index_rescan(indexScan, NULL, 0, NULL, 0); } else diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index 9927ad70e6..74d15e6b43 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -719,7 +719,8 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index, retry: conflict = false; found_self = false; - index_scan = index_beginscan(heap, index, &DirtySnapshot, indnkeyatts, 0); + index_scan = index_beginscan(heap, index, &DirtySnapshot, indnkeyatts, 0, + CurrentMemoryContext); index_rescan(index_scan, scankeys, indnkeyatts, NULL, 0); while ((tup = index_getnext(index_scan, diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 25ba93e03c..432785c7d1 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -132,7 +132,7 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid, InitDirtySnapshot(snap); scan = index_beginscan(rel, idxrel, &snap, IndexRelationGetNumberOfKeyAttributes(idxrel), - 0); + 0, CurrentMemoryContext); /* Build scan key. */ build_replindex_scan_key(skey, rel, idxrel, searchslot); diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index daedf342f7..8b654a9eee 100644 --- a/src/backend/executor/nodeIndexonlyscan.c +++ b/src/backend/executor/nodeIndexonlyscan.c @@ -91,7 +91,8 @@ IndexOnlyNext(IndexOnlyScanState *node) node->ioss_RelationDesc, estate->es_snapshot, node->ioss_NumScanKeys, - node->ioss_NumOrderByKeys); + node->ioss_NumOrderByKeys, + CurrentMemoryContext); node->ioss_ScanDesc = scandesc; diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c index ba7821b0e2..750b455e66 100644 --- a/src/backend/executor/nodeIndexscan.c +++ b/src/backend/executor/nodeIndexscan.c @@ -114,7 +114,8 @@ IndexNext(IndexScanState *node) node->iss_RelationDesc, estate->es_snapshot, node->iss_NumScanKeys, - node->iss_NumOrderByKeys); + node->iss_NumOrderByKeys, + CurrentMemoryContext); node->iss_ScanDesc = scandesc; @@ -220,7 +221,8 @@ IndexNextWithReorder(IndexScanState *node) node->iss_RelationDesc, estate->es_snapshot, node->iss_NumScanKeys, - node->iss_NumOrderByKeys); + node->iss_NumOrderByKeys, + CurrentMemoryContext); node->iss_ScanDesc = scandesc; diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index e0ece74bb9..16f7cc3641 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -5582,7 +5582,7 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata, */ index_scan = index_beginscan(heapRel, indexRel, &SnapshotNonVacuumable, - 1, 0); + 1, 0, CurrentMemoryContext); index_rescan(index_scan, scankeys, 1, NULL, 0); /* Fetch first tuple in sortop's direction */ @@ -5615,7 +5615,7 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata, { index_scan = index_beginscan(heapRel, indexRel, &SnapshotNonVacuumable, - 1, 0); + 1, 0, CurrentMemoryContext); index_rescan(index_scan, scankeys, 1, NULL, 0); /* Fetch first tuple in reverse direction */ diff --git a/src/include/access/genam.h b/src/include/access/genam.h index 534fac7bf2..676eb2b752 100644 --- a/src/include/access/genam.h +++ b/src/include/access/genam.h @@ -140,7 +140,8 @@ extern bool index_insert(Relation indexRelation, extern IndexScanDesc index_beginscan(Relation heapRelation, Relation indexRelation, Snapshot snapshot, - int nkeys, int norderbys); + int nkeys, int norderbys, + MemoryContext scan_cxt); extern IndexScanDesc index_beginscan_bitmap(Relation indexRelation, Snapshot snapshot, int nkeys); diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h index e5289b8aa7..169435d746 100644 --- a/src/include/access/relscan.h +++ b/src/include/access/relscan.h @@ -139,6 +139,12 @@ typedef struct IndexScanDescData /* parallel index scan information, in shared memory */ ParallelIndexScanDesc parallel_scan; + + /* + * The context under which memory for this struct and its members are + * allocated. + */ + MemoryContext xs_scan_cxt; } IndexScanDescData; /* Generic structure for parallel scans */ -- 2.11.0 --------------5253696EAEF276D5B6588979 Content-Type: text/plain; charset=UTF-8; name="v1-0002-Add-a-MemoryContext-arg-to-check_exclusion_or_uni.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="v1-0002-Add-a-MemoryContext-arg-to-check_exclusion_or_uni.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 38+ messages in thread
* [PATCH v17 08/23] autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C @ 2022-09-26 21:05 Andres Freund <[email protected]> 0 siblings, 0 replies; 38+ messages in thread From: Andres Freund @ 2022-09-26 21:05 UTC (permalink / raw) There's no need for two different set of flags, as we'll only ever run on a one architecture at a time... Simplifies meson PGXS compatibility. --- src/port/Makefile | 16 ++++++++-------- config/c-compiler.m4 | 8 ++++---- configure | 19 +++++++++---------- configure.ac | 10 +++++----- src/Makefile.global.in | 3 +-- 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/src/port/Makefile b/src/port/Makefile index b3754d8940a..711f59e32bd 100644 --- a/src/port/Makefile +++ b/src/port/Makefile @@ -88,15 +88,15 @@ libpgport.a: $(OBJS) thread.o: CFLAGS+=$(PTHREAD_CFLAGS) thread_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS) -# all versions of pg_crc32c_sse42.o need CFLAGS_SSE42 -pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_SSE42) -pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_SSE42) -pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_SSE42) +# all versions of pg_crc32c_sse42.o need CFLAGS_CRC +pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_CRC) -# all versions of pg_crc32c_armv8.o need CFLAGS_ARMV8_CRC32C -pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) -pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) -pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) +# all versions of pg_crc32c_armv8.o need CFLAGS_CRC +pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_CRC) # # Shared library versions of object files diff --git a/config/c-compiler.m4 b/config/c-compiler.m4 index 000b075312e..eb8cc8ce170 100644 --- a/config/c-compiler.m4 +++ b/config/c-compiler.m4 @@ -597,7 +597,7 @@ fi])# PGAC_HAVE_GCC__ATOMIC_INT64_CAS # the other ones are, on x86-64 platforms) # # An optional compiler flag can be passed as argument (e.g. -msse4.2). If the -# intrinsics are supported, sets pgac_sse42_crc32_intrinsics, and CFLAGS_SSE42. +# intrinsics are supported, sets pgac_sse42_crc32_intrinsics, and CFLAGS_CRC. AC_DEFUN([PGAC_SSE42_CRC32_INTRINSICS], [define([Ac_cachevar], [AS_TR_SH([pgac_cv_sse42_crc32_intrinsics_$1])])dnl AC_CACHE_CHECK([for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=$1], [Ac_cachevar], @@ -613,7 +613,7 @@ AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <nmmintrin.h>], [Ac_cachevar=no]) CFLAGS="$pgac_save_CFLAGS"]) if test x"$Ac_cachevar" = x"yes"; then - CFLAGS_SSE42="$1" + CFLAGS_CRC="$1" pgac_sse42_crc32_intrinsics=yes fi undefine([Ac_cachevar])dnl @@ -629,7 +629,7 @@ undefine([Ac_cachevar])dnl # # An optional compiler flag can be passed as argument (e.g. # -march=armv8-a+crc). If the intrinsics are supported, sets -# pgac_armv8_crc32c_intrinsics, and CFLAGS_ARMV8_CRC32C. +# pgac_armv8_crc32c_intrinsics, and CFLAGS_CRC. AC_DEFUN([PGAC_ARMV8_CRC32C_INTRINSICS], [define([Ac_cachevar], [AS_TR_SH([pgac_cv_armv8_crc32c_intrinsics_$1])])dnl AC_CACHE_CHECK([for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=$1], [Ac_cachevar], @@ -647,7 +647,7 @@ AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <arm_acle.h>], [Ac_cachevar=no]) CFLAGS="$pgac_save_CFLAGS"]) if test x"$Ac_cachevar" = x"yes"; then - CFLAGS_ARMV8_CRC32C="$1" + CFLAGS_CRC="$1" pgac_armv8_crc32c_intrinsics=yes fi undefine([Ac_cachevar])dnl diff --git a/configure b/configure index 1caca21b625..80b28cb9310 100755 --- a/configure +++ b/configure @@ -645,8 +645,7 @@ MSGMERGE MSGFMT_FLAGS MSGFMT PG_CRC32C_OBJS -CFLAGS_ARMV8_CRC32C -CFLAGS_SSE42 +CFLAGS_CRC LIBOBJS ZSTD LZ4 @@ -17815,7 +17814,7 @@ fi # # First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used # with the default compiler flags. If not, check if adding the -msse4.2 -# flag helps. CFLAGS_SSE42 is set to -msse4.2 if that's required. +# flag helps. CFLAGS_CRC is set to -msse4.2 if that's required. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=" >&5 $as_echo_n "checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=... " >&6; } if ${pgac_cv_sse42_crc32_intrinsics_+:} false; then : @@ -17850,7 +17849,7 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_sse42_crc32_intrinsics_" >&5 $as_echo "$pgac_cv_sse42_crc32_intrinsics_" >&6; } if test x"$pgac_cv_sse42_crc32_intrinsics_" = x"yes"; then - CFLAGS_SSE42="" + CFLAGS_CRC="" pgac_sse42_crc32_intrinsics=yes fi @@ -17889,13 +17888,12 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_sse42_crc32_intrinsics__msse4_2" >&5 $as_echo "$pgac_cv_sse42_crc32_intrinsics__msse4_2" >&6; } if test x"$pgac_cv_sse42_crc32_intrinsics__msse4_2" = x"yes"; then - CFLAGS_SSE42="-msse4.2" + CFLAGS_CRC="-msse4.2" pgac_sse42_crc32_intrinsics=yes fi fi - # Are we targeting a processor that supports SSE 4.2? gcc, clang and icc all # define __SSE4_2__ in that case. cat confdefs.h - <<_ACEOF >conftest.$ac_ext @@ -17922,7 +17920,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext # # First check if __crc32c* intrinsics can be used with the default compiler # flags. If not, check if adding -march=armv8-a+crc flag helps. -# CFLAGS_ARMV8_CRC32C is set if the extra flag is required. +# CFLAGS_CRC is set if the extra flag is required. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=" >&5 $as_echo_n "checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=... " >&6; } if ${pgac_cv_armv8_crc32c_intrinsics_+:} false; then : @@ -17959,7 +17957,7 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_armv8_crc32c_intrinsics_" >&5 $as_echo "$pgac_cv_armv8_crc32c_intrinsics_" >&6; } if test x"$pgac_cv_armv8_crc32c_intrinsics_" = x"yes"; then - CFLAGS_ARMV8_CRC32C="" + CFLAGS_CRC="" pgac_armv8_crc32c_intrinsics=yes fi @@ -18000,13 +17998,14 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" >&5 $as_echo "$pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" >&6; } if test x"$pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" = x"yes"; then - CFLAGS_ARMV8_CRC32C="-march=armv8-a+crc" + CFLAGS_CRC="-march=armv8-a+crc" pgac_armv8_crc32c_intrinsics=yes fi fi + # Select CRC-32C implementation. # # If we are targeting a processor that has Intel SSE 4.2 instructions, we can @@ -18034,7 +18033,7 @@ if test x"$USE_SLICING_BY_8_CRC32C" = x"" && test x"$USE_SSE42_CRC32C" = x"" && USE_SSE42_CRC32C_WITH_RUNTIME_CHECK=1 else # Use ARM CRC Extension if available. - if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_ARMV8_CRC32C" = x""; then + if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_CRC" = x""; then USE_ARMV8_CRC32C=1 else # ARM CRC Extension, with runtime check? diff --git a/configure.ac b/configure.ac index 10fa55dd154..f837a602d01 100644 --- a/configure.ac +++ b/configure.ac @@ -2086,12 +2086,11 @@ fi # # First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used # with the default compiler flags. If not, check if adding the -msse4.2 -# flag helps. CFLAGS_SSE42 is set to -msse4.2 if that's required. +# flag helps. CFLAGS_CRC is set to -msse4.2 if that's required. PGAC_SSE42_CRC32_INTRINSICS([]) if test x"$pgac_sse42_crc32_intrinsics" != x"yes"; then PGAC_SSE42_CRC32_INTRINSICS([-msse4.2]) fi -AC_SUBST(CFLAGS_SSE42) # Are we targeting a processor that supports SSE 4.2? gcc, clang and icc all # define __SSE4_2__ in that case. @@ -2105,12 +2104,13 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [ # # First check if __crc32c* intrinsics can be used with the default compiler # flags. If not, check if adding -march=armv8-a+crc flag helps. -# CFLAGS_ARMV8_CRC32C is set if the extra flag is required. +# CFLAGS_CRC is set if the extra flag is required. PGAC_ARMV8_CRC32C_INTRINSICS([]) if test x"$pgac_armv8_crc32c_intrinsics" != x"yes"; then PGAC_ARMV8_CRC32C_INTRINSICS([-march=armv8-a+crc]) fi -AC_SUBST(CFLAGS_ARMV8_CRC32C) + +AC_SUBST(CFLAGS_CRC) # Select CRC-32C implementation. # @@ -2139,7 +2139,7 @@ if test x"$USE_SLICING_BY_8_CRC32C" = x"" && test x"$USE_SSE42_CRC32C" = x"" && USE_SSE42_CRC32C_WITH_RUNTIME_CHECK=1 else # Use ARM CRC Extension if available. - if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_ARMV8_CRC32C" = x""; then + if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_CRC" = x""; then USE_ARMV8_CRC32C=1 else # ARM CRC Extension, with runtime check? diff --git a/src/Makefile.global.in b/src/Makefile.global.in index d8ea2da583c..c87b6d97ca9 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -262,8 +262,7 @@ CFLAGS_SL_MODULE = @CFLAGS_SL_MODULE@ CXXFLAGS_SL_MODULE = @CXXFLAGS_SL_MODULE@ CFLAGS_UNROLL_LOOPS = @CFLAGS_UNROLL_LOOPS@ CFLAGS_VECTORIZE = @CFLAGS_VECTORIZE@ -CFLAGS_SSE42 = @CFLAGS_SSE42@ -CFLAGS_ARMV8_CRC32C = @CFLAGS_ARMV8_CRC32C@ +CFLAGS_CRC = @CFLAGS_CRC@ PERMIT_DECLARATION_AFTER_STATEMENT = @PERMIT_DECLARATION_AFTER_STATEMENT@ CXXFLAGS = @CXXFLAGS@ -- 2.37.3.542.gdd3f6c4cae --losgcde4yzp6hbli Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v17-0009-autoconf-Rely-on-ar-supporting-index-creation.patch" ^ permalink raw reply [nested|flat] 38+ messages in thread
* [PATCH v3 1/4] autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C @ 2022-10-05 19:24 Andres Freund <[email protected]> 0 siblings, 0 replies; 38+ messages in thread From: Andres Freund @ 2022-10-05 19:24 UTC (permalink / raw) Until now we emitted the cflags to build the CRC objects into architecture specific variables. That doesn't make a whole lot of sense to me - we're never going to target x86 and arm at the same time, so they don't need to be separate variables. It might be better to instead continue to have CFLAGS_SSE42 / CFLAGS_ARMV8_CRC32C be computed by PGAC_ARMV8_CRC32C_INTRINSICS / PGAC_SSE42_CRC32_INTRINSICS and then set CFLAGS_CRC based on those. But it seems unlikely that we'd need other sets of CRC specific flags for those two architectures at the same time. Discussion: https://postgr.es/m/[email protected] --- src/port/Makefile | 16 ++++++++-------- config/c-compiler.m4 | 8 ++++---- configure | 19 +++++++++---------- configure.ac | 10 +++++----- src/Makefile.global.in | 3 +-- 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/src/port/Makefile b/src/port/Makefile index b3754d8940a..711f59e32bd 100644 --- a/src/port/Makefile +++ b/src/port/Makefile @@ -88,15 +88,15 @@ libpgport.a: $(OBJS) thread.o: CFLAGS+=$(PTHREAD_CFLAGS) thread_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS) -# all versions of pg_crc32c_sse42.o need CFLAGS_SSE42 -pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_SSE42) -pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_SSE42) -pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_SSE42) +# all versions of pg_crc32c_sse42.o need CFLAGS_CRC +pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_CRC) -# all versions of pg_crc32c_armv8.o need CFLAGS_ARMV8_CRC32C -pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) -pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) -pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) +# all versions of pg_crc32c_armv8.o need CFLAGS_CRC +pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_CRC) # # Shared library versions of object files diff --git a/config/c-compiler.m4 b/config/c-compiler.m4 index 000b075312e..eb8cc8ce170 100644 --- a/config/c-compiler.m4 +++ b/config/c-compiler.m4 @@ -597,7 +597,7 @@ fi])# PGAC_HAVE_GCC__ATOMIC_INT64_CAS # the other ones are, on x86-64 platforms) # # An optional compiler flag can be passed as argument (e.g. -msse4.2). If the -# intrinsics are supported, sets pgac_sse42_crc32_intrinsics, and CFLAGS_SSE42. +# intrinsics are supported, sets pgac_sse42_crc32_intrinsics, and CFLAGS_CRC. AC_DEFUN([PGAC_SSE42_CRC32_INTRINSICS], [define([Ac_cachevar], [AS_TR_SH([pgac_cv_sse42_crc32_intrinsics_$1])])dnl AC_CACHE_CHECK([for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=$1], [Ac_cachevar], @@ -613,7 +613,7 @@ AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <nmmintrin.h>], [Ac_cachevar=no]) CFLAGS="$pgac_save_CFLAGS"]) if test x"$Ac_cachevar" = x"yes"; then - CFLAGS_SSE42="$1" + CFLAGS_CRC="$1" pgac_sse42_crc32_intrinsics=yes fi undefine([Ac_cachevar])dnl @@ -629,7 +629,7 @@ undefine([Ac_cachevar])dnl # # An optional compiler flag can be passed as argument (e.g. # -march=armv8-a+crc). If the intrinsics are supported, sets -# pgac_armv8_crc32c_intrinsics, and CFLAGS_ARMV8_CRC32C. +# pgac_armv8_crc32c_intrinsics, and CFLAGS_CRC. AC_DEFUN([PGAC_ARMV8_CRC32C_INTRINSICS], [define([Ac_cachevar], [AS_TR_SH([pgac_cv_armv8_crc32c_intrinsics_$1])])dnl AC_CACHE_CHECK([for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=$1], [Ac_cachevar], @@ -647,7 +647,7 @@ AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <arm_acle.h>], [Ac_cachevar=no]) CFLAGS="$pgac_save_CFLAGS"]) if test x"$Ac_cachevar" = x"yes"; then - CFLAGS_ARMV8_CRC32C="$1" + CFLAGS_CRC="$1" pgac_armv8_crc32c_intrinsics=yes fi undefine([Ac_cachevar])dnl diff --git a/configure b/configure index e04ee9fb416..e65ac574fb5 100755 --- a/configure +++ b/configure @@ -645,8 +645,7 @@ MSGMERGE MSGFMT_FLAGS MSGFMT PG_CRC32C_OBJS -CFLAGS_ARMV8_CRC32C -CFLAGS_SSE42 +CFLAGS_CRC LIBOBJS ZSTD LZ4 @@ -17813,7 +17812,7 @@ fi # # First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used # with the default compiler flags. If not, check if adding the -msse4.2 -# flag helps. CFLAGS_SSE42 is set to -msse4.2 if that's required. +# flag helps. CFLAGS_CRC is set to -msse4.2 if that's required. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=" >&5 $as_echo_n "checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=... " >&6; } if ${pgac_cv_sse42_crc32_intrinsics_+:} false; then : @@ -17848,7 +17847,7 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_sse42_crc32_intrinsics_" >&5 $as_echo "$pgac_cv_sse42_crc32_intrinsics_" >&6; } if test x"$pgac_cv_sse42_crc32_intrinsics_" = x"yes"; then - CFLAGS_SSE42="" + CFLAGS_CRC="" pgac_sse42_crc32_intrinsics=yes fi @@ -17887,13 +17886,12 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_sse42_crc32_intrinsics__msse4_2" >&5 $as_echo "$pgac_cv_sse42_crc32_intrinsics__msse4_2" >&6; } if test x"$pgac_cv_sse42_crc32_intrinsics__msse4_2" = x"yes"; then - CFLAGS_SSE42="-msse4.2" + CFLAGS_CRC="-msse4.2" pgac_sse42_crc32_intrinsics=yes fi fi - # Are we targeting a processor that supports SSE 4.2? gcc, clang and icc all # define __SSE4_2__ in that case. cat confdefs.h - <<_ACEOF >conftest.$ac_ext @@ -17920,7 +17918,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext # # First check if __crc32c* intrinsics can be used with the default compiler # flags. If not, check if adding -march=armv8-a+crc flag helps. -# CFLAGS_ARMV8_CRC32C is set if the extra flag is required. +# CFLAGS_CRC is set if the extra flag is required. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=" >&5 $as_echo_n "checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=... " >&6; } if ${pgac_cv_armv8_crc32c_intrinsics_+:} false; then : @@ -17957,7 +17955,7 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_armv8_crc32c_intrinsics_" >&5 $as_echo "$pgac_cv_armv8_crc32c_intrinsics_" >&6; } if test x"$pgac_cv_armv8_crc32c_intrinsics_" = x"yes"; then - CFLAGS_ARMV8_CRC32C="" + CFLAGS_CRC="" pgac_armv8_crc32c_intrinsics=yes fi @@ -17998,13 +17996,14 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" >&5 $as_echo "$pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" >&6; } if test x"$pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" = x"yes"; then - CFLAGS_ARMV8_CRC32C="-march=armv8-a+crc" + CFLAGS_CRC="-march=armv8-a+crc" pgac_armv8_crc32c_intrinsics=yes fi fi + # Select CRC-32C implementation. # # If we are targeting a processor that has Intel SSE 4.2 instructions, we can @@ -18032,7 +18031,7 @@ if test x"$USE_SLICING_BY_8_CRC32C" = x"" && test x"$USE_SSE42_CRC32C" = x"" && USE_SSE42_CRC32C_WITH_RUNTIME_CHECK=1 else # Use ARM CRC Extension if available. - if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_ARMV8_CRC32C" = x""; then + if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_CRC" = x""; then USE_ARMV8_CRC32C=1 else # ARM CRC Extension, with runtime check? diff --git a/configure.ac b/configure.ac index f146c8301ae..219e85441d2 100644 --- a/configure.ac +++ b/configure.ac @@ -2087,12 +2087,11 @@ fi # # First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used # with the default compiler flags. If not, check if adding the -msse4.2 -# flag helps. CFLAGS_SSE42 is set to -msse4.2 if that's required. +# flag helps. CFLAGS_CRC is set to -msse4.2 if that's required. PGAC_SSE42_CRC32_INTRINSICS([]) if test x"$pgac_sse42_crc32_intrinsics" != x"yes"; then PGAC_SSE42_CRC32_INTRINSICS([-msse4.2]) fi -AC_SUBST(CFLAGS_SSE42) # Are we targeting a processor that supports SSE 4.2? gcc, clang and icc all # define __SSE4_2__ in that case. @@ -2106,12 +2105,13 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [ # # First check if __crc32c* intrinsics can be used with the default compiler # flags. If not, check if adding -march=armv8-a+crc flag helps. -# CFLAGS_ARMV8_CRC32C is set if the extra flag is required. +# CFLAGS_CRC is set if the extra flag is required. PGAC_ARMV8_CRC32C_INTRINSICS([]) if test x"$pgac_armv8_crc32c_intrinsics" != x"yes"; then PGAC_ARMV8_CRC32C_INTRINSICS([-march=armv8-a+crc]) fi -AC_SUBST(CFLAGS_ARMV8_CRC32C) + +AC_SUBST(CFLAGS_CRC) # Select CRC-32C implementation. # @@ -2140,7 +2140,7 @@ if test x"$USE_SLICING_BY_8_CRC32C" = x"" && test x"$USE_SSE42_CRC32C" = x"" && USE_SSE42_CRC32C_WITH_RUNTIME_CHECK=1 else # Use ARM CRC Extension if available. - if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_ARMV8_CRC32C" = x""; then + if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_CRC" = x""; then USE_ARMV8_CRC32C=1 else # ARM CRC Extension, with runtime check? diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 99889167e18..a5e2f36d217 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -262,8 +262,7 @@ CFLAGS_SL_MODULE = @CFLAGS_SL_MODULE@ CXXFLAGS_SL_MODULE = @CXXFLAGS_SL_MODULE@ CFLAGS_UNROLL_LOOPS = @CFLAGS_UNROLL_LOOPS@ CFLAGS_VECTORIZE = @CFLAGS_VECTORIZE@ -CFLAGS_SSE42 = @CFLAGS_SSE42@ -CFLAGS_ARMV8_CRC32C = @CFLAGS_ARMV8_CRC32C@ +CFLAGS_CRC = @CFLAGS_CRC@ PERMIT_DECLARATION_AFTER_STATEMENT = @PERMIT_DECLARATION_AFTER_STATEMENT@ CXXFLAGS = @CXXFLAGS@ -- 2.38.0 --72v5daafgeyrdmny Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v3-0002-autoconf-Don-t-AC_SUBST-LD-in-configure.patch" ^ permalink raw reply [nested|flat] 38+ messages in thread
* [PATCH v2 1/5] autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C @ 2022-10-05 19:24 Andres Freund <[email protected]> 0 siblings, 0 replies; 38+ messages in thread From: Andres Freund @ 2022-10-05 19:24 UTC (permalink / raw) Until now we emit the cflags to build the CRC objects into architecture specific variables. That doesn't make a whole lot of sense to me - we're never going to target x86 and arm at the same time, so they don't need to be separate variables. It might be better to instead continue to have CFLAGS_SSE42 / CFLAGS_ARMV8_CRC32C be computed by PGAC_ARMV8_CRC32C_INTRINSICS / PGAC_SSE42_CRC32_INTRINSICS and then set CFLAGS_CRC based on those. But it seems unlikely that we'd need other sets of flags for those two architectures at the same time. --- src/port/Makefile | 16 ++++++++-------- config/c-compiler.m4 | 8 ++++---- configure | 19 +++++++++---------- configure.ac | 10 +++++----- src/Makefile.global.in | 3 +-- 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/src/port/Makefile b/src/port/Makefile index b3754d8940a..711f59e32bd 100644 --- a/src/port/Makefile +++ b/src/port/Makefile @@ -88,15 +88,15 @@ libpgport.a: $(OBJS) thread.o: CFLAGS+=$(PTHREAD_CFLAGS) thread_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS) -# all versions of pg_crc32c_sse42.o need CFLAGS_SSE42 -pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_SSE42) -pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_SSE42) -pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_SSE42) +# all versions of pg_crc32c_sse42.o need CFLAGS_CRC +pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_CRC) -# all versions of pg_crc32c_armv8.o need CFLAGS_ARMV8_CRC32C -pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) -pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) -pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) +# all versions of pg_crc32c_armv8.o need CFLAGS_CRC +pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_CRC) # # Shared library versions of object files diff --git a/config/c-compiler.m4 b/config/c-compiler.m4 index 000b075312e..eb8cc8ce170 100644 --- a/config/c-compiler.m4 +++ b/config/c-compiler.m4 @@ -597,7 +597,7 @@ fi])# PGAC_HAVE_GCC__ATOMIC_INT64_CAS # the other ones are, on x86-64 platforms) # # An optional compiler flag can be passed as argument (e.g. -msse4.2). If the -# intrinsics are supported, sets pgac_sse42_crc32_intrinsics, and CFLAGS_SSE42. +# intrinsics are supported, sets pgac_sse42_crc32_intrinsics, and CFLAGS_CRC. AC_DEFUN([PGAC_SSE42_CRC32_INTRINSICS], [define([Ac_cachevar], [AS_TR_SH([pgac_cv_sse42_crc32_intrinsics_$1])])dnl AC_CACHE_CHECK([for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=$1], [Ac_cachevar], @@ -613,7 +613,7 @@ AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <nmmintrin.h>], [Ac_cachevar=no]) CFLAGS="$pgac_save_CFLAGS"]) if test x"$Ac_cachevar" = x"yes"; then - CFLAGS_SSE42="$1" + CFLAGS_CRC="$1" pgac_sse42_crc32_intrinsics=yes fi undefine([Ac_cachevar])dnl @@ -629,7 +629,7 @@ undefine([Ac_cachevar])dnl # # An optional compiler flag can be passed as argument (e.g. # -march=armv8-a+crc). If the intrinsics are supported, sets -# pgac_armv8_crc32c_intrinsics, and CFLAGS_ARMV8_CRC32C. +# pgac_armv8_crc32c_intrinsics, and CFLAGS_CRC. AC_DEFUN([PGAC_ARMV8_CRC32C_INTRINSICS], [define([Ac_cachevar], [AS_TR_SH([pgac_cv_armv8_crc32c_intrinsics_$1])])dnl AC_CACHE_CHECK([for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=$1], [Ac_cachevar], @@ -647,7 +647,7 @@ AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <arm_acle.h>], [Ac_cachevar=no]) CFLAGS="$pgac_save_CFLAGS"]) if test x"$Ac_cachevar" = x"yes"; then - CFLAGS_ARMV8_CRC32C="$1" + CFLAGS_CRC="$1" pgac_armv8_crc32c_intrinsics=yes fi undefine([Ac_cachevar])dnl diff --git a/configure b/configure index 1caca21b625..80b28cb9310 100755 --- a/configure +++ b/configure @@ -645,8 +645,7 @@ MSGMERGE MSGFMT_FLAGS MSGFMT PG_CRC32C_OBJS -CFLAGS_ARMV8_CRC32C -CFLAGS_SSE42 +CFLAGS_CRC LIBOBJS ZSTD LZ4 @@ -17815,7 +17814,7 @@ fi # # First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used # with the default compiler flags. If not, check if adding the -msse4.2 -# flag helps. CFLAGS_SSE42 is set to -msse4.2 if that's required. +# flag helps. CFLAGS_CRC is set to -msse4.2 if that's required. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=" >&5 $as_echo_n "checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=... " >&6; } if ${pgac_cv_sse42_crc32_intrinsics_+:} false; then : @@ -17850,7 +17849,7 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_sse42_crc32_intrinsics_" >&5 $as_echo "$pgac_cv_sse42_crc32_intrinsics_" >&6; } if test x"$pgac_cv_sse42_crc32_intrinsics_" = x"yes"; then - CFLAGS_SSE42="" + CFLAGS_CRC="" pgac_sse42_crc32_intrinsics=yes fi @@ -17889,13 +17888,12 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_sse42_crc32_intrinsics__msse4_2" >&5 $as_echo "$pgac_cv_sse42_crc32_intrinsics__msse4_2" >&6; } if test x"$pgac_cv_sse42_crc32_intrinsics__msse4_2" = x"yes"; then - CFLAGS_SSE42="-msse4.2" + CFLAGS_CRC="-msse4.2" pgac_sse42_crc32_intrinsics=yes fi fi - # Are we targeting a processor that supports SSE 4.2? gcc, clang and icc all # define __SSE4_2__ in that case. cat confdefs.h - <<_ACEOF >conftest.$ac_ext @@ -17922,7 +17920,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext # # First check if __crc32c* intrinsics can be used with the default compiler # flags. If not, check if adding -march=armv8-a+crc flag helps. -# CFLAGS_ARMV8_CRC32C is set if the extra flag is required. +# CFLAGS_CRC is set if the extra flag is required. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=" >&5 $as_echo_n "checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=... " >&6; } if ${pgac_cv_armv8_crc32c_intrinsics_+:} false; then : @@ -17959,7 +17957,7 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_armv8_crc32c_intrinsics_" >&5 $as_echo "$pgac_cv_armv8_crc32c_intrinsics_" >&6; } if test x"$pgac_cv_armv8_crc32c_intrinsics_" = x"yes"; then - CFLAGS_ARMV8_CRC32C="" + CFLAGS_CRC="" pgac_armv8_crc32c_intrinsics=yes fi @@ -18000,13 +17998,14 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" >&5 $as_echo "$pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" >&6; } if test x"$pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" = x"yes"; then - CFLAGS_ARMV8_CRC32C="-march=armv8-a+crc" + CFLAGS_CRC="-march=armv8-a+crc" pgac_armv8_crc32c_intrinsics=yes fi fi + # Select CRC-32C implementation. # # If we are targeting a processor that has Intel SSE 4.2 instructions, we can @@ -18034,7 +18033,7 @@ if test x"$USE_SLICING_BY_8_CRC32C" = x"" && test x"$USE_SSE42_CRC32C" = x"" && USE_SSE42_CRC32C_WITH_RUNTIME_CHECK=1 else # Use ARM CRC Extension if available. - if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_ARMV8_CRC32C" = x""; then + if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_CRC" = x""; then USE_ARMV8_CRC32C=1 else # ARM CRC Extension, with runtime check? diff --git a/configure.ac b/configure.ac index 10fa55dd154..f837a602d01 100644 --- a/configure.ac +++ b/configure.ac @@ -2086,12 +2086,11 @@ fi # # First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used # with the default compiler flags. If not, check if adding the -msse4.2 -# flag helps. CFLAGS_SSE42 is set to -msse4.2 if that's required. +# flag helps. CFLAGS_CRC is set to -msse4.2 if that's required. PGAC_SSE42_CRC32_INTRINSICS([]) if test x"$pgac_sse42_crc32_intrinsics" != x"yes"; then PGAC_SSE42_CRC32_INTRINSICS([-msse4.2]) fi -AC_SUBST(CFLAGS_SSE42) # Are we targeting a processor that supports SSE 4.2? gcc, clang and icc all # define __SSE4_2__ in that case. @@ -2105,12 +2104,13 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [ # # First check if __crc32c* intrinsics can be used with the default compiler # flags. If not, check if adding -march=armv8-a+crc flag helps. -# CFLAGS_ARMV8_CRC32C is set if the extra flag is required. +# CFLAGS_CRC is set if the extra flag is required. PGAC_ARMV8_CRC32C_INTRINSICS([]) if test x"$pgac_armv8_crc32c_intrinsics" != x"yes"; then PGAC_ARMV8_CRC32C_INTRINSICS([-march=armv8-a+crc]) fi -AC_SUBST(CFLAGS_ARMV8_CRC32C) + +AC_SUBST(CFLAGS_CRC) # Select CRC-32C implementation. # @@ -2139,7 +2139,7 @@ if test x"$USE_SLICING_BY_8_CRC32C" = x"" && test x"$USE_SSE42_CRC32C" = x"" && USE_SSE42_CRC32C_WITH_RUNTIME_CHECK=1 else # Use ARM CRC Extension if available. - if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_ARMV8_CRC32C" = x""; then + if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_CRC" = x""; then USE_ARMV8_CRC32C=1 else # ARM CRC Extension, with runtime check? diff --git a/src/Makefile.global.in b/src/Makefile.global.in index d8ea2da583c..c87b6d97ca9 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -262,8 +262,7 @@ CFLAGS_SL_MODULE = @CFLAGS_SL_MODULE@ CXXFLAGS_SL_MODULE = @CXXFLAGS_SL_MODULE@ CFLAGS_UNROLL_LOOPS = @CFLAGS_UNROLL_LOOPS@ CFLAGS_VECTORIZE = @CFLAGS_VECTORIZE@ -CFLAGS_SSE42 = @CFLAGS_SSE42@ -CFLAGS_ARMV8_CRC32C = @CFLAGS_ARMV8_CRC32C@ +CFLAGS_CRC = @CFLAGS_CRC@ PERMIT_DECLARATION_AFTER_STATEMENT = @PERMIT_DECLARATION_AFTER_STATEMENT@ CXXFLAGS = @CXXFLAGS@ -- 2.37.3.542.gdd3f6c4cae --muxyv6qxhuvraoxq Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v2-0002-autoconf-Rely-on-ar-supporting-index-creation.patch" ^ permalink raw reply [nested|flat] 38+ messages in thread
* [PATCH v4 1/4] autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C @ 2022-10-05 19:24 Andres Freund <[email protected]> 0 siblings, 0 replies; 38+ messages in thread From: Andres Freund @ 2022-10-05 19:24 UTC (permalink / raw) Until now we emitted the cflags to build the CRC objects into architecture specific variables. That doesn't make a whole lot of sense to me - we're never going to target x86 and arm at the same time, so they don't need to be separate variables. It might be better to instead continue to have CFLAGS_SSE42 / CFLAGS_ARMV8_CRC32C be computed by PGAC_ARMV8_CRC32C_INTRINSICS / PGAC_SSE42_CRC32_INTRINSICS and then set CFLAGS_CRC based on those. But it seems unlikely that we'd need other sets of CRC specific flags for those two architectures at the same time. This simplifies the upcoming meson PGXS compatibility. Discussion: https://postgr.es/m/[email protected] --- configure.ac | 10 +++++----- config/c-compiler.m4 | 8 ++++---- src/port/Makefile | 16 ++++++++-------- configure | 19 +++++++++---------- src/Makefile.global.in | 3 +-- 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/configure.ac b/configure.ac index f76b7ee31fc..6e7c8e09411 100644 --- a/configure.ac +++ b/configure.ac @@ -2091,12 +2091,11 @@ fi # # First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used # with the default compiler flags. If not, check if adding the -msse4.2 -# flag helps. CFLAGS_SSE42 is set to -msse4.2 if that's required. +# flag helps. CFLAGS_CRC is set to -msse4.2 if that's required. PGAC_SSE42_CRC32_INTRINSICS([]) if test x"$pgac_sse42_crc32_intrinsics" != x"yes"; then PGAC_SSE42_CRC32_INTRINSICS([-msse4.2]) fi -AC_SUBST(CFLAGS_SSE42) # Are we targeting a processor that supports SSE 4.2? gcc, clang and icc all # define __SSE4_2__ in that case. @@ -2110,12 +2109,13 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [ # # First check if __crc32c* intrinsics can be used with the default compiler # flags. If not, check if adding -march=armv8-a+crc flag helps. -# CFLAGS_ARMV8_CRC32C is set if the extra flag is required. +# CFLAGS_CRC is set if the extra flag is required. PGAC_ARMV8_CRC32C_INTRINSICS([]) if test x"$pgac_armv8_crc32c_intrinsics" != x"yes"; then PGAC_ARMV8_CRC32C_INTRINSICS([-march=armv8-a+crc]) fi -AC_SUBST(CFLAGS_ARMV8_CRC32C) + +AC_SUBST(CFLAGS_CRC) # Select CRC-32C implementation. # @@ -2144,7 +2144,7 @@ if test x"$USE_SLICING_BY_8_CRC32C" = x"" && test x"$USE_SSE42_CRC32C" = x"" && USE_SSE42_CRC32C_WITH_RUNTIME_CHECK=1 else # Use ARM CRC Extension if available. - if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_ARMV8_CRC32C" = x""; then + if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_CRC" = x""; then USE_ARMV8_CRC32C=1 else # ARM CRC Extension, with runtime check? diff --git a/config/c-compiler.m4 b/config/c-compiler.m4 index 000b075312e..eb8cc8ce170 100644 --- a/config/c-compiler.m4 +++ b/config/c-compiler.m4 @@ -597,7 +597,7 @@ fi])# PGAC_HAVE_GCC__ATOMIC_INT64_CAS # the other ones are, on x86-64 platforms) # # An optional compiler flag can be passed as argument (e.g. -msse4.2). If the -# intrinsics are supported, sets pgac_sse42_crc32_intrinsics, and CFLAGS_SSE42. +# intrinsics are supported, sets pgac_sse42_crc32_intrinsics, and CFLAGS_CRC. AC_DEFUN([PGAC_SSE42_CRC32_INTRINSICS], [define([Ac_cachevar], [AS_TR_SH([pgac_cv_sse42_crc32_intrinsics_$1])])dnl AC_CACHE_CHECK([for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=$1], [Ac_cachevar], @@ -613,7 +613,7 @@ AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <nmmintrin.h>], [Ac_cachevar=no]) CFLAGS="$pgac_save_CFLAGS"]) if test x"$Ac_cachevar" = x"yes"; then - CFLAGS_SSE42="$1" + CFLAGS_CRC="$1" pgac_sse42_crc32_intrinsics=yes fi undefine([Ac_cachevar])dnl @@ -629,7 +629,7 @@ undefine([Ac_cachevar])dnl # # An optional compiler flag can be passed as argument (e.g. # -march=armv8-a+crc). If the intrinsics are supported, sets -# pgac_armv8_crc32c_intrinsics, and CFLAGS_ARMV8_CRC32C. +# pgac_armv8_crc32c_intrinsics, and CFLAGS_CRC. AC_DEFUN([PGAC_ARMV8_CRC32C_INTRINSICS], [define([Ac_cachevar], [AS_TR_SH([pgac_cv_armv8_crc32c_intrinsics_$1])])dnl AC_CACHE_CHECK([for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=$1], [Ac_cachevar], @@ -647,7 +647,7 @@ AC_LINK_IFELSE([AC_LANG_PROGRAM([#include <arm_acle.h>], [Ac_cachevar=no]) CFLAGS="$pgac_save_CFLAGS"]) if test x"$Ac_cachevar" = x"yes"; then - CFLAGS_ARMV8_CRC32C="$1" + CFLAGS_CRC="$1" pgac_armv8_crc32c_intrinsics=yes fi undefine([Ac_cachevar])dnl diff --git a/src/port/Makefile b/src/port/Makefile index b3754d8940a..711f59e32bd 100644 --- a/src/port/Makefile +++ b/src/port/Makefile @@ -88,15 +88,15 @@ libpgport.a: $(OBJS) thread.o: CFLAGS+=$(PTHREAD_CFLAGS) thread_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS) -# all versions of pg_crc32c_sse42.o need CFLAGS_SSE42 -pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_SSE42) -pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_SSE42) -pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_SSE42) +# all versions of pg_crc32c_sse42.o need CFLAGS_CRC +pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_CRC) -# all versions of pg_crc32c_armv8.o need CFLAGS_ARMV8_CRC32C -pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) -pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) -pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C) +# all versions of pg_crc32c_armv8.o need CFLAGS_CRC +pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_CRC) +pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_CRC) # # Shared library versions of object files diff --git a/configure b/configure index 3966368b8d9..f62fbc5d0f5 100755 --- a/configure +++ b/configure @@ -645,8 +645,7 @@ MSGMERGE MSGFMT_FLAGS MSGFMT PG_CRC32C_OBJS -CFLAGS_ARMV8_CRC32C -CFLAGS_SSE42 +CFLAGS_CRC LIBOBJS OPENSSL ZSTD @@ -17957,7 +17956,7 @@ fi # # First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used # with the default compiler flags. If not, check if adding the -msse4.2 -# flag helps. CFLAGS_SSE42 is set to -msse4.2 if that's required. +# flag helps. CFLAGS_CRC is set to -msse4.2 if that's required. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=" >&5 $as_echo_n "checking for _mm_crc32_u8 and _mm_crc32_u32 with CFLAGS=... " >&6; } if ${pgac_cv_sse42_crc32_intrinsics_+:} false; then : @@ -17992,7 +17991,7 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_sse42_crc32_intrinsics_" >&5 $as_echo "$pgac_cv_sse42_crc32_intrinsics_" >&6; } if test x"$pgac_cv_sse42_crc32_intrinsics_" = x"yes"; then - CFLAGS_SSE42="" + CFLAGS_CRC="" pgac_sse42_crc32_intrinsics=yes fi @@ -18031,13 +18030,12 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_sse42_crc32_intrinsics__msse4_2" >&5 $as_echo "$pgac_cv_sse42_crc32_intrinsics__msse4_2" >&6; } if test x"$pgac_cv_sse42_crc32_intrinsics__msse4_2" = x"yes"; then - CFLAGS_SSE42="-msse4.2" + CFLAGS_CRC="-msse4.2" pgac_sse42_crc32_intrinsics=yes fi fi - # Are we targeting a processor that supports SSE 4.2? gcc, clang and icc all # define __SSE4_2__ in that case. cat confdefs.h - <<_ACEOF >conftest.$ac_ext @@ -18064,7 +18062,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext # # First check if __crc32c* intrinsics can be used with the default compiler # flags. If not, check if adding -march=armv8-a+crc flag helps. -# CFLAGS_ARMV8_CRC32C is set if the extra flag is required. +# CFLAGS_CRC is set if the extra flag is required. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=" >&5 $as_echo_n "checking for __crc32cb, __crc32ch, __crc32cw, and __crc32cd with CFLAGS=... " >&6; } if ${pgac_cv_armv8_crc32c_intrinsics_+:} false; then : @@ -18101,7 +18099,7 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_armv8_crc32c_intrinsics_" >&5 $as_echo "$pgac_cv_armv8_crc32c_intrinsics_" >&6; } if test x"$pgac_cv_armv8_crc32c_intrinsics_" = x"yes"; then - CFLAGS_ARMV8_CRC32C="" + CFLAGS_CRC="" pgac_armv8_crc32c_intrinsics=yes fi @@ -18142,13 +18140,14 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" >&5 $as_echo "$pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" >&6; } if test x"$pgac_cv_armv8_crc32c_intrinsics__march_armv8_apcrc" = x"yes"; then - CFLAGS_ARMV8_CRC32C="-march=armv8-a+crc" + CFLAGS_CRC="-march=armv8-a+crc" pgac_armv8_crc32c_intrinsics=yes fi fi + # Select CRC-32C implementation. # # If we are targeting a processor that has Intel SSE 4.2 instructions, we can @@ -18176,7 +18175,7 @@ if test x"$USE_SLICING_BY_8_CRC32C" = x"" && test x"$USE_SSE42_CRC32C" = x"" && USE_SSE42_CRC32C_WITH_RUNTIME_CHECK=1 else # Use ARM CRC Extension if available. - if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_ARMV8_CRC32C" = x""; then + if test x"$pgac_armv8_crc32c_intrinsics" = x"yes" && test x"$CFLAGS_CRC" = x""; then USE_ARMV8_CRC32C=1 else # ARM CRC Extension, with runtime check? diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 84e72e99278..346b73260fc 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -262,8 +262,7 @@ CFLAGS_SL_MODULE = @CFLAGS_SL_MODULE@ CXXFLAGS_SL_MODULE = @CXXFLAGS_SL_MODULE@ CFLAGS_UNROLL_LOOPS = @CFLAGS_UNROLL_LOOPS@ CFLAGS_VECTORIZE = @CFLAGS_VECTORIZE@ -CFLAGS_SSE42 = @CFLAGS_SSE42@ -CFLAGS_ARMV8_CRC32C = @CFLAGS_ARMV8_CRC32C@ +CFLAGS_CRC = @CFLAGS_CRC@ PERMIT_DECLARATION_AFTER_STATEMENT = @PERMIT_DECLARATION_AFTER_STATEMENT@ CXXFLAGS = @CXXFLAGS@ -- 2.38.0 --qh3eagtoyzwfapdm Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v4-0002-autoconf-Don-t-AC_SUBST-LD-in-configure.patch" ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-02-28 19:40 Robert Haas <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Robert Haas @ 2025-02-28 19:40 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers On Wed, Nov 6, 2024 at 5:07 PM Nathan Bossart <[email protected]> wrote: > these user relation files will have the same name. Therefore, it can be > much faster to instead move the entire data directory from the old cluster > to the new cluster and to then swap the catalog relation files. This is a cool idea. > Another interesting problem is that pg_upgrade currently doesn't transfer > the sequence data files. Since v10, we've restored these via pg_restore. > I believe this was originally done for the introduction of the pg_sequence > catalog, which changed the format of sequence tuples. In the new > catalog-swap mode I am proposing, this means we need to transfer all the > pg_restore-generated sequence data files. If there are many sequences, it > can be difficult to determine which transfer mode and synchronization > method will be faster. Since sequence tuple modifications are very rare, I > think the new catalog-swap mode should just use the sequence data files > from the old cluster whenever possible. Maybe we should rethink the decision not to transfer relfilenodes for sequences. Or have more than one way to do it. pg_upgrade --binary-upgrade --binary-upgrade-even-for-sequences, or whatever. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-02-28 19:41 Robert Haas <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Robert Haas @ 2025-02-28 19:41 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers On Fri, Feb 28, 2025 at 2:40 PM Robert Haas <[email protected]> wrote: > Maybe we should rethink the decision not to transfer relfilenodes for > sequences. Or have more than one way to do it. pg_upgrade > --binary-upgrade --binary-upgrade-even-for-sequences, or whatever. Sorry, I meant: pg_dump --binary-upgrade --binary-upgrade-even-for-sequences i.e. pg_upgrade could decide which way to ask pg_dump to do it, depending on versions and flags. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-02-28 20:01 Nathan Bossart <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-02-28 20:01 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Fri, Feb 28, 2025 at 02:41:22PM -0500, Robert Haas wrote: > On Fri, Feb 28, 2025 at 2:40 PM Robert Haas <[email protected]> wrote: >> Maybe we should rethink the decision not to transfer relfilenodes for >> sequences. Or have more than one way to do it. pg_upgrade >> --binary-upgrade --binary-upgrade-even-for-sequences, or whatever. > > Sorry, I meant: pg_dump --binary-upgrade --binary-upgrade-even-for-sequences > > i.e. pg_upgrade could decide which way to ask pg_dump to do it, > depending on versions and flags. That's exactly where I landed (see v3-0002). I haven't measured whether transferring relfilenodes or dumping the sequence data is faster for the existing modes, but for now I've left those alone, i.e., they still dump sequence data. The new "swap" mode just uses the old cluster's sequence files, and I've disallowed using swap mode for upgrades from <v10 to avoid the sequence tuple format change (along with other incompatible changes). I'll admit I'm a bit concerned that this will cause problems if and when someone wants to change the sequence tuple format again. But that hasn't happened for a while, AFAIK nobody's planning to change it, and even if it does happen, we just need to have my proposed new mode transfer the sequence files like it transfers the catalog files. That will make this mode slower, especially if you have a ton of sequences, but maybe it'll still be a win in most cases. Of course, we probably will need to have pg_upgrade handle other kinds of format changes, too, but IMHO it's still worth trying to speed up pg_upgrade despite the potential future complexities. -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-02-28 20:37 Robert Haas <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Robert Haas @ 2025-02-28 20:37 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers On Fri, Feb 28, 2025 at 3:01 PM Nathan Bossart <[email protected]> wrote: > That's exactly where I landed (see v3-0002). I haven't measured whether > transferring relfilenodes or dumping the sequence data is faster for the > existing modes, but for now I've left those alone, i.e., they still dump > sequence data. The new "swap" mode just uses the old cluster's sequence > files, and I've disallowed using swap mode for upgrades from <v10 to avoid > the sequence tuple format change (along with other incompatible changes). Ah. Perhaps I should have read the thread more carefully before commenting. Sounds good, at any rate. > I'll admit I'm a bit concerned that this will cause problems if and when > someone wants to change the sequence tuple format again. But that hasn't > happened for a while, AFAIK nobody's planning to change it, and even if it > does happen, we just need to have my proposed new mode transfer the > sequence files like it transfers the catalog files. That will make this > mode slower, especially if you have a ton of sequences, but maybe it'll > still be a win in most cases. Of course, we probably will need to have > pg_upgrade handle other kinds of format changes, too, but IMHO it's still > worth trying to speed up pg_upgrade despite the potential future > complexities. I think it's fine. If somebody comes along and says "hey, when v23 came out Nathan's feature only sped up pg_upgrade by 2x instead of 3x like it did for v22, so Nathan is a bad person," I think we can fairly reply "thanks for sharing your opinion, feel free not to use the feature and run at 1x speed". There's no rule saying that every optimization must always produce the maximum possible benefit in every scenario. We're just concerned about regressions, and "only delivers some of the speedup if the sequence format has changed on disk" is not a regression. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-02-28 20:51 Nathan Bossart <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-02-28 20:51 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Fri, Feb 28, 2025 at 03:37:49PM -0500, Robert Haas wrote: > On Fri, Feb 28, 2025 at 3:01 PM Nathan Bossart <[email protected]> wrote: >> That's exactly where I landed (see v3-0002). I haven't measured whether >> transferring relfilenodes or dumping the sequence data is faster for the >> existing modes, but for now I've left those alone, i.e., they still dump >> sequence data. The new "swap" mode just uses the old cluster's sequence >> files, and I've disallowed using swap mode for upgrades from <v10 to avoid >> the sequence tuple format change (along with other incompatible changes). > > Ah. Perhaps I should have read the thread more carefully before > commenting. Sounds good, at any rate. On the contrary, I'm glad you independently came to the same conclusion. >> I'll admit I'm a bit concerned that this will cause problems if and when >> someone wants to change the sequence tuple format again. But that hasn't >> happened for a while, AFAIK nobody's planning to change it, and even if it >> does happen, we just need to have my proposed new mode transfer the >> sequence files like it transfers the catalog files. That will make this >> mode slower, especially if you have a ton of sequences, but maybe it'll >> still be a win in most cases. Of course, we probably will need to have >> pg_upgrade handle other kinds of format changes, too, but IMHO it's still >> worth trying to speed up pg_upgrade despite the potential future >> complexities. > > I think it's fine. If somebody comes along and says "hey, when v23 > came out Nathan's feature only sped up pg_upgrade by 2x instead of 3x > like it did for v22, so Nathan is a bad person," I think we can fairly > reply "thanks for sharing your opinion, feel free not to use the > feature and run at 1x speed". There's no rule saying that every > optimization must always produce the maximum possible benefit in every > scenario. We're just concerned about regressions, and "only delivers > some of the speedup if the sequence format has changed on disk" is not > a regression. Cool. I appreciate the design feedback. -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-05 19:42 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 2 replies; 38+ messages in thread From: Nathan Bossart @ 2025-03-05 19:42 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Fri, Feb 28, 2025 at 02:51:27PM -0600, Nathan Bossart wrote: > Cool. I appreciate the design feedback. One other design point I wanted to bring up is whether we should bother generating a rollback script for the new "swap" mode. In short, I'm wondering if it would be unreasonable to say that, just for this mode, once pg_upgrade enters the file transfer step, reverting to the old cluster requires restoring a backup. I believe that's worth considering for the following reasons: * Anecdotally, I'm not sure I've ever actually seen pg_upgrade fail during or after file transfer, and I'm hoping to get some real data about that in the near future. Has anyone else dealt with such a failure? I suspect that failures during file transfer are typically due to OS crashes, power losses, etc., and hopefully those are rare. * I've spent quite some time trying to generate a portable script, but it's quite complicated and difficult to reason about its correctness. And I haven't even started on the Windows version. Leaving this part out would simplify the patch set quite a bit. * If we give up the idea of reverting to the old cluster, we also can avoid a bunch of intermediate fsync() calls which I only included to help reason about the state of the files in case you failed halfway through. This might not add up to much, but it's at least another area of simplification. Of course, rollback would still be possible, but you'd really need to understand what "swap" mode does behind the scenes to do so safely. In any case, I'm growing skeptical that a probably-not-super-well-tested script that extremely few people will need and fewer will use is worth the complexity. -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-05 20:12 Robert Haas <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 0 replies; 38+ messages in thread From: Robert Haas @ 2025-03-05 20:12 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers On Wed, Mar 5, 2025 at 2:42 PM Nathan Bossart <[email protected]> wrote: > Of course, rollback would still be possible, but you'd really need to > understand what "swap" mode does behind the scenes to do so safely. In any > case, I'm growing skeptical that a probably-not-super-well-tested script > that extremely few people will need and fewer will use is worth the > complexity. I don't have a super-strong view on what the right thing to do is here, but I'm definitely in favor of not doing something half-baked. If you think the revert script is going to suck, then let's not have it at all and just be clear about what the requirements for using this mode are. One serious danger that you didn't mention here is that, if pg_upgrade does fail, you may well try several times. And if you forget the revert script at some point, or run it more than once, or run the wrong version, you will be in trouble. I feel like this is something someone could very easily mess up even if in theory it works perfectly. Upgrades are often stressful times. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-05 20:40 Greg Sabino Mullane <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 2 replies; 38+ messages in thread From: Greg Sabino Mullane @ 2025-03-05 20:40 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Wed, Mar 5, 2025 at 2:43 PM Nathan Bossart <[email protected]> wrote: > One other design point I wanted to bring up is whether we should bother > generating a rollback script for the new "swap" mode. In short, I'm > wondering if it would be unreasonable to say that, just for this mode, once > pg_upgrade enters the file transfer step, reverting to the old cluster > requires restoring a backup. I think that's a fair requirement. And like Robert, revert scripts make me nervous. * Anecdotally, I'm not sure I've ever actually seen pg_upgrade fail > during or after file transfer, and I'm hoping to get some real data about > that in the near future. Has anyone else dealt with such a failure? I've seen various failures, but they always get caught quite early. Certainly early enough to easily abort, fix perms/mounts/etc., then retry. I think your instinct is correct that this reversion is more trouble than its worth. I don't think the pg_upgrade docs mention taking a backup, but that's always step 0 in my playbook, and that's the rollback plan in the unlikely event of failure. Cheers, Greg -- Crunchy Data - https://www.crunchydata.com Enterprise Postgres Software Products & Tech Support ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-06 02:34 Nathan Bossart <[email protected]> parent: Greg Sabino Mullane <[email protected]> 1 sibling, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-06 02:34 UTC (permalink / raw) To: Greg Sabino Mullane <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Wed, Mar 05, 2025 at 03:40:52PM -0500, Greg Sabino Mullane wrote: > I've seen various failures, but they always get caught quite early. > Certainly early enough to easily abort, fix perms/mounts/etc., then retry. > I think your instinct is correct that this reversion is more trouble than > its worth. I don't think the pg_upgrade docs mention taking a backup, but > that's always step 0 in my playbook, and that's the rollback plan in the > unlikely event of failure. Thank you, Greg and Robert, for sharing your thoughts. With that, here's what I'm considering to be a reasonably complete patch set for this feature. This leaves about a month for rigorous testing and editing, so I'm hopeful it'll be ready v18. -- nathan From 6716e7b16a795911f55432dfd6d3c246aa8fd9fe Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 19 Feb 2025 09:14:51 -0600 Subject: [PATCH v4 1/3] initdb: Add --no-sync-data-files. This new option instructs initdb to skip synchronizing any files in database directories and the database directories themselves, i.e., everything in the base/ subdirectory and any other tablespace directories. Other files, such as those in pg_wal/ and pg_xact/, will still be synchronized unless --no-sync is also specified. --no-sync-data-files is primarily intended for internal use by tools that separately ensure the skipped files are synchronized to disk. A follow-up commit will use this to help optimize pg_upgrade's file transfer step. Discussion: https://postgr.es/m/Zyvop-LxLXBLrZil%40nathan --- doc/src/sgml/ref/initdb.sgml | 20 +++++ src/bin/initdb/initdb.c | 10 ++- src/bin/initdb/t/001_initdb.pl | 1 + src/bin/pg_basebackup/pg_basebackup.c | 2 +- src/bin/pg_checksums/pg_checksums.c | 2 +- src/bin/pg_combinebackup/pg_combinebackup.c | 2 +- src/bin/pg_rewind/file_ops.c | 2 +- src/common/file_utils.c | 85 +++++++++++++-------- src/include/common/file_utils.h | 2 +- 9 files changed, 89 insertions(+), 37 deletions(-) diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml index 0026318485a..14c401b9a99 100644 --- a/doc/src/sgml/ref/initdb.sgml +++ b/doc/src/sgml/ref/initdb.sgml @@ -527,6 +527,26 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry id="app-initdb-option-no-sync-data-files"> + <term><option>--no-sync-data-files</option></term> + <listitem> + <para> + By default, <command>initdb</command> safely writes all database files + to disk. This option instructs <command>initdb</command> to skip + synchronizing all files in the individual database directories and the + database directories themselves, i.e., everything in the + <filename>base</filename> subdirectory and any other tablespace + directories. Other files, such as those in <literal>pg_wal</literal> + and <literal>pg_xact</literal>, will still be synchronized unless the + <option>--no-sync</option> option is also specified. + </para> + <para> + This option is primarily intended for internal use by tools that + separately ensure the skipped files are synchronized to disk. + </para> + </listitem> + </varlistentry> + <varlistentry id="app-initdb-option-no-instructions"> <term><option>--no-instructions</option></term> <listitem> diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 21a0fe3ecd9..22b7d31b165 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -168,6 +168,7 @@ static bool data_checksums = true; static char *xlog_dir = NULL; static int wal_segment_size_mb = (DEFAULT_XLOG_SEG_SIZE) / (1024 * 1024); static DataDirSyncMethod sync_method = DATA_DIR_SYNC_METHOD_FSYNC; +static bool sync_data_files = true; /* internal vars */ @@ -2566,6 +2567,7 @@ usage(const char *progname) printf(_(" -L DIRECTORY where to find the input files\n")); printf(_(" -n, --no-clean do not clean up after errors\n")); printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n")); + printf(_(" --no-sync-data-files do not sync files within database directories\n")); printf(_(" --no-instructions do not print instructions for next steps\n")); printf(_(" -s, --show show internal settings, then exit\n")); printf(_(" --sync-method=METHOD set method for syncing files to disk\n")); @@ -3208,6 +3210,7 @@ main(int argc, char *argv[]) {"icu-rules", required_argument, NULL, 18}, {"sync-method", required_argument, NULL, 19}, {"no-data-checksums", no_argument, NULL, 20}, + {"no-sync-data-files", no_argument, NULL, 21}, {NULL, 0, NULL, 0} }; @@ -3402,6 +3405,9 @@ main(int argc, char *argv[]) case 20: data_checksums = false; break; + case 21: + sync_data_files = false; + break; default: /* getopt_long already emitted a complaint */ pg_log_error_hint("Try \"%s --help\" for more information.", progname); @@ -3453,7 +3459,7 @@ main(int argc, char *argv[]) fputs(_("syncing data to disk ... "), stdout); fflush(stdout); - sync_pgdata(pg_data, PG_VERSION_NUM, sync_method); + sync_pgdata(pg_data, PG_VERSION_NUM, sync_method, sync_data_files); check_ok(); return 0; } @@ -3516,7 +3522,7 @@ main(int argc, char *argv[]) { fputs(_("syncing data to disk ... "), stdout); fflush(stdout); - sync_pgdata(pg_data, PG_VERSION_NUM, sync_method); + sync_pgdata(pg_data, PG_VERSION_NUM, sync_method, sync_data_files); check_ok(); } else diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl index 01cc4a1602b..15dd10ce40a 100644 --- a/src/bin/initdb/t/001_initdb.pl +++ b/src/bin/initdb/t/001_initdb.pl @@ -76,6 +76,7 @@ command_like( 'checksums are enabled in control file'); command_ok([ 'initdb', '--sync-only', $datadir ], 'sync only'); +command_ok([ 'initdb', '--sync-only', '--no-sync-data-files', $datadir ], '--no-sync-data-files'); command_fails([ 'initdb', $datadir ], 'existing data directory'); if ($supports_syncfs) diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index dc0c805137a..bc94c114d27 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -2310,7 +2310,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail, } else { - (void) sync_pgdata(basedir, serverVersion, sync_method); + (void) sync_pgdata(basedir, serverVersion, sync_method, true); } } diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 867aeddc601..f20be82862a 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -633,7 +633,7 @@ main(int argc, char *argv[]) if (do_sync) { pg_log_info("syncing data directory"); - sync_pgdata(DataDir, PG_VERSION_NUM, sync_method); + sync_pgdata(DataDir, PG_VERSION_NUM, sync_method, true); } pg_log_info("updating control file"); diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index 5864ec574fb..c0ec09485c3 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -420,7 +420,7 @@ main(int argc, char *argv[]) else { pg_log_debug("recursively fsyncing \"%s\"", opt.output); - sync_pgdata(opt.output, version * 10000, opt.sync_method); + sync_pgdata(opt.output, version * 10000, opt.sync_method, true); } } diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c index 467845419ed..55659ce201f 100644 --- a/src/bin/pg_rewind/file_ops.c +++ b/src/bin/pg_rewind/file_ops.c @@ -296,7 +296,7 @@ sync_target_dir(void) if (!do_sync || dry_run) return; - sync_pgdata(datadir_target, PG_VERSION_NUM, sync_method); + sync_pgdata(datadir_target, PG_VERSION_NUM, sync_method, true); } diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 0e3cfede935..78e272916f5 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -50,7 +50,8 @@ static int pre_sync_fname(const char *fname, bool isdir); #endif static void walkdir(const char *path, int (*action) (const char *fname, bool isdir), - bool process_symlinks); + bool process_symlinks, + const char *exclude_dir); #ifdef HAVE_SYNCFS @@ -93,11 +94,15 @@ do_syncfs(const char *path) * syncing, and might not have privileges to write at all. * * serverVersion indicates the version of the server to be sync'd. + * + * If sync_data_files is false, this function skips syncing "base/" and any + * other tablespace directories. */ void sync_pgdata(const char *pg_data, int serverVersion, - DataDirSyncMethod sync_method) + DataDirSyncMethod sync_method, + bool sync_data_files) { bool xlog_is_symlink; char pg_wal[MAXPGPATH]; @@ -147,30 +152,33 @@ sync_pgdata(const char *pg_data, do_syncfs(pg_data); /* If any tablespaces are configured, sync each of those. */ - dir = opendir(pg_tblspc); - if (dir == NULL) - pg_log_error("could not open directory \"%s\": %m", - pg_tblspc); - else + if (sync_data_files) { - while (errno = 0, (de = readdir(dir)) != NULL) + dir = opendir(pg_tblspc); + if (dir == NULL) + pg_log_error("could not open directory \"%s\": %m", + pg_tblspc); + else { - char subpath[MAXPGPATH * 2]; + while (errno = 0, (de = readdir(dir)) != NULL) + { + char subpath[MAXPGPATH * 2]; - if (strcmp(de->d_name, ".") == 0 || - strcmp(de->d_name, "..") == 0) - continue; + if (strcmp(de->d_name, ".") == 0 || + strcmp(de->d_name, "..") == 0) + continue; - snprintf(subpath, sizeof(subpath), "%s/%s", - pg_tblspc, de->d_name); - do_syncfs(subpath); - } + snprintf(subpath, sizeof(subpath), "%s/%s", + pg_tblspc, de->d_name); + do_syncfs(subpath); + } - if (errno) - pg_log_error("could not read directory \"%s\": %m", - pg_tblspc); + if (errno) + pg_log_error("could not read directory \"%s\": %m", + pg_tblspc); - (void) closedir(dir); + (void) closedir(dir); + } } /* If pg_wal is a symlink, process that too. */ @@ -182,15 +190,21 @@ sync_pgdata(const char *pg_data, case DATA_DIR_SYNC_METHOD_FSYNC: { + char *exclude_dir = NULL; + + if (!sync_data_files) + exclude_dir = psprintf("%s/base", pg_data); + /* * If possible, hint to the kernel that we're soon going to * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(pg_data, pre_sync_fname, false); + walkdir(pg_data, pre_sync_fname, false, exclude_dir); if (xlog_is_symlink) - walkdir(pg_wal, pre_sync_fname, false); - walkdir(pg_tblspc, pre_sync_fname, true); + walkdir(pg_wal, pre_sync_fname, false, NULL); + if (sync_data_files) + walkdir(pg_tblspc, pre_sync_fname, true, NULL); #endif /* @@ -203,10 +217,14 @@ sync_pgdata(const char *pg_data, * get fsync'd twice. That's not an expected case so we don't * worry about optimizing it. */ - walkdir(pg_data, fsync_fname, false); + walkdir(pg_data, fsync_fname, false, exclude_dir); if (xlog_is_symlink) - walkdir(pg_wal, fsync_fname, false); - walkdir(pg_tblspc, fsync_fname, true); + walkdir(pg_wal, fsync_fname, false, NULL); + if (sync_data_files) + walkdir(pg_tblspc, fsync_fname, true, NULL); + + if (exclude_dir) + pfree(exclude_dir); } break; } @@ -245,10 +263,10 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(dir, pre_sync_fname, false); + walkdir(dir, pre_sync_fname, false, NULL); #endif - walkdir(dir, fsync_fname, false); + walkdir(dir, fsync_fname, false, NULL); } break; } @@ -264,6 +282,9 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) * ignored in subdirectories, ie we intentionally don't pass down the * process_symlinks flag to recursive calls. * + * If exclude_dir is not NULL, it specifies a directory path to skip + * processing. + * * Errors are reported but not considered fatal. * * See also walkdir in fd.c, which is a backend version of this logic. @@ -271,11 +292,15 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) static void walkdir(const char *path, int (*action) (const char *fname, bool isdir), - bool process_symlinks) + bool process_symlinks, + const char *exclude_dir) { DIR *dir; struct dirent *de; + if (exclude_dir && strcmp(exclude_dir, path) == 0) + return; + dir = opendir(path); if (dir == NULL) { @@ -299,7 +324,7 @@ walkdir(const char *path, (*action) (subpath, false); break; case PGFILETYPE_DIR: - walkdir(subpath, action, false); + walkdir(subpath, action, false, exclude_dir); break; default: diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index a832210adc1..8274bc877ab 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -35,7 +35,7 @@ struct iovec; /* avoid including port/pg_iovec.h here */ #ifdef FRONTEND extern int fsync_fname(const char *fname, bool isdir); extern void sync_pgdata(const char *pg_data, int serverVersion, - DataDirSyncMethod sync_method); + DataDirSyncMethod sync_method, bool sync_data_files); extern void sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method); extern int durable_rename(const char *oldfile, const char *newfile); extern int fsync_parent_path(const char *fname); -- 2.39.5 (Apple Git-154) From e6dc183b8a80a32f6ca52b0d21a173d1b291deea Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 19 Feb 2025 11:25:28 -0600 Subject: [PATCH v4 2/3] pg_dump: Add --sequence-data. This new option instructs pg_dump to dump sequence data when the --no-data, --schema-only, or --statistics-only option is specified. This was originally considered for commit a7e5457db8, but it was left out at that time because there was no known use-case. A follow-up commit will use this to optimize pg_upgrade's file transfer step. Discussion: https://postgr.es/m/Zyvop-LxLXBLrZil%40nathan --- doc/src/sgml/ref/pg_dump.sgml | 11 +++++++++++ src/bin/pg_dump/pg_dump.c | 10 ++-------- src/bin/pg_dump/t/002_pg_dump.pl | 1 + src/bin/pg_upgrade/dump.c | 2 +- src/test/modules/test_pg_dump/t/001_base.pl | 2 +- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml index 1975054d7bf..b05f16995c3 100644 --- a/doc/src/sgml/ref/pg_dump.sgml +++ b/doc/src/sgml/ref/pg_dump.sgml @@ -1289,6 +1289,17 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>--sequence-data</option></term> + <listitem> + <para> + Include sequence data in the dump. This is the default behavior except + when <option>--no-data</option>, <option>--schema-only</option>, or + <option>--statistics-only</option> is specified. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>--serializable-deferrable</option></term> <listitem> diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 4f4ad2ee150..f63215eb3f9 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -517,6 +517,7 @@ main(int argc, char **argv) {"sync-method", required_argument, NULL, 15}, {"filter", required_argument, NULL, 16}, {"exclude-extension", required_argument, NULL, 17}, + {"sequence-data", no_argument, &dopt.sequence_data, 1}, {NULL, 0, NULL, 0} }; @@ -803,14 +804,6 @@ main(int argc, char **argv) if (dopt.column_inserts && dopt.dump_inserts == 0) dopt.dump_inserts = DUMP_DEFAULT_ROWS_PER_INSERT; - /* - * Binary upgrade mode implies dumping sequence data even in schema-only - * mode. This is not exposed as a separate option, but kept separate - * internally for clarity. - */ - if (dopt.binary_upgrade) - dopt.sequence_data = 1; - if (data_only && schema_only) pg_fatal("options -s/--schema-only and -a/--data-only cannot be used together"); if (schema_only && statistics_only) @@ -1275,6 +1268,7 @@ help(const char *progname) printf(_(" --quote-all-identifiers quote all identifiers, even if not key words\n")); printf(_(" --rows-per-insert=NROWS number of rows per INSERT; implies --inserts\n")); printf(_(" --section=SECTION dump named section (pre-data, data, or post-data)\n")); + printf(_(" --sequence-data include sequence data in dump\n")); printf(_(" --serializable-deferrable wait until the dump can run without anomalies\n")); printf(_(" --snapshot=SNAPSHOT use given snapshot for the dump\n")); printf(_(" --statistics-only dump only the statistics, not schema or data\n")); diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index c7bffc1b045..8ae6c5374fc 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -66,6 +66,7 @@ my %pgdump_runs = ( '--file' => "$tempdir/binary_upgrade.dump", '--no-password', '--no-data', + '--sequence-data', '--binary-upgrade', '--dbname' => 'postgres', # alternative way to specify database ], diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index 23fe7280a16..b8fd0d0acee 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -52,7 +52,7 @@ generate_old_dump(void) snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid); parallel_exec_prog(log_file_name, NULL, - "\"%s/pg_dump\" %s --no-data %s --quote-all-identifiers " + "\"%s/pg_dump\" %s --no-data %s --sequence-data --quote-all-identifiers " "--binary-upgrade --format=custom %s --no-sync --file=\"%s/%s\" %s", new_cluster.bindir, cluster_conn_opts(&old_cluster), log_opts.verbose ? "--verbose" : "", diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl index 9b2a90b0469..27c6c2ab0f3 100644 --- a/src/test/modules/test_pg_dump/t/001_base.pl +++ b/src/test/modules/test_pg_dump/t/001_base.pl @@ -48,7 +48,7 @@ my %pgdump_runs = ( dump_cmd => [ 'pg_dump', '--no-sync', "--file=$tempdir/binary_upgrade.sql", '--schema-only', - '--binary-upgrade', '--dbname=postgres', + '--sequence-data', '--binary-upgrade', '--dbname=postgres', ], }, clean => { -- 2.39.5 (Apple Git-154) From ecd5b53daefd3187195e5a8fbf47e0a8a278bf30 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 5 Mar 2025 17:36:54 -0600 Subject: [PATCH v4 3/3] pg_upgrade: Add --swap for faster file transfer. This new option instructs pg_upgrade to move the data directories from the old cluster to the new cluster and then to replace the catalog files with those generated for the new cluster. This mode can outperform --link, --clone, --copy, and --copy-file-range, especially on clusters with many relations. However, this mode creates many garbage files in the old cluster, which can prolong the file synchronization step. To handle that, we use "initdb --sync-only --no-sync-data-files" for file synchronization, and we synchronize the catalog files as they are transferred. We assume that the database files transferred from the old cluster were synchronized prior to upgrade. This mode also complicates reverting to the old cluster, so we recommend restoring from backup upon failure during or after file transfer. The new mode is limited to clusters located in the same file system and to upgrades from version 10 and newer. Discussion: https://postgr.es/m/Zyvop-LxLXBLrZil%40nathan --- doc/src/sgml/ref/pgupgrade.sgml | 59 ++++- src/bin/pg_upgrade/check.c | 29 ++- src/bin/pg_upgrade/controldata.c | 23 +- src/bin/pg_upgrade/dump.c | 4 +- src/bin/pg_upgrade/file.c | 16 +- src/bin/pg_upgrade/info.c | 4 +- src/bin/pg_upgrade/option.c | 7 + src/bin/pg_upgrade/pg_upgrade.c | 16 +- src/bin/pg_upgrade/pg_upgrade.h | 5 +- src/bin/pg_upgrade/relfilenumber.c | 358 +++++++++++++++++++++++++++++ src/common/file_utils.c | 14 +- src/include/common/file_utils.h | 1 + 12 files changed, 505 insertions(+), 31 deletions(-) diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml index 7bdd85c5cff..08278232e71 100644 --- a/doc/src/sgml/ref/pgupgrade.sgml +++ b/doc/src/sgml/ref/pgupgrade.sgml @@ -244,7 +244,8 @@ PostgreSQL documentation <listitem> <para> Copy files to the new cluster. This is the default. (See also - <option>--link</option> and <option>--clone</option>.) + <option>--link</option>, <option>--clone</option>, + <option>--copy-file-range</option>, and <option>--swap</option>.) </para> </listitem> </varlistentry> @@ -262,6 +263,32 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>--swap</option></term> + <listitem> + <para> + Move the data directories from the old cluster to the new cluster. + Then, replace the catalog files with those generated for the new + cluster. This mode can outperform <option>--link</option>, + <option>--clone</option>, <option>--copy</option>, and + <option>--copy-file-range</option>, especially on clusters with many + relations. + </para> + <para> + However, this mode creates many garbage files in the old cluster, which + can prolong the file synchronization step if + <option>--sync-method=syncfs</option> is used. Therefore, it is + recommended to use <option>--sync-method=fsync</option> with + <option>--swap</option>. + </para> + <para> + Additionally, once the file transfer step begins, the old cluster will + be destructively modified and therefore will no longer be safe to + start. See <xref linkend="pgupgrade-step-revert"/> for details. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>--sync-method=</option><replaceable>method</replaceable></term> <listitem> @@ -530,6 +557,10 @@ NET STOP postgresql-&majorversion; is started. Clone mode also requires that the old and new data directories be in the same file system. This mode is only available on certain operating systems and file systems. + Swap mode may be the fastest if there are many relations, but you will not + be able to access your old cluster once the file transfer step begins. + Swap mode also requires that the old and new cluster data directories be + in the same file system. </para> <para> @@ -889,6 +920,32 @@ psql --username=postgres --file=script.sql postgres </itemizedlist></para> </listitem> + + <listitem> + <para> + If the <option>--swap</option> option was used, the old cluster might + be destructively modified: + + <itemizedlist> + <listitem> + <para> + If <command>pg_upgrade</command> aborts before reporting that the + old cluster is no longer safe to start, the old cluster was + unmodified; it can be restarted. + </para> + </listitem> + + <listitem> + <para> + If <command>pg_upgrade</command> has reported that the old cluster + is no longer safe to start, the old cluster was destructively + modified. The old cluster will need to be restored from backup in + this case. + </para> + </listitem> + </itemizedlist> + </para> + </listitem> </itemizedlist></para> </step> </procedure> diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 88db8869b6e..5405ed7bc8f 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -709,7 +709,34 @@ check_new_cluster(void) check_copy_file_range(); break; case TRANSFER_MODE_LINK: - check_hard_link(); + check_hard_link(TRANSFER_MODE_LINK); + break; + case TRANSFER_MODE_SWAP: + + /* + * We do the hard link check for --swap, too, since it's an easy + * way to verify the clusters are in the same file system. This + * allows us to take some shortcuts in the file synchronization + * step. With some more effort, we could probably support the + * separate-file-system use case, but this mode is unlikely to + * offer much benefit if we have to copy the files across file + * system boundaries. + */ + check_hard_link(TRANSFER_MODE_SWAP); + + /* + * There are a few known issues with using --swap to upgrade from + * versions older than 10. For example, the sequence tuple format + * changed in v10, and the visibility map format changed in 9.6. + * While such problems are not insurmountable (and we may have to + * deal with similar problems in the future, anyway), it doesn't + * seem worth the effort to support swap mode for upgrades from + * long-unsupported versions. + */ + if (GET_MAJOR_VERSION(old_cluster.major_version) < 1000) + pg_fatal("Swap mode can only upgrade clusters from PostgreSQL version %s and later.", + "10"); + break; } diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c index bd49ea867bf..391ed3e1085 100644 --- a/src/bin/pg_upgrade/controldata.c +++ b/src/bin/pg_upgrade/controldata.c @@ -751,11 +751,15 @@ check_control_data(ControlData *oldctrl, void -disable_old_cluster(void) +disable_old_cluster(transferMode transfer_mode) { char old_path[MAXPGPATH], new_path[MAXPGPATH]; + /* only used for --link and --swap */ + Assert(transfer_mode == TRANSFER_MODE_LINK || + transfer_mode == TRANSFER_MODE_SWAP); + /* rename pg_control so old server cannot be accidentally started */ prep_status("Adding \".old\" suffix to old global/pg_control"); @@ -766,10 +770,15 @@ disable_old_cluster(void) old_path, new_path); check_ok(); - pg_log(PG_REPORT, "\n" - "If you want to start the old cluster, you will need to remove\n" - "the \".old\" suffix from %s/global/pg_control.old.\n" - "Because \"link\" mode was used, the old cluster cannot be safely\n" - "started once the new cluster has been started.", - old_cluster.pgdata); + if (transfer_mode == TRANSFER_MODE_LINK) + pg_log(PG_REPORT, "\n" + "If you want to start the old cluster, you will need to remove\n" + "the \".old\" suffix from %s/global/pg_control.old.\n" + "Because \"link\" mode was used, the old cluster cannot be safely\n" + "started once the new cluster has been started.", + old_cluster.pgdata); + else + pg_log(PG_REPORT, "\n" + "Because \"swap\" mode was used, the old cluster can no longer be\n" + "safely started."); } diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index b8fd0d0acee..23cb08e8347 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -52,9 +52,11 @@ generate_old_dump(void) snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid); parallel_exec_prog(log_file_name, NULL, - "\"%s/pg_dump\" %s --no-data %s --sequence-data --quote-all-identifiers " + "\"%s/pg_dump\" %s --no-data %s %s --quote-all-identifiers " "--binary-upgrade --format=custom %s --no-sync --file=\"%s/%s\" %s", new_cluster.bindir, cluster_conn_opts(&old_cluster), + (user_opts.transfer_mode == TRANSFER_MODE_SWAP) ? + "" : "--sequence-data", log_opts.verbose ? "--verbose" : "", user_opts.do_statistics ? "" : "--no-statistics", log_opts.dumpdir, diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c index 7fd1991204a..4fe784e8b94 100644 --- a/src/bin/pg_upgrade/file.c +++ b/src/bin/pg_upgrade/file.c @@ -434,18 +434,28 @@ check_copy_file_range(void) } void -check_hard_link(void) +check_hard_link(transferMode transfer_mode) { char existing_file[MAXPGPATH]; char new_link_file[MAXPGPATH]; + /* only used for --link and --swap */ + Assert(transfer_mode == TRANSFER_MODE_LINK || + transfer_mode == TRANSFER_MODE_SWAP); + snprintf(existing_file, sizeof(existing_file), "%s/PG_VERSION", old_cluster.pgdata); snprintf(new_link_file, sizeof(new_link_file), "%s/PG_VERSION.linktest", new_cluster.pgdata); unlink(new_link_file); /* might fail */ if (link(existing_file, new_link_file) < 0) - pg_fatal("could not create hard link between old and new data directories: %m\n" - "In link mode the old and new data directories must be on the same file system."); + { + if (transfer_mode == TRANSFER_MODE_LINK) + pg_fatal("could not create hard link between old and new data directories: %m\n" + "In link mode the old and new data directories must be on the same file system."); + else + pg_fatal("could not create hard link between old and new data directories: %m\n" + "In swap mode the old and new data directories must be on the same file system."); + } unlink(new_link_file); } diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c index ad52de8b607..4b7a56f5b3b 100644 --- a/src/bin/pg_upgrade/info.c +++ b/src/bin/pg_upgrade/info.c @@ -490,7 +490,7 @@ get_rel_infos_query(void) " FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n " " ON c.relnamespace = n.oid " " WHERE relkind IN (" CppAsString2(RELKIND_RELATION) ", " - CppAsString2(RELKIND_MATVIEW) ") AND " + CppAsString2(RELKIND_MATVIEW) "%s) AND " /* exclude possible orphaned temp tables */ " ((n.nspname !~ '^pg_temp_' AND " " n.nspname !~ '^pg_toast_temp_' AND " @@ -499,6 +499,8 @@ get_rel_infos_query(void) " c.oid >= %u::pg_catalog.oid) OR " " (n.nspname = 'pg_catalog' AND " " relname IN ('pg_largeobject') ))), ", + (user_opts.transfer_mode == TRANSFER_MODE_SWAP) ? + ", " CppAsString2(RELKIND_SEQUENCE) : "", FirstNormalObjectId); /* diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index 188dd8d8a8b..7fd7f1d33fc 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -62,6 +62,7 @@ parseCommandLine(int argc, char *argv[]) {"sync-method", required_argument, NULL, 4}, {"no-statistics", no_argument, NULL, 5}, {"set-char-signedness", required_argument, NULL, 6}, + {"swap", no_argument, NULL, 7}, {NULL, 0, NULL, 0} }; @@ -228,6 +229,11 @@ parseCommandLine(int argc, char *argv[]) else pg_fatal("invalid argument for option %s", "--set-char-signedness"); break; + + case 7: + user_opts.transfer_mode = TRANSFER_MODE_SWAP; + break; + default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), os_info.progname); @@ -325,6 +331,7 @@ usage(void) printf(_(" --no-statistics do not import statistics from old cluster\n")); printf(_(" --set-char-signedness=OPTION set new cluster char signedness to \"signed\" or\n" " \"unsigned\"\n")); + printf(_(" --swap move data directories to new cluster\n")); printf(_(" --sync-method=METHOD set method for syncing files to disk\n")); printf(_(" -?, --help show this help, then exit\n")); printf(_("\n" diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index 174cd920840..9295e46aed3 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -170,12 +170,14 @@ main(int argc, char **argv) /* * Most failures happen in create_new_objects(), which has completed at - * this point. We do this here because it is just before linking, which - * will link the old and new cluster data files, preventing the old - * cluster from being safely started once the new cluster is started. + * this point. We do this here because it is just before file transfer, + * which for --link will make it unsafe to start the old cluster once the + * new cluster is started, and for --swap will make it unsafe to start the + * old cluster at all. */ - if (user_opts.transfer_mode == TRANSFER_MODE_LINK) - disable_old_cluster(); + if (user_opts.transfer_mode == TRANSFER_MODE_LINK || + user_opts.transfer_mode == TRANSFER_MODE_SWAP) + disable_old_cluster(user_opts.transfer_mode); transfer_all_new_tablespaces(&old_cluster.dbarr, &new_cluster.dbarr, old_cluster.pgdata, new_cluster.pgdata); @@ -212,8 +214,10 @@ main(int argc, char **argv) { prep_status("Sync data directory to disk"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/initdb\" --sync-only \"%s\" --sync-method %s", + "\"%s/initdb\" --sync-only %s \"%s\" --sync-method %s", new_cluster.bindir, + (user_opts.transfer_mode == TRANSFER_MODE_SWAP) ? + "--no-sync-data-files" : "", new_cluster.pgdata, user_opts.sync_method); check_ok(); diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index f4e375d27c7..120c38929d4 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -262,6 +262,7 @@ typedef enum TRANSFER_MODE_COPY, TRANSFER_MODE_COPY_FILE_RANGE, TRANSFER_MODE_LINK, + TRANSFER_MODE_SWAP, } transferMode; /* @@ -391,7 +392,7 @@ void create_script_for_old_cluster_deletion(char **deletion_script_file_name); void get_control_data(ClusterInfo *cluster); void check_control_data(ControlData *oldctrl, ControlData *newctrl); -void disable_old_cluster(void); +void disable_old_cluster(transferMode transfer_mode); /* dump.c */ @@ -423,7 +424,7 @@ void rewriteVisibilityMap(const char *fromfile, const char *tofile, const char *schemaName, const char *relName); void check_file_clone(void); void check_copy_file_range(void); -void check_hard_link(void); +void check_hard_link(transferMode transfer_mode); /* fopen_priv() is no longer different from fopen() */ #define fopen_priv(path, mode) fopen(path, mode) diff --git a/src/bin/pg_upgrade/relfilenumber.c b/src/bin/pg_upgrade/relfilenumber.c index 8c23c583172..2abe90dd239 100644 --- a/src/bin/pg_upgrade/relfilenumber.c +++ b/src/bin/pg_upgrade/relfilenumber.c @@ -11,11 +11,92 @@ #include <sys/stat.h> +#include "common/file_perm.h" +#include "common/file_utils.h" +#include "common/int.h" +#include "common/logging.h" #include "pg_upgrade.h" static void transfer_single_new_db(FileNameMap *maps, int size, char *old_tablespace); static void transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_frozenbit); +/* + * The following set of sync_queue_* functions are used for --swap to reduce + * the amount of time spent synchronizing the swapped catalog files. When a + * file is added to the queue, we also alert the file system that we'd like it + * to be persisted to disk in the near future (if that operation is supported + * by the current platform). Once the queue is full, all of the files are + * synchronized to disk. This strategy should generally be much faster than + * simply calling fsync() on the files right away. + * + * The general usage pattern should be something like: + * + * for (int i = 0; i < num_files; i++) + * sync_queue_push(files[i]); + * + * // be sure to sync any remaining files in the queue + * sync_queue_sync_all(); + * synq_queue_destroy(); + */ + +#define SYNC_QUEUE_MAX_LEN (1024) + +static char *sync_queue[SYNC_QUEUE_MAX_LEN]; +static bool sync_queue_inited; +static int sync_queue_len; + +static inline void +sync_queue_init(void) +{ + if (sync_queue_inited) + return; + + sync_queue_inited = true; + for (int i = 0; i < SYNC_QUEUE_MAX_LEN; i++) + sync_queue[i] = palloc(MAXPGPATH); +} + +static inline void +sync_queue_sync_all(void) +{ + if (!sync_queue_inited) + return; + + for (int i = 0; i < sync_queue_len; i++) + { + if (fsync_fname(sync_queue[i], false) != 0) + pg_fatal("could not synchronize file \"%s\": %m", sync_queue[i]); + } + + sync_queue_len = 0; +} + +static inline void +sync_queue_push(const char *fname) +{ + sync_queue_init(); + + pre_sync_fname(fname, false); + + strncpy(sync_queue[sync_queue_len++], fname, MAXPGPATH); + if (sync_queue_len >= SYNC_QUEUE_MAX_LEN) + sync_queue_sync_all(); +} + +static inline void +sync_queue_destroy(void) +{ + if (!sync_queue_inited) + return; + + sync_queue_inited = false; + sync_queue_len = 0; + for (int i = 0; i < SYNC_QUEUE_MAX_LEN; i++) + { + pfree(sync_queue[i]); + sync_queue[i] = NULL; + } +} /* * transfer_all_new_tablespaces() @@ -41,6 +122,9 @@ transfer_all_new_tablespaces(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, case TRANSFER_MODE_LINK: prep_status_progress("Linking user relation files"); break; + case TRANSFER_MODE_SWAP: + prep_status_progress("Swapping data directories"); + break; } /* @@ -125,6 +209,261 @@ transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, /* We allocate something even for n_maps == 0 */ pg_free(mappings); } + + /* + * Make sure anything pending synchronization in swap mode is fully + * persisted to disk. This is a no-op for other transfer modes. + */ + sync_queue_sync_all(); + sync_queue_destroy(); +} + +/* + * prepare_for_swap() + * + * This function durably moves the database directories from the old cluster to + * the new cluster in preparation for moving the pg_restore-generated catalog + * files into place. Returns false if the database with the given OID does not + * have a directory in the given tablespace, otherwise returns true. + */ +static bool +prepare_for_swap(const char *old_tablespace, Oid db_oid, + char *old_cat, char *new_dat, char *moved_dat) +{ + const char *new_tablespace; + const char *old_tblspc_suffix; + const char *new_tblspc_suffix; + char old_tblspc[MAXPGPATH]; + char new_tblspc[MAXPGPATH]; + char moved_tblspc[MAXPGPATH]; + char old_dat[MAXPGPATH]; + struct stat st; + + if (strcmp(old_tablespace, old_cluster.pgdata) == 0) + { + new_tablespace = new_cluster.pgdata; + new_tblspc_suffix = "/base"; + old_tblspc_suffix = "/base"; + } + else + { + new_tablespace = old_tablespace; + new_tblspc_suffix = new_cluster.tablespace_suffix; + old_tblspc_suffix = old_cluster.tablespace_suffix; + } + + snprintf(old_tblspc, sizeof(old_tblspc), "%s%s", old_tablespace, old_tblspc_suffix); + snprintf(moved_tblspc, sizeof(moved_tblspc), "%s_moved", old_tblspc); + snprintf(old_cat, MAXPGPATH, "%s/%u_old_cat", moved_tblspc, db_oid); + snprintf(new_tblspc, sizeof(new_tblspc), "%s%s", new_tablespace, new_tblspc_suffix); + snprintf(new_dat, MAXPGPATH, "%s/%u", new_tblspc, db_oid); + snprintf(moved_dat, MAXPGPATH, "%s/%u", moved_tblspc, db_oid); + snprintf(old_dat, sizeof(old_dat), "%s/%u", old_tblspc, db_oid); + + /* Check that the database directory exists in the given tablespace. */ + if (stat(old_dat, &st) != 0) + { + if (errno != ENOENT) + pg_fatal("could not stat file \"%s\": %m", old_dat); + return false; + } + + /* Create directory for stuff that is moved aside. */ + if (pg_mkdir_p(moved_tblspc, pg_dir_create_mode) != 0 && errno != EEXIST) + pg_fatal("could not create directory \"%s\"", moved_tblspc); + + /* Create directory for old catalog files. */ + if (pg_mkdir_p(old_cat, pg_dir_create_mode) != 0) + pg_fatal("could not create directory \"%s\"", old_cat); + + /* Move the new cluster's database directory aside. */ + if (rename(new_dat, moved_dat) != 0) + pg_fatal("could not rename \"%s\" to \"%s\"", new_dat, moved_dat); + + /* Move the old cluster's database directory into place. */ + if (rename(old_dat, new_dat) != 0) + pg_fatal("could not rename \"%s\" to \"%s\"", old_dat, new_dat); + + return true; +} + +/* + * FileNameMapCmp() + * + * qsort() comparator for FileNameMap that sorts by RelFileNumber. + */ +static int +FileNameMapCmp(const void *a, const void *b) +{ + const FileNameMap *map1 = (const FileNameMap *) a; + const FileNameMap *map2 = (const FileNameMap *) b; + + return pg_cmp_u32(map1->relfilenumber, map2->relfilenumber); +} + +/* + * parse_relfilenumber() + * + * Attempt to parse the RelFileNumber of the given file name. If we can't, + * return InvalidRelFileNumber. + */ +static RelFileNumber +parse_relfilenumber(const char *filename) +{ + char *endp; + unsigned long n; + + if (filename[0] < '1' || filename[0] > '9') + return InvalidRelFileNumber; + + errno = 0; + n = strtoul(filename, &endp, 10); + if (errno || filename == endp || n <= 0 || n > PG_UINT32_MAX) + return InvalidRelFileNumber; + + return (RelFileNumber) n; +} + +/* + * swap_catalog_files() + * + * Moves the old catalog files aside, and moves the new catalog files into + * place. + */ +static void +swap_catalog_files(FileNameMap *maps, int size, const char *old_cat, + const char *new_dat, const char *moved_dat) +{ + DIR *dir; + struct dirent *de; + char path[MAXPGPATH]; + char dest[MAXPGPATH]; + RelFileNumber rfn; + + /* Move the old catalog files aside. */ + dir = opendir(new_dat); + if (dir == NULL) + pg_fatal("could not open directory \"%s\": %m", new_dat); + while (errno = 0, (de = readdir(dir)) != NULL) + { + if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0) + continue; + + snprintf(path, sizeof(path), "%s/%s", new_dat, de->d_name); + if (get_dirent_type(path, de, false, PG_LOG_ERROR) != PGFILETYPE_REG) + continue; + + rfn = parse_relfilenumber(de->d_name); + if (RelFileNumberIsValid(rfn)) + { + FileNameMap key = {.relfilenumber = rfn}; + + if (bsearch(&key, maps, size, sizeof(FileNameMap), FileNameMapCmp)) + continue; + } + + snprintf(dest, sizeof(dest), "%s/%s", old_cat, de->d_name); + if (rename(path, dest) != 0) + pg_fatal("could not rename \"%s\" to \"%s\": %m", path, dest); + } + + if (errno) + pg_fatal("could not read directory \"%s\": %m", new_dat); + (void) closedir(dir); + + /* Move the new catalog files into place. */ + dir = opendir(moved_dat); + if (dir == NULL) + pg_fatal("could not open directory \"%s\": %m", moved_dat); + while (errno = 0, (de = readdir(dir)) != NULL) + { + if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0) + continue; + + snprintf(path, sizeof(path), "%s/%s", moved_dat, de->d_name); + if (get_dirent_type(path, de, false, PG_LOG_ERROR) != PGFILETYPE_REG) + continue; + + rfn = parse_relfilenumber(de->d_name); + if (RelFileNumberIsValid(rfn)) + { + FileNameMap key = {.relfilenumber = rfn}; + + if (bsearch(&key, maps, size, sizeof(FileNameMap), FileNameMapCmp)) + continue; + } + + snprintf(dest, sizeof(dest), "%s/%s", new_dat, de->d_name); + if (rename(path, dest) != 0) + pg_fatal("could not rename \"%s\" to \"%s\": %m", path, dest); + + /* + * We don't fsync() the database files in the file synchronization + * stage of pg_upgrade in swap mode, so we need to synchronize them + * ourselves. We only do this for the catalog files because they were + * created during pg_restore with fsync=off. We assume that the user + * data files files were properly persisted to disk when the user last + * shut it down. + */ + sync_queue_push(dest); + } + + if (errno) + pg_fatal("could not read directory \"%s\": %m", moved_dat); + (void) closedir(dir); + + /* Ensure the directory entries are persisted to disk. */ + if (fsync_fname(new_dat, true) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", new_dat); + if (fsync_parent_path(new_dat) != 0) + pg_fatal("could not synchronize parent directory of \"%s\": %m", new_dat); +} + +/* + * do_swap() + * + * Perform the required steps for --swap for a single database. In short this + * moves the old cluster's database directory into the new cluster and then + * replaces any files for system catalogs with the ones that were generated + * during pg_restore. + */ +static void +do_swap(FileNameMap *maps, int size, char *old_tablespace) +{ + char old_cat[MAXPGPATH]; + char new_dat[MAXPGPATH]; + char moved_dat[MAXPGPATH]; + + /* + * We perform many lookups on maps by relfilenumber in swap mode, so make + * sure it's sorted. + */ + qsort(maps, size, sizeof(FileNameMap), FileNameMapCmp); + + /* + * If an old tablespace is given, we only need to process that one. If no + * old tablespace is specified, we need to process all the tablespaces on + * the system. + */ + if (old_tablespace) + { + if (prepare_for_swap(old_tablespace, maps[0].db_oid, + old_cat, new_dat, moved_dat)) + swap_catalog_files(maps, size, old_cat, new_dat, moved_dat); + } + else + { + if (prepare_for_swap(old_cluster.pgdata, maps[0].db_oid, + old_cat, new_dat, moved_dat)) + swap_catalog_files(maps, size, old_cat, new_dat, moved_dat); + + for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++) + { + if (prepare_for_swap(os_info.old_tablespaces[tblnum], maps[0].db_oid, + old_cat, new_dat, moved_dat)) + swap_catalog_files(maps, size, old_cat, new_dat, moved_dat); + } + } } /* @@ -145,6 +484,20 @@ transfer_single_new_db(FileNameMap *maps, int size, char *old_tablespace) new_cluster.controldata.cat_ver >= VISIBILITY_MAP_FROZEN_BIT_CAT_VER) vm_must_add_frozenbit = true; + /* --swap has its own subroutine */ + if (user_opts.transfer_mode == TRANSFER_MODE_SWAP) + { + /* + * We don't support --swap to upgrade from versions that require + * rewriting the visibility map. We should've failed already if + * someone tries to do that. + */ + Assert(!vm_must_add_frozenbit); + + do_swap(maps, size, old_tablespace); + return; + } + for (mapnum = 0; mapnum < size; mapnum++) { if (old_tablespace == NULL || @@ -259,6 +612,11 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro pg_log(PG_VERBOSE, "linking \"%s\" to \"%s\"", old_file, new_file); linkFile(old_file, new_file, map->nspname, map->relname); + break; + case TRANSFER_MODE_SWAP: + /* swap mode is handled in its own code path */ + pg_fatal("should never happen"); + break; } } } diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 78e272916f5..4405ef8b425 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -45,9 +45,6 @@ */ #define MINIMUM_VERSION_FOR_PG_WAL 100000 -#ifdef PG_FLUSH_DATA_WORKS -static int pre_sync_fname(const char *fname, bool isdir); -#endif static void walkdir(const char *path, int (*action) (const char *fname, bool isdir), bool process_symlinks, @@ -352,16 +349,16 @@ walkdir(const char *path, } /* - * Hint to the OS that it should get ready to fsync() this file. + * Hint to the OS that it should get ready to fsync() this file, if supported + * by the platform. * * Ignores errors trying to open unreadable files, and reports other errors * non-fatally. */ -#ifdef PG_FLUSH_DATA_WORKS - -static int +int pre_sync_fname(const char *fname, bool isdir) { +#ifdef PG_FLUSH_DATA_WORKS int fd; fd = open(fname, O_RDONLY | PG_BINARY, 0); @@ -388,11 +385,10 @@ pre_sync_fname(const char *fname, bool isdir) #endif (void) close(fd); +#endif /* PG_FLUSH_DATA_WORKS */ return 0; } -#endif /* PG_FLUSH_DATA_WORKS */ - /* * fsync_fname -- Try to fsync a file or directory * diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index 8274bc877ab..9fd88953e43 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -33,6 +33,7 @@ typedef enum DataDirSyncMethod struct iovec; /* avoid including port/pg_iovec.h here */ #ifdef FRONTEND +extern int pre_sync_fname(const char *fname, bool isdir); extern int fsync_fname(const char *fname, bool isdir); extern void sync_pgdata(const char *pg_data, int serverVersion, DataDirSyncMethod sync_method, bool sync_data_files); -- 2.39.5 (Apple Git-154) Attachments: [text/plain] v4-0001-initdb-Add-no-sync-data-files.patch (13.0K, ../../Z8kJveW4yBOI7Jtx@nathan/2-v4-0001-initdb-Add-no-sync-data-files.patch) download | inline diff: From 6716e7b16a795911f55432dfd6d3c246aa8fd9fe Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 19 Feb 2025 09:14:51 -0600 Subject: [PATCH v4 1/3] initdb: Add --no-sync-data-files. This new option instructs initdb to skip synchronizing any files in database directories and the database directories themselves, i.e., everything in the base/ subdirectory and any other tablespace directories. Other files, such as those in pg_wal/ and pg_xact/, will still be synchronized unless --no-sync is also specified. --no-sync-data-files is primarily intended for internal use by tools that separately ensure the skipped files are synchronized to disk. A follow-up commit will use this to help optimize pg_upgrade's file transfer step. Discussion: https://postgr.es/m/Zyvop-LxLXBLrZil%40nathan --- doc/src/sgml/ref/initdb.sgml | 20 +++++ src/bin/initdb/initdb.c | 10 ++- src/bin/initdb/t/001_initdb.pl | 1 + src/bin/pg_basebackup/pg_basebackup.c | 2 +- src/bin/pg_checksums/pg_checksums.c | 2 +- src/bin/pg_combinebackup/pg_combinebackup.c | 2 +- src/bin/pg_rewind/file_ops.c | 2 +- src/common/file_utils.c | 85 +++++++++++++-------- src/include/common/file_utils.h | 2 +- 9 files changed, 89 insertions(+), 37 deletions(-) diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml index 0026318485a..14c401b9a99 100644 --- a/doc/src/sgml/ref/initdb.sgml +++ b/doc/src/sgml/ref/initdb.sgml @@ -527,6 +527,26 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry id="app-initdb-option-no-sync-data-files"> + <term><option>--no-sync-data-files</option></term> + <listitem> + <para> + By default, <command>initdb</command> safely writes all database files + to disk. This option instructs <command>initdb</command> to skip + synchronizing all files in the individual database directories and the + database directories themselves, i.e., everything in the + <filename>base</filename> subdirectory and any other tablespace + directories. Other files, such as those in <literal>pg_wal</literal> + and <literal>pg_xact</literal>, will still be synchronized unless the + <option>--no-sync</option> option is also specified. + </para> + <para> + This option is primarily intended for internal use by tools that + separately ensure the skipped files are synchronized to disk. + </para> + </listitem> + </varlistentry> + <varlistentry id="app-initdb-option-no-instructions"> <term><option>--no-instructions</option></term> <listitem> diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 21a0fe3ecd9..22b7d31b165 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -168,6 +168,7 @@ static bool data_checksums = true; static char *xlog_dir = NULL; static int wal_segment_size_mb = (DEFAULT_XLOG_SEG_SIZE) / (1024 * 1024); static DataDirSyncMethod sync_method = DATA_DIR_SYNC_METHOD_FSYNC; +static bool sync_data_files = true; /* internal vars */ @@ -2566,6 +2567,7 @@ usage(const char *progname) printf(_(" -L DIRECTORY where to find the input files\n")); printf(_(" -n, --no-clean do not clean up after errors\n")); printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n")); + printf(_(" --no-sync-data-files do not sync files within database directories\n")); printf(_(" --no-instructions do not print instructions for next steps\n")); printf(_(" -s, --show show internal settings, then exit\n")); printf(_(" --sync-method=METHOD set method for syncing files to disk\n")); @@ -3208,6 +3210,7 @@ main(int argc, char *argv[]) {"icu-rules", required_argument, NULL, 18}, {"sync-method", required_argument, NULL, 19}, {"no-data-checksums", no_argument, NULL, 20}, + {"no-sync-data-files", no_argument, NULL, 21}, {NULL, 0, NULL, 0} }; @@ -3402,6 +3405,9 @@ main(int argc, char *argv[]) case 20: data_checksums = false; break; + case 21: + sync_data_files = false; + break; default: /* getopt_long already emitted a complaint */ pg_log_error_hint("Try \"%s --help\" for more information.", progname); @@ -3453,7 +3459,7 @@ main(int argc, char *argv[]) fputs(_("syncing data to disk ... "), stdout); fflush(stdout); - sync_pgdata(pg_data, PG_VERSION_NUM, sync_method); + sync_pgdata(pg_data, PG_VERSION_NUM, sync_method, sync_data_files); check_ok(); return 0; } @@ -3516,7 +3522,7 @@ main(int argc, char *argv[]) { fputs(_("syncing data to disk ... "), stdout); fflush(stdout); - sync_pgdata(pg_data, PG_VERSION_NUM, sync_method); + sync_pgdata(pg_data, PG_VERSION_NUM, sync_method, sync_data_files); check_ok(); } else diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl index 01cc4a1602b..15dd10ce40a 100644 --- a/src/bin/initdb/t/001_initdb.pl +++ b/src/bin/initdb/t/001_initdb.pl @@ -76,6 +76,7 @@ command_like( 'checksums are enabled in control file'); command_ok([ 'initdb', '--sync-only', $datadir ], 'sync only'); +command_ok([ 'initdb', '--sync-only', '--no-sync-data-files', $datadir ], '--no-sync-data-files'); command_fails([ 'initdb', $datadir ], 'existing data directory'); if ($supports_syncfs) diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index dc0c805137a..bc94c114d27 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -2310,7 +2310,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail, } else { - (void) sync_pgdata(basedir, serverVersion, sync_method); + (void) sync_pgdata(basedir, serverVersion, sync_method, true); } } diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 867aeddc601..f20be82862a 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -633,7 +633,7 @@ main(int argc, char *argv[]) if (do_sync) { pg_log_info("syncing data directory"); - sync_pgdata(DataDir, PG_VERSION_NUM, sync_method); + sync_pgdata(DataDir, PG_VERSION_NUM, sync_method, true); } pg_log_info("updating control file"); diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index 5864ec574fb..c0ec09485c3 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -420,7 +420,7 @@ main(int argc, char *argv[]) else { pg_log_debug("recursively fsyncing \"%s\"", opt.output); - sync_pgdata(opt.output, version * 10000, opt.sync_method); + sync_pgdata(opt.output, version * 10000, opt.sync_method, true); } } diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c index 467845419ed..55659ce201f 100644 --- a/src/bin/pg_rewind/file_ops.c +++ b/src/bin/pg_rewind/file_ops.c @@ -296,7 +296,7 @@ sync_target_dir(void) if (!do_sync || dry_run) return; - sync_pgdata(datadir_target, PG_VERSION_NUM, sync_method); + sync_pgdata(datadir_target, PG_VERSION_NUM, sync_method, true); } diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 0e3cfede935..78e272916f5 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -50,7 +50,8 @@ static int pre_sync_fname(const char *fname, bool isdir); #endif static void walkdir(const char *path, int (*action) (const char *fname, bool isdir), - bool process_symlinks); + bool process_symlinks, + const char *exclude_dir); #ifdef HAVE_SYNCFS @@ -93,11 +94,15 @@ do_syncfs(const char *path) * syncing, and might not have privileges to write at all. * * serverVersion indicates the version of the server to be sync'd. + * + * If sync_data_files is false, this function skips syncing "base/" and any + * other tablespace directories. */ void sync_pgdata(const char *pg_data, int serverVersion, - DataDirSyncMethod sync_method) + DataDirSyncMethod sync_method, + bool sync_data_files) { bool xlog_is_symlink; char pg_wal[MAXPGPATH]; @@ -147,30 +152,33 @@ sync_pgdata(const char *pg_data, do_syncfs(pg_data); /* If any tablespaces are configured, sync each of those. */ - dir = opendir(pg_tblspc); - if (dir == NULL) - pg_log_error("could not open directory \"%s\": %m", - pg_tblspc); - else + if (sync_data_files) { - while (errno = 0, (de = readdir(dir)) != NULL) + dir = opendir(pg_tblspc); + if (dir == NULL) + pg_log_error("could not open directory \"%s\": %m", + pg_tblspc); + else { - char subpath[MAXPGPATH * 2]; + while (errno = 0, (de = readdir(dir)) != NULL) + { + char subpath[MAXPGPATH * 2]; - if (strcmp(de->d_name, ".") == 0 || - strcmp(de->d_name, "..") == 0) - continue; + if (strcmp(de->d_name, ".") == 0 || + strcmp(de->d_name, "..") == 0) + continue; - snprintf(subpath, sizeof(subpath), "%s/%s", - pg_tblspc, de->d_name); - do_syncfs(subpath); - } + snprintf(subpath, sizeof(subpath), "%s/%s", + pg_tblspc, de->d_name); + do_syncfs(subpath); + } - if (errno) - pg_log_error("could not read directory \"%s\": %m", - pg_tblspc); + if (errno) + pg_log_error("could not read directory \"%s\": %m", + pg_tblspc); - (void) closedir(dir); + (void) closedir(dir); + } } /* If pg_wal is a symlink, process that too. */ @@ -182,15 +190,21 @@ sync_pgdata(const char *pg_data, case DATA_DIR_SYNC_METHOD_FSYNC: { + char *exclude_dir = NULL; + + if (!sync_data_files) + exclude_dir = psprintf("%s/base", pg_data); + /* * If possible, hint to the kernel that we're soon going to * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(pg_data, pre_sync_fname, false); + walkdir(pg_data, pre_sync_fname, false, exclude_dir); if (xlog_is_symlink) - walkdir(pg_wal, pre_sync_fname, false); - walkdir(pg_tblspc, pre_sync_fname, true); + walkdir(pg_wal, pre_sync_fname, false, NULL); + if (sync_data_files) + walkdir(pg_tblspc, pre_sync_fname, true, NULL); #endif /* @@ -203,10 +217,14 @@ sync_pgdata(const char *pg_data, * get fsync'd twice. That's not an expected case so we don't * worry about optimizing it. */ - walkdir(pg_data, fsync_fname, false); + walkdir(pg_data, fsync_fname, false, exclude_dir); if (xlog_is_symlink) - walkdir(pg_wal, fsync_fname, false); - walkdir(pg_tblspc, fsync_fname, true); + walkdir(pg_wal, fsync_fname, false, NULL); + if (sync_data_files) + walkdir(pg_tblspc, fsync_fname, true, NULL); + + if (exclude_dir) + pfree(exclude_dir); } break; } @@ -245,10 +263,10 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(dir, pre_sync_fname, false); + walkdir(dir, pre_sync_fname, false, NULL); #endif - walkdir(dir, fsync_fname, false); + walkdir(dir, fsync_fname, false, NULL); } break; } @@ -264,6 +282,9 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) * ignored in subdirectories, ie we intentionally don't pass down the * process_symlinks flag to recursive calls. * + * If exclude_dir is not NULL, it specifies a directory path to skip + * processing. + * * Errors are reported but not considered fatal. * * See also walkdir in fd.c, which is a backend version of this logic. @@ -271,11 +292,15 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) static void walkdir(const char *path, int (*action) (const char *fname, bool isdir), - bool process_symlinks) + bool process_symlinks, + const char *exclude_dir) { DIR *dir; struct dirent *de; + if (exclude_dir && strcmp(exclude_dir, path) == 0) + return; + dir = opendir(path); if (dir == NULL) { @@ -299,7 +324,7 @@ walkdir(const char *path, (*action) (subpath, false); break; case PGFILETYPE_DIR: - walkdir(subpath, action, false); + walkdir(subpath, action, false, exclude_dir); break; default: diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index a832210adc1..8274bc877ab 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -35,7 +35,7 @@ struct iovec; /* avoid including port/pg_iovec.h here */ #ifdef FRONTEND extern int fsync_fname(const char *fname, bool isdir); extern void sync_pgdata(const char *pg_data, int serverVersion, - DataDirSyncMethod sync_method); + DataDirSyncMethod sync_method, bool sync_data_files); extern void sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method); extern int durable_rename(const char *oldfile, const char *newfile); extern int fsync_parent_path(const char *fname); -- 2.39.5 (Apple Git-154) [text/plain] v4-0002-pg_dump-Add-sequence-data.patch (5.0K, ../../Z8kJveW4yBOI7Jtx@nathan/3-v4-0002-pg_dump-Add-sequence-data.patch) download | inline diff: From e6dc183b8a80a32f6ca52b0d21a173d1b291deea Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 19 Feb 2025 11:25:28 -0600 Subject: [PATCH v4 2/3] pg_dump: Add --sequence-data. This new option instructs pg_dump to dump sequence data when the --no-data, --schema-only, or --statistics-only option is specified. This was originally considered for commit a7e5457db8, but it was left out at that time because there was no known use-case. A follow-up commit will use this to optimize pg_upgrade's file transfer step. Discussion: https://postgr.es/m/Zyvop-LxLXBLrZil%40nathan --- doc/src/sgml/ref/pg_dump.sgml | 11 +++++++++++ src/bin/pg_dump/pg_dump.c | 10 ++-------- src/bin/pg_dump/t/002_pg_dump.pl | 1 + src/bin/pg_upgrade/dump.c | 2 +- src/test/modules/test_pg_dump/t/001_base.pl | 2 +- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml index 1975054d7bf..b05f16995c3 100644 --- a/doc/src/sgml/ref/pg_dump.sgml +++ b/doc/src/sgml/ref/pg_dump.sgml @@ -1289,6 +1289,17 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>--sequence-data</option></term> + <listitem> + <para> + Include sequence data in the dump. This is the default behavior except + when <option>--no-data</option>, <option>--schema-only</option>, or + <option>--statistics-only</option> is specified. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>--serializable-deferrable</option></term> <listitem> diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 4f4ad2ee150..f63215eb3f9 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -517,6 +517,7 @@ main(int argc, char **argv) {"sync-method", required_argument, NULL, 15}, {"filter", required_argument, NULL, 16}, {"exclude-extension", required_argument, NULL, 17}, + {"sequence-data", no_argument, &dopt.sequence_data, 1}, {NULL, 0, NULL, 0} }; @@ -803,14 +804,6 @@ main(int argc, char **argv) if (dopt.column_inserts && dopt.dump_inserts == 0) dopt.dump_inserts = DUMP_DEFAULT_ROWS_PER_INSERT; - /* - * Binary upgrade mode implies dumping sequence data even in schema-only - * mode. This is not exposed as a separate option, but kept separate - * internally for clarity. - */ - if (dopt.binary_upgrade) - dopt.sequence_data = 1; - if (data_only && schema_only) pg_fatal("options -s/--schema-only and -a/--data-only cannot be used together"); if (schema_only && statistics_only) @@ -1275,6 +1268,7 @@ help(const char *progname) printf(_(" --quote-all-identifiers quote all identifiers, even if not key words\n")); printf(_(" --rows-per-insert=NROWS number of rows per INSERT; implies --inserts\n")); printf(_(" --section=SECTION dump named section (pre-data, data, or post-data)\n")); + printf(_(" --sequence-data include sequence data in dump\n")); printf(_(" --serializable-deferrable wait until the dump can run without anomalies\n")); printf(_(" --snapshot=SNAPSHOT use given snapshot for the dump\n")); printf(_(" --statistics-only dump only the statistics, not schema or data\n")); diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index c7bffc1b045..8ae6c5374fc 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -66,6 +66,7 @@ my %pgdump_runs = ( '--file' => "$tempdir/binary_upgrade.dump", '--no-password', '--no-data', + '--sequence-data', '--binary-upgrade', '--dbname' => 'postgres', # alternative way to specify database ], diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index 23fe7280a16..b8fd0d0acee 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -52,7 +52,7 @@ generate_old_dump(void) snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid); parallel_exec_prog(log_file_name, NULL, - "\"%s/pg_dump\" %s --no-data %s --quote-all-identifiers " + "\"%s/pg_dump\" %s --no-data %s --sequence-data --quote-all-identifiers " "--binary-upgrade --format=custom %s --no-sync --file=\"%s/%s\" %s", new_cluster.bindir, cluster_conn_opts(&old_cluster), log_opts.verbose ? "--verbose" : "", diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl index 9b2a90b0469..27c6c2ab0f3 100644 --- a/src/test/modules/test_pg_dump/t/001_base.pl +++ b/src/test/modules/test_pg_dump/t/001_base.pl @@ -48,7 +48,7 @@ my %pgdump_runs = ( dump_cmd => [ 'pg_dump', '--no-sync', "--file=$tempdir/binary_upgrade.sql", '--schema-only', - '--binary-upgrade', '--dbname=postgres', + '--sequence-data', '--binary-upgrade', '--dbname=postgres', ], }, clean => { -- 2.39.5 (Apple Git-154) [text/plain] v4-0003-pg_upgrade-Add-swap-for-faster-file-transfer.patch (28.9K, ../../Z8kJveW4yBOI7Jtx@nathan/4-v4-0003-pg_upgrade-Add-swap-for-faster-file-transfer.patch) download | inline diff: From ecd5b53daefd3187195e5a8fbf47e0a8a278bf30 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 5 Mar 2025 17:36:54 -0600 Subject: [PATCH v4 3/3] pg_upgrade: Add --swap for faster file transfer. This new option instructs pg_upgrade to move the data directories from the old cluster to the new cluster and then to replace the catalog files with those generated for the new cluster. This mode can outperform --link, --clone, --copy, and --copy-file-range, especially on clusters with many relations. However, this mode creates many garbage files in the old cluster, which can prolong the file synchronization step. To handle that, we use "initdb --sync-only --no-sync-data-files" for file synchronization, and we synchronize the catalog files as they are transferred. We assume that the database files transferred from the old cluster were synchronized prior to upgrade. This mode also complicates reverting to the old cluster, so we recommend restoring from backup upon failure during or after file transfer. The new mode is limited to clusters located in the same file system and to upgrades from version 10 and newer. Discussion: https://postgr.es/m/Zyvop-LxLXBLrZil%40nathan --- doc/src/sgml/ref/pgupgrade.sgml | 59 ++++- src/bin/pg_upgrade/check.c | 29 ++- src/bin/pg_upgrade/controldata.c | 23 +- src/bin/pg_upgrade/dump.c | 4 +- src/bin/pg_upgrade/file.c | 16 +- src/bin/pg_upgrade/info.c | 4 +- src/bin/pg_upgrade/option.c | 7 + src/bin/pg_upgrade/pg_upgrade.c | 16 +- src/bin/pg_upgrade/pg_upgrade.h | 5 +- src/bin/pg_upgrade/relfilenumber.c | 358 +++++++++++++++++++++++++++++ src/common/file_utils.c | 14 +- src/include/common/file_utils.h | 1 + 12 files changed, 505 insertions(+), 31 deletions(-) diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml index 7bdd85c5cff..08278232e71 100644 --- a/doc/src/sgml/ref/pgupgrade.sgml +++ b/doc/src/sgml/ref/pgupgrade.sgml @@ -244,7 +244,8 @@ PostgreSQL documentation <listitem> <para> Copy files to the new cluster. This is the default. (See also - <option>--link</option> and <option>--clone</option>.) + <option>--link</option>, <option>--clone</option>, + <option>--copy-file-range</option>, and <option>--swap</option>.) </para> </listitem> </varlistentry> @@ -262,6 +263,32 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>--swap</option></term> + <listitem> + <para> + Move the data directories from the old cluster to the new cluster. + Then, replace the catalog files with those generated for the new + cluster. This mode can outperform <option>--link</option>, + <option>--clone</option>, <option>--copy</option>, and + <option>--copy-file-range</option>, especially on clusters with many + relations. + </para> + <para> + However, this mode creates many garbage files in the old cluster, which + can prolong the file synchronization step if + <option>--sync-method=syncfs</option> is used. Therefore, it is + recommended to use <option>--sync-method=fsync</option> with + <option>--swap</option>. + </para> + <para> + Additionally, once the file transfer step begins, the old cluster will + be destructively modified and therefore will no longer be safe to + start. See <xref linkend="pgupgrade-step-revert"/> for details. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>--sync-method=</option><replaceable>method</replaceable></term> <listitem> @@ -530,6 +557,10 @@ NET STOP postgresql-&majorversion; is started. Clone mode also requires that the old and new data directories be in the same file system. This mode is only available on certain operating systems and file systems. + Swap mode may be the fastest if there are many relations, but you will not + be able to access your old cluster once the file transfer step begins. + Swap mode also requires that the old and new cluster data directories be + in the same file system. </para> <para> @@ -889,6 +920,32 @@ psql --username=postgres --file=script.sql postgres </itemizedlist></para> </listitem> + + <listitem> + <para> + If the <option>--swap</option> option was used, the old cluster might + be destructively modified: + + <itemizedlist> + <listitem> + <para> + If <command>pg_upgrade</command> aborts before reporting that the + old cluster is no longer safe to start, the old cluster was + unmodified; it can be restarted. + </para> + </listitem> + + <listitem> + <para> + If <command>pg_upgrade</command> has reported that the old cluster + is no longer safe to start, the old cluster was destructively + modified. The old cluster will need to be restored from backup in + this case. + </para> + </listitem> + </itemizedlist> + </para> + </listitem> </itemizedlist></para> </step> </procedure> diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 88db8869b6e..5405ed7bc8f 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -709,7 +709,34 @@ check_new_cluster(void) check_copy_file_range(); break; case TRANSFER_MODE_LINK: - check_hard_link(); + check_hard_link(TRANSFER_MODE_LINK); + break; + case TRANSFER_MODE_SWAP: + + /* + * We do the hard link check for --swap, too, since it's an easy + * way to verify the clusters are in the same file system. This + * allows us to take some shortcuts in the file synchronization + * step. With some more effort, we could probably support the + * separate-file-system use case, but this mode is unlikely to + * offer much benefit if we have to copy the files across file + * system boundaries. + */ + check_hard_link(TRANSFER_MODE_SWAP); + + /* + * There are a few known issues with using --swap to upgrade from + * versions older than 10. For example, the sequence tuple format + * changed in v10, and the visibility map format changed in 9.6. + * While such problems are not insurmountable (and we may have to + * deal with similar problems in the future, anyway), it doesn't + * seem worth the effort to support swap mode for upgrades from + * long-unsupported versions. + */ + if (GET_MAJOR_VERSION(old_cluster.major_version) < 1000) + pg_fatal("Swap mode can only upgrade clusters from PostgreSQL version %s and later.", + "10"); + break; } diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c index bd49ea867bf..391ed3e1085 100644 --- a/src/bin/pg_upgrade/controldata.c +++ b/src/bin/pg_upgrade/controldata.c @@ -751,11 +751,15 @@ check_control_data(ControlData *oldctrl, void -disable_old_cluster(void) +disable_old_cluster(transferMode transfer_mode) { char old_path[MAXPGPATH], new_path[MAXPGPATH]; + /* only used for --link and --swap */ + Assert(transfer_mode == TRANSFER_MODE_LINK || + transfer_mode == TRANSFER_MODE_SWAP); + /* rename pg_control so old server cannot be accidentally started */ prep_status("Adding \".old\" suffix to old global/pg_control"); @@ -766,10 +770,15 @@ disable_old_cluster(void) old_path, new_path); check_ok(); - pg_log(PG_REPORT, "\n" - "If you want to start the old cluster, you will need to remove\n" - "the \".old\" suffix from %s/global/pg_control.old.\n" - "Because \"link\" mode was used, the old cluster cannot be safely\n" - "started once the new cluster has been started.", - old_cluster.pgdata); + if (transfer_mode == TRANSFER_MODE_LINK) + pg_log(PG_REPORT, "\n" + "If you want to start the old cluster, you will need to remove\n" + "the \".old\" suffix from %s/global/pg_control.old.\n" + "Because \"link\" mode was used, the old cluster cannot be safely\n" + "started once the new cluster has been started.", + old_cluster.pgdata); + else + pg_log(PG_REPORT, "\n" + "Because \"swap\" mode was used, the old cluster can no longer be\n" + "safely started."); } diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index b8fd0d0acee..23cb08e8347 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -52,9 +52,11 @@ generate_old_dump(void) snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid); parallel_exec_prog(log_file_name, NULL, - "\"%s/pg_dump\" %s --no-data %s --sequence-data --quote-all-identifiers " + "\"%s/pg_dump\" %s --no-data %s %s --quote-all-identifiers " "--binary-upgrade --format=custom %s --no-sync --file=\"%s/%s\" %s", new_cluster.bindir, cluster_conn_opts(&old_cluster), + (user_opts.transfer_mode == TRANSFER_MODE_SWAP) ? + "" : "--sequence-data", log_opts.verbose ? "--verbose" : "", user_opts.do_statistics ? "" : "--no-statistics", log_opts.dumpdir, diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c index 7fd1991204a..4fe784e8b94 100644 --- a/src/bin/pg_upgrade/file.c +++ b/src/bin/pg_upgrade/file.c @@ -434,18 +434,28 @@ check_copy_file_range(void) } void -check_hard_link(void) +check_hard_link(transferMode transfer_mode) { char existing_file[MAXPGPATH]; char new_link_file[MAXPGPATH]; + /* only used for --link and --swap */ + Assert(transfer_mode == TRANSFER_MODE_LINK || + transfer_mode == TRANSFER_MODE_SWAP); + snprintf(existing_file, sizeof(existing_file), "%s/PG_VERSION", old_cluster.pgdata); snprintf(new_link_file, sizeof(new_link_file), "%s/PG_VERSION.linktest", new_cluster.pgdata); unlink(new_link_file); /* might fail */ if (link(existing_file, new_link_file) < 0) - pg_fatal("could not create hard link between old and new data directories: %m\n" - "In link mode the old and new data directories must be on the same file system."); + { + if (transfer_mode == TRANSFER_MODE_LINK) + pg_fatal("could not create hard link between old and new data directories: %m\n" + "In link mode the old and new data directories must be on the same file system."); + else + pg_fatal("could not create hard link between old and new data directories: %m\n" + "In swap mode the old and new data directories must be on the same file system."); + } unlink(new_link_file); } diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c index ad52de8b607..4b7a56f5b3b 100644 --- a/src/bin/pg_upgrade/info.c +++ b/src/bin/pg_upgrade/info.c @@ -490,7 +490,7 @@ get_rel_infos_query(void) " FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n " " ON c.relnamespace = n.oid " " WHERE relkind IN (" CppAsString2(RELKIND_RELATION) ", " - CppAsString2(RELKIND_MATVIEW) ") AND " + CppAsString2(RELKIND_MATVIEW) "%s) AND " /* exclude possible orphaned temp tables */ " ((n.nspname !~ '^pg_temp_' AND " " n.nspname !~ '^pg_toast_temp_' AND " @@ -499,6 +499,8 @@ get_rel_infos_query(void) " c.oid >= %u::pg_catalog.oid) OR " " (n.nspname = 'pg_catalog' AND " " relname IN ('pg_largeobject') ))), ", + (user_opts.transfer_mode == TRANSFER_MODE_SWAP) ? + ", " CppAsString2(RELKIND_SEQUENCE) : "", FirstNormalObjectId); /* diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index 188dd8d8a8b..7fd7f1d33fc 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -62,6 +62,7 @@ parseCommandLine(int argc, char *argv[]) {"sync-method", required_argument, NULL, 4}, {"no-statistics", no_argument, NULL, 5}, {"set-char-signedness", required_argument, NULL, 6}, + {"swap", no_argument, NULL, 7}, {NULL, 0, NULL, 0} }; @@ -228,6 +229,11 @@ parseCommandLine(int argc, char *argv[]) else pg_fatal("invalid argument for option %s", "--set-char-signedness"); break; + + case 7: + user_opts.transfer_mode = TRANSFER_MODE_SWAP; + break; + default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), os_info.progname); @@ -325,6 +331,7 @@ usage(void) printf(_(" --no-statistics do not import statistics from old cluster\n")); printf(_(" --set-char-signedness=OPTION set new cluster char signedness to \"signed\" or\n" " \"unsigned\"\n")); + printf(_(" --swap move data directories to new cluster\n")); printf(_(" --sync-method=METHOD set method for syncing files to disk\n")); printf(_(" -?, --help show this help, then exit\n")); printf(_("\n" diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index 174cd920840..9295e46aed3 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -170,12 +170,14 @@ main(int argc, char **argv) /* * Most failures happen in create_new_objects(), which has completed at - * this point. We do this here because it is just before linking, which - * will link the old and new cluster data files, preventing the old - * cluster from being safely started once the new cluster is started. + * this point. We do this here because it is just before file transfer, + * which for --link will make it unsafe to start the old cluster once the + * new cluster is started, and for --swap will make it unsafe to start the + * old cluster at all. */ - if (user_opts.transfer_mode == TRANSFER_MODE_LINK) - disable_old_cluster(); + if (user_opts.transfer_mode == TRANSFER_MODE_LINK || + user_opts.transfer_mode == TRANSFER_MODE_SWAP) + disable_old_cluster(user_opts.transfer_mode); transfer_all_new_tablespaces(&old_cluster.dbarr, &new_cluster.dbarr, old_cluster.pgdata, new_cluster.pgdata); @@ -212,8 +214,10 @@ main(int argc, char **argv) { prep_status("Sync data directory to disk"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/initdb\" --sync-only \"%s\" --sync-method %s", + "\"%s/initdb\" --sync-only %s \"%s\" --sync-method %s", new_cluster.bindir, + (user_opts.transfer_mode == TRANSFER_MODE_SWAP) ? + "--no-sync-data-files" : "", new_cluster.pgdata, user_opts.sync_method); check_ok(); diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index f4e375d27c7..120c38929d4 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -262,6 +262,7 @@ typedef enum TRANSFER_MODE_COPY, TRANSFER_MODE_COPY_FILE_RANGE, TRANSFER_MODE_LINK, + TRANSFER_MODE_SWAP, } transferMode; /* @@ -391,7 +392,7 @@ void create_script_for_old_cluster_deletion(char **deletion_script_file_name); void get_control_data(ClusterInfo *cluster); void check_control_data(ControlData *oldctrl, ControlData *newctrl); -void disable_old_cluster(void); +void disable_old_cluster(transferMode transfer_mode); /* dump.c */ @@ -423,7 +424,7 @@ void rewriteVisibilityMap(const char *fromfile, const char *tofile, const char *schemaName, const char *relName); void check_file_clone(void); void check_copy_file_range(void); -void check_hard_link(void); +void check_hard_link(transferMode transfer_mode); /* fopen_priv() is no longer different from fopen() */ #define fopen_priv(path, mode) fopen(path, mode) diff --git a/src/bin/pg_upgrade/relfilenumber.c b/src/bin/pg_upgrade/relfilenumber.c index 8c23c583172..2abe90dd239 100644 --- a/src/bin/pg_upgrade/relfilenumber.c +++ b/src/bin/pg_upgrade/relfilenumber.c @@ -11,11 +11,92 @@ #include <sys/stat.h> +#include "common/file_perm.h" +#include "common/file_utils.h" +#include "common/int.h" +#include "common/logging.h" #include "pg_upgrade.h" static void transfer_single_new_db(FileNameMap *maps, int size, char *old_tablespace); static void transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_frozenbit); +/* + * The following set of sync_queue_* functions are used for --swap to reduce + * the amount of time spent synchronizing the swapped catalog files. When a + * file is added to the queue, we also alert the file system that we'd like it + * to be persisted to disk in the near future (if that operation is supported + * by the current platform). Once the queue is full, all of the files are + * synchronized to disk. This strategy should generally be much faster than + * simply calling fsync() on the files right away. + * + * The general usage pattern should be something like: + * + * for (int i = 0; i < num_files; i++) + * sync_queue_push(files[i]); + * + * // be sure to sync any remaining files in the queue + * sync_queue_sync_all(); + * synq_queue_destroy(); + */ + +#define SYNC_QUEUE_MAX_LEN (1024) + +static char *sync_queue[SYNC_QUEUE_MAX_LEN]; +static bool sync_queue_inited; +static int sync_queue_len; + +static inline void +sync_queue_init(void) +{ + if (sync_queue_inited) + return; + + sync_queue_inited = true; + for (int i = 0; i < SYNC_QUEUE_MAX_LEN; i++) + sync_queue[i] = palloc(MAXPGPATH); +} + +static inline void +sync_queue_sync_all(void) +{ + if (!sync_queue_inited) + return; + + for (int i = 0; i < sync_queue_len; i++) + { + if (fsync_fname(sync_queue[i], false) != 0) + pg_fatal("could not synchronize file \"%s\": %m", sync_queue[i]); + } + + sync_queue_len = 0; +} + +static inline void +sync_queue_push(const char *fname) +{ + sync_queue_init(); + + pre_sync_fname(fname, false); + + strncpy(sync_queue[sync_queue_len++], fname, MAXPGPATH); + if (sync_queue_len >= SYNC_QUEUE_MAX_LEN) + sync_queue_sync_all(); +} + +static inline void +sync_queue_destroy(void) +{ + if (!sync_queue_inited) + return; + + sync_queue_inited = false; + sync_queue_len = 0; + for (int i = 0; i < SYNC_QUEUE_MAX_LEN; i++) + { + pfree(sync_queue[i]); + sync_queue[i] = NULL; + } +} /* * transfer_all_new_tablespaces() @@ -41,6 +122,9 @@ transfer_all_new_tablespaces(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, case TRANSFER_MODE_LINK: prep_status_progress("Linking user relation files"); break; + case TRANSFER_MODE_SWAP: + prep_status_progress("Swapping data directories"); + break; } /* @@ -125,6 +209,261 @@ transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, /* We allocate something even for n_maps == 0 */ pg_free(mappings); } + + /* + * Make sure anything pending synchronization in swap mode is fully + * persisted to disk. This is a no-op for other transfer modes. + */ + sync_queue_sync_all(); + sync_queue_destroy(); +} + +/* + * prepare_for_swap() + * + * This function durably moves the database directories from the old cluster to + * the new cluster in preparation for moving the pg_restore-generated catalog + * files into place. Returns false if the database with the given OID does not + * have a directory in the given tablespace, otherwise returns true. + */ +static bool +prepare_for_swap(const char *old_tablespace, Oid db_oid, + char *old_cat, char *new_dat, char *moved_dat) +{ + const char *new_tablespace; + const char *old_tblspc_suffix; + const char *new_tblspc_suffix; + char old_tblspc[MAXPGPATH]; + char new_tblspc[MAXPGPATH]; + char moved_tblspc[MAXPGPATH]; + char old_dat[MAXPGPATH]; + struct stat st; + + if (strcmp(old_tablespace, old_cluster.pgdata) == 0) + { + new_tablespace = new_cluster.pgdata; + new_tblspc_suffix = "/base"; + old_tblspc_suffix = "/base"; + } + else + { + new_tablespace = old_tablespace; + new_tblspc_suffix = new_cluster.tablespace_suffix; + old_tblspc_suffix = old_cluster.tablespace_suffix; + } + + snprintf(old_tblspc, sizeof(old_tblspc), "%s%s", old_tablespace, old_tblspc_suffix); + snprintf(moved_tblspc, sizeof(moved_tblspc), "%s_moved", old_tblspc); + snprintf(old_cat, MAXPGPATH, "%s/%u_old_cat", moved_tblspc, db_oid); + snprintf(new_tblspc, sizeof(new_tblspc), "%s%s", new_tablespace, new_tblspc_suffix); + snprintf(new_dat, MAXPGPATH, "%s/%u", new_tblspc, db_oid); + snprintf(moved_dat, MAXPGPATH, "%s/%u", moved_tblspc, db_oid); + snprintf(old_dat, sizeof(old_dat), "%s/%u", old_tblspc, db_oid); + + /* Check that the database directory exists in the given tablespace. */ + if (stat(old_dat, &st) != 0) + { + if (errno != ENOENT) + pg_fatal("could not stat file \"%s\": %m", old_dat); + return false; + } + + /* Create directory for stuff that is moved aside. */ + if (pg_mkdir_p(moved_tblspc, pg_dir_create_mode) != 0 && errno != EEXIST) + pg_fatal("could not create directory \"%s\"", moved_tblspc); + + /* Create directory for old catalog files. */ + if (pg_mkdir_p(old_cat, pg_dir_create_mode) != 0) + pg_fatal("could not create directory \"%s\"", old_cat); + + /* Move the new cluster's database directory aside. */ + if (rename(new_dat, moved_dat) != 0) + pg_fatal("could not rename \"%s\" to \"%s\"", new_dat, moved_dat); + + /* Move the old cluster's database directory into place. */ + if (rename(old_dat, new_dat) != 0) + pg_fatal("could not rename \"%s\" to \"%s\"", old_dat, new_dat); + + return true; +} + +/* + * FileNameMapCmp() + * + * qsort() comparator for FileNameMap that sorts by RelFileNumber. + */ +static int +FileNameMapCmp(const void *a, const void *b) +{ + const FileNameMap *map1 = (const FileNameMap *) a; + const FileNameMap *map2 = (const FileNameMap *) b; + + return pg_cmp_u32(map1->relfilenumber, map2->relfilenumber); +} + +/* + * parse_relfilenumber() + * + * Attempt to parse the RelFileNumber of the given file name. If we can't, + * return InvalidRelFileNumber. + */ +static RelFileNumber +parse_relfilenumber(const char *filename) +{ + char *endp; + unsigned long n; + + if (filename[0] < '1' || filename[0] > '9') + return InvalidRelFileNumber; + + errno = 0; + n = strtoul(filename, &endp, 10); + if (errno || filename == endp || n <= 0 || n > PG_UINT32_MAX) + return InvalidRelFileNumber; + + return (RelFileNumber) n; +} + +/* + * swap_catalog_files() + * + * Moves the old catalog files aside, and moves the new catalog files into + * place. + */ +static void +swap_catalog_files(FileNameMap *maps, int size, const char *old_cat, + const char *new_dat, const char *moved_dat) +{ + DIR *dir; + struct dirent *de; + char path[MAXPGPATH]; + char dest[MAXPGPATH]; + RelFileNumber rfn; + + /* Move the old catalog files aside. */ + dir = opendir(new_dat); + if (dir == NULL) + pg_fatal("could not open directory \"%s\": %m", new_dat); + while (errno = 0, (de = readdir(dir)) != NULL) + { + if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0) + continue; + + snprintf(path, sizeof(path), "%s/%s", new_dat, de->d_name); + if (get_dirent_type(path, de, false, PG_LOG_ERROR) != PGFILETYPE_REG) + continue; + + rfn = parse_relfilenumber(de->d_name); + if (RelFileNumberIsValid(rfn)) + { + FileNameMap key = {.relfilenumber = rfn}; + + if (bsearch(&key, maps, size, sizeof(FileNameMap), FileNameMapCmp)) + continue; + } + + snprintf(dest, sizeof(dest), "%s/%s", old_cat, de->d_name); + if (rename(path, dest) != 0) + pg_fatal("could not rename \"%s\" to \"%s\": %m", path, dest); + } + + if (errno) + pg_fatal("could not read directory \"%s\": %m", new_dat); + (void) closedir(dir); + + /* Move the new catalog files into place. */ + dir = opendir(moved_dat); + if (dir == NULL) + pg_fatal("could not open directory \"%s\": %m", moved_dat); + while (errno = 0, (de = readdir(dir)) != NULL) + { + if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0) + continue; + + snprintf(path, sizeof(path), "%s/%s", moved_dat, de->d_name); + if (get_dirent_type(path, de, false, PG_LOG_ERROR) != PGFILETYPE_REG) + continue; + + rfn = parse_relfilenumber(de->d_name); + if (RelFileNumberIsValid(rfn)) + { + FileNameMap key = {.relfilenumber = rfn}; + + if (bsearch(&key, maps, size, sizeof(FileNameMap), FileNameMapCmp)) + continue; + } + + snprintf(dest, sizeof(dest), "%s/%s", new_dat, de->d_name); + if (rename(path, dest) != 0) + pg_fatal("could not rename \"%s\" to \"%s\": %m", path, dest); + + /* + * We don't fsync() the database files in the file synchronization + * stage of pg_upgrade in swap mode, so we need to synchronize them + * ourselves. We only do this for the catalog files because they were + * created during pg_restore with fsync=off. We assume that the user + * data files files were properly persisted to disk when the user last + * shut it down. + */ + sync_queue_push(dest); + } + + if (errno) + pg_fatal("could not read directory \"%s\": %m", moved_dat); + (void) closedir(dir); + + /* Ensure the directory entries are persisted to disk. */ + if (fsync_fname(new_dat, true) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", new_dat); + if (fsync_parent_path(new_dat) != 0) + pg_fatal("could not synchronize parent directory of \"%s\": %m", new_dat); +} + +/* + * do_swap() + * + * Perform the required steps for --swap for a single database. In short this + * moves the old cluster's database directory into the new cluster and then + * replaces any files for system catalogs with the ones that were generated + * during pg_restore. + */ +static void +do_swap(FileNameMap *maps, int size, char *old_tablespace) +{ + char old_cat[MAXPGPATH]; + char new_dat[MAXPGPATH]; + char moved_dat[MAXPGPATH]; + + /* + * We perform many lookups on maps by relfilenumber in swap mode, so make + * sure it's sorted. + */ + qsort(maps, size, sizeof(FileNameMap), FileNameMapCmp); + + /* + * If an old tablespace is given, we only need to process that one. If no + * old tablespace is specified, we need to process all the tablespaces on + * the system. + */ + if (old_tablespace) + { + if (prepare_for_swap(old_tablespace, maps[0].db_oid, + old_cat, new_dat, moved_dat)) + swap_catalog_files(maps, size, old_cat, new_dat, moved_dat); + } + else + { + if (prepare_for_swap(old_cluster.pgdata, maps[0].db_oid, + old_cat, new_dat, moved_dat)) + swap_catalog_files(maps, size, old_cat, new_dat, moved_dat); + + for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++) + { + if (prepare_for_swap(os_info.old_tablespaces[tblnum], maps[0].db_oid, + old_cat, new_dat, moved_dat)) + swap_catalog_files(maps, size, old_cat, new_dat, moved_dat); + } + } } /* @@ -145,6 +484,20 @@ transfer_single_new_db(FileNameMap *maps, int size, char *old_tablespace) new_cluster.controldata.cat_ver >= VISIBILITY_MAP_FROZEN_BIT_CAT_VER) vm_must_add_frozenbit = true; + /* --swap has its own subroutine */ + if (user_opts.transfer_mode == TRANSFER_MODE_SWAP) + { + /* + * We don't support --swap to upgrade from versions that require + * rewriting the visibility map. We should've failed already if + * someone tries to do that. + */ + Assert(!vm_must_add_frozenbit); + + do_swap(maps, size, old_tablespace); + return; + } + for (mapnum = 0; mapnum < size; mapnum++) { if (old_tablespace == NULL || @@ -259,6 +612,11 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro pg_log(PG_VERBOSE, "linking \"%s\" to \"%s\"", old_file, new_file); linkFile(old_file, new_file, map->nspname, map->relname); + break; + case TRANSFER_MODE_SWAP: + /* swap mode is handled in its own code path */ + pg_fatal("should never happen"); + break; } } } diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 78e272916f5..4405ef8b425 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -45,9 +45,6 @@ */ #define MINIMUM_VERSION_FOR_PG_WAL 100000 -#ifdef PG_FLUSH_DATA_WORKS -static int pre_sync_fname(const char *fname, bool isdir); -#endif static void walkdir(const char *path, int (*action) (const char *fname, bool isdir), bool process_symlinks, @@ -352,16 +349,16 @@ walkdir(const char *path, } /* - * Hint to the OS that it should get ready to fsync() this file. + * Hint to the OS that it should get ready to fsync() this file, if supported + * by the platform. * * Ignores errors trying to open unreadable files, and reports other errors * non-fatally. */ -#ifdef PG_FLUSH_DATA_WORKS - -static int +int pre_sync_fname(const char *fname, bool isdir) { +#ifdef PG_FLUSH_DATA_WORKS int fd; fd = open(fname, O_RDONLY | PG_BINARY, 0); @@ -388,11 +385,10 @@ pre_sync_fname(const char *fname, bool isdir) #endif (void) close(fd); +#endif /* PG_FLUSH_DATA_WORKS */ return 0; } -#endif /* PG_FLUSH_DATA_WORKS */ - /* * fsync_fname -- Try to fsync a file or directory * diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index 8274bc877ab..9fd88953e43 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -33,6 +33,7 @@ typedef enum DataDirSyncMethod struct iovec; /* avoid including port/pg_iovec.h here */ #ifdef FRONTEND +extern int pre_sync_fname(const char *fname, bool isdir); extern int fsync_fname(const char *fname, bool isdir); extern void sync_pgdata(const char *pg_data, int serverVersion, DataDirSyncMethod sync_method, bool sync_data_files); -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-17 19:27 Bruce Momjian <[email protected]> parent: Greg Sabino Mullane <[email protected]> 1 sibling, 0 replies; 38+ messages in thread From: Bruce Momjian @ 2025-03-17 19:27 UTC (permalink / raw) To: Greg Sabino Mullane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On Wed, Mar 5, 2025 at 03:40:52PM -0500, Greg Sabino Mullane wrote: > On Wed, Mar 5, 2025 at 2:43 PM Nathan Bossart <[email protected]> wrote: > > One other design point I wanted to bring up is whether we should bother > generating a rollback script for the new "swap" mode. In short, I'm > wondering if it would be unreasonable to say that, just for this mode, once > pg_upgrade enters the file transfer step, reverting to the old cluster > requires restoring a backup. > > > I think that's a fair requirement. And like Robert, revert scripts make me > nervous. > > > * Anecdotally, I'm not sure I've ever actually seen pg_upgrade fail > during or after file transfer, and I'm hoping to get some real data about > that in the near future. Has anyone else dealt with such a failure? > > > I've seen various failures, but they always get caught quite early. Certainly > early enough to easily abort, fix perms/mounts/etc., then retry. I think your > instinct is correct that this reversion is more trouble than its worth. I don't > think the pg_upgrade docs mention taking a backup, but that's always step 0 in > my playbook, and that's the rollback plan in the unlikely event of failure. I avoided many optimizations in pg_upgrade in the fear they would lead to hard-to-detect bugs, or breakage from major release changes. pg_upgrade is probably old enough now (15 years) that we can risk these optimizations. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future. ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-17 19:34 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-17 19:34 UTC (permalink / raw) To: Greg Sabino Mullane <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers; [email protected] On Wed, Mar 05, 2025 at 08:34:37PM -0600, Nathan Bossart wrote: > Thank you, Greg and Robert, for sharing your thoughts. With that, here's > what I'm considering to be a reasonably complete patch set for this > feature. This leaves about a month for rigorous testing and editing, so > I'm hopeful it'll be ready v18. Here are my notes after a round of self-review. 0001: * The documentation does not adequately describe the interaction between --no-sync-data-files and --sync-method=syncfs. * I really don't like the exclude_dir hack for skipping the main tablespace directory, but I haven't thought of anything that seems better. * I should verify that there's no path separator issues on Windows for the exclude_dir hack. From some quick code analysis, I think it should work fine, but I probably ought to test it out to be sure. * The documentation needs to mention that the tablespace directories themselves are not synchronized. 0002: * The documentation changes are subject to update based on ongoing stats import/export work. * Does --statistics-only --sequence-data make any sense? It seems like it ought to function as expected, but it's hard to see a use-case. 0003: * Once committed, I should update one of my buildfarm animals to use PG_TEST_PG_UPGRADE_MODE=--swap. * For check_hard_link() and disable_old_cluster(), move the Assert() to an "else" block with a pg_fatal() call for sturdiness. * I need to do a thorough pass-through on all comments. Many are not sufficiently detailed. * The "." and ".." checks in the catalog swap code are redundant and can be removed. * The directory for "moved-aside" stuff should be placed within the old cluster's corresponding tablespace directory so that no changes need to be made to delete_old_cluster.{sh,bat}. * Manual testing with non-default tablespaces! Updated patches based on these notes are attached. -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-17 20:04 Robert Haas <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Robert Haas @ 2025-03-17 20:04 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Mon, Mar 17, 2025 at 3:34 PM Nathan Bossart <[email protected]> wrote: > * Once committed, I should update one of my buildfarm animals to use > PG_TEST_PG_UPGRADE_MODE=--swap. It would be better if we didn't need a separate buildfarm animal to test this, because that means you won't get notified by local testing OR by CI if you break this. Can we instead have one test that checks this which is part of the normal test run? -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-17 20:30 Nathan Bossart <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-17 20:30 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Mon, Mar 17, 2025 at 04:04:45PM -0400, Robert Haas wrote: > On Mon, Mar 17, 2025 at 3:34 PM Nathan Bossart <[email protected]> wrote: >> * Once committed, I should update one of my buildfarm animals to use >> PG_TEST_PG_UPGRADE_MODE=--swap. > > It would be better if we didn't need a separate buildfarm animal to > test this, because that means you won't get notified by local testing > OR by CI if you break this. Can we instead have one test that checks > this which is part of the normal test run? That's what I set out to do before I discovered PG_TEST_PG_UPGRADE_MODE. The commit message for b059a24 seemed to indicate that we don't want to automatically test all supported modes, but I agree that it would be nice to have some basic coverage for --swap on CI/buildfarm regardless of PG_TEST_PG_UPGRADE_MODE. How about we add a simple TAP test (attached), and I still plan on switching a buildfarm animal to --swap for more in-depth testing? -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-18 13:40 Robert Haas <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 2 replies; 38+ messages in thread From: Robert Haas @ 2025-03-18 13:40 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Mon, Mar 17, 2025 at 4:30 PM Nathan Bossart <[email protected]> wrote: > On Mon, Mar 17, 2025 at 04:04:45PM -0400, Robert Haas wrote: > > On Mon, Mar 17, 2025 at 3:34 PM Nathan Bossart <[email protected]> wrote: > >> * Once committed, I should update one of my buildfarm animals to use > >> PG_TEST_PG_UPGRADE_MODE=--swap. > > > > It would be better if we didn't need a separate buildfarm animal to > > test this, because that means you won't get notified by local testing > > OR by CI if you break this. Can we instead have one test that checks > > this which is part of the normal test run? > > That's what I set out to do before I discovered PG_TEST_PG_UPGRADE_MODE. > The commit message for b059a24 seemed to indicate that we don't want to > automatically test all supported modes, but I agree that it would be nice > to have some basic coverage for --swap on CI/buildfarm regardless of > PG_TEST_PG_UPGRADE_MODE. How about we add a simple TAP test (attached), > and I still plan on switching a buildfarm animal to --swap for more > in-depth testing? The background here is that I'm kind of on the warpath against weird configurations that we only test on certain buildfarm animals at the moment, because the result of that is that CI is clean and then the buildfarm turns red when you commit. That's an unenjoyable experience for the committer and for everyone who looks at the buildfarm results. The way to fix it is to stop relying on "rerun all the tests with this weird mode flag" and rely more on tests that are designed to test that specific flag and, ideally, that get run by in local testing or at least by CI. I'm not quite sure what the best thing is to do is for the pg_upgrade tests in particular, and it may well be best to do as you propose for now and figure that out later. But I question whether just rerunning all of those tests with several different mode flags is the right thing to do. Why for example does 005_char_signedness.pl need to be checked under both --link and --clone? I would guess that there are one or maybe two tests in src/bin/pg_upgrade/t that needs to test --link and --clone and they should grow internal loops to do that (when supported by the local platform) and PG_UPGRADE_TEST_MODE should go in the garbage. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-18 14:04 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 38+ messages in thread From: Tom Lane @ 2025-03-18 14:04 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] Robert Haas <[email protected]> writes: > I'm not quite sure what the best thing is to do is for the pg_upgrade > tests in particular, and it may well be best to do as you propose for > now and figure that out later. But I question whether just rerunning > all of those tests with several different mode flags is the right > thing to do. Why for example does 005_char_signedness.pl need to be > checked under both --link and --clone? I would guess that there are > one or maybe two tests in src/bin/pg_upgrade/t that needs to test > --link and --clone and they should grow internal loops to do that > (when supported by the local platform) and PG_UPGRADE_TEST_MODE should > go in the garbage. +1 I'd be particularly allergic to running 002_pg_upgrade.pl multiple times, as that's one of our most expensive tests, and I flat out don't believe that expending that many cycles could be justified. Surely we can test these modes sufficiently in some much cheaper and more targeted way. regards, tom lane ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-18 14:12 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Andres Freund @ 2025-03-18 14:12 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Nathan Bossart <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] Hi, On 2025-03-18 10:04:41 -0400, Tom Lane wrote: > Robert Haas <[email protected]> writes: > > I'm not quite sure what the best thing is to do is for the pg_upgrade > > tests in particular, and it may well be best to do as you propose for > > now and figure that out later. But I question whether just rerunning > > all of those tests with several different mode flags is the right > > thing to do. Why for example does 005_char_signedness.pl need to be > > checked under both --link and --clone? I would guess that there are > > one or maybe two tests in src/bin/pg_upgrade/t that needs to test > > --link and --clone and they should grow internal loops to do that > > (when supported by the local platform) and PG_UPGRADE_TEST_MODE should > > go in the garbage. > > +1 > > I'd be particularly allergic to running 002_pg_upgrade.pl multiple > times, as that's one of our most expensive tests, and I flat out > don't believe that expending that many cycles could be justified. > Surely we can test these modes sufficiently in some much cheaper and > more targeted way. +1 It's useful to have coverage of as many object types as possible in pg_upgrade - hence 002_pg_upgrade.pl. It helps us to find problems in new code that didn't think about pg_upgrade. But that doesn't mean that it's a good idea to run all other pg_upgrade tests the same way, to the contrary - the cost is too high. Even leaving runtime aside, I have a hard time believing that --link, --clone, --swap benefit from running the same way as 002_pg_upgrade.pl - the implementation of those flags is on a lower level and works the same across e.g. different index AMs. I'd go so far as to say that 002_pg_upgrade.pl style testing actually makes it *harder* to diagnose problems related to things like --link, because there are no targeted tests, but just a huge set of things that maybe allow to infer some bug if you spend a lot of time. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-18 15:51 Álvaro Herrera <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 38+ messages in thread From: Álvaro Herrera @ 2025-03-18 15:51 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On 2025-Mar-18, Robert Haas wrote: > The background here is that I'm kind of on the warpath against weird > configurations that we only test on certain buildfarm animals at the > moment, because the result of that is that CI is clean and then the > buildfarm turns red when you commit. That's an unenjoyable experience > for the committer and for everyone who looks at the buildfarm results. > The way to fix it is to stop relying on "rerun all the tests with this > weird mode flag" and rely more on tests that are designed to test that > specific flag and, ideally, that get run by in local testing or at > least by CI. FWIW this is exactly the rationale that got me writing an email on the Ashutosh's thread for a new pg_dump/restore test under 002_pg_upgrade.pl, whereby I was saying that we should not hide it behind PG_TEST_EXTRA which almost nobody would remember to use. But I discarded that draft, because that had actually been Ashutosh's idea at some point in the thread and had been discarded because of the runtime increase it'd cause. But, somehow, I still don't believe the theory that it's such a bad idea to add a few seconds so that we have such a comprehensive pg_dump test, with much less programmer overhead than pg_dump's own weird enormous test script. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Cada quien es cada cual y baja las escaleras como quiere" (JMSerrat) ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-18 17:29 Nathan Bossart <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-18 17:29 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Tue, Mar 18, 2025 at 10:12:51AM -0400, Andres Freund wrote: > On 2025-03-18 10:04:41 -0400, Tom Lane wrote: >> Robert Haas <[email protected]> writes: >> > I'm not quite sure what the best thing is to do is for the pg_upgrade >> > tests in particular, and it may well be best to do as you propose for >> > now and figure that out later. But I question whether just rerunning >> > all of those tests with several different mode flags is the right >> > thing to do. Why for example does 005_char_signedness.pl need to be >> > checked under both --link and --clone? I would guess that there are >> > one or maybe two tests in src/bin/pg_upgrade/t that needs to test >> > --link and --clone and they should grow internal loops to do that >> > (when supported by the local platform) and PG_UPGRADE_TEST_MODE should >> > go in the garbage. >> >> +1 >> >> I'd be particularly allergic to running 002_pg_upgrade.pl multiple >> times, as that's one of our most expensive tests, and I flat out >> don't believe that expending that many cycles could be justified. >> Surely we can test these modes sufficiently in some much cheaper and >> more targeted way. > > +1 Here is a first sketch at a test that cycles through all the transfer modes and makes sure they succeed or fail with an error along the lines of "not supported on this platform." Each test verifies that some very simple objects make it to the new version, which we could of course expand on. Would something like this suffice? -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-18 17:37 Andres Freund <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Andres Freund @ 2025-03-18 17:37 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] Hi, On 2025-03-18 12:29:02 -0500, Nathan Bossart wrote: > Here is a first sketch at a test that cycles through all the transfer modes > and makes sure they succeed or fail with an error along the lines of "not > supported on this platform." Each test verifies that some very simple > objects make it to the new version, which we could of course expand on. > Would something like this suffice? I'd add a few more complications: - Create and test a relation that was rewritten, to ensure we test the relfilenode != oid case and one that isn't rewritten. - Perhaps create a tablespace? - Do we need a new old cluster for each of the modes? That seems like wasted time? I guess it's required for --link... Greetings, Andres Freund ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-18 17:47 Nathan Bossart <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-18 17:47 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Tue, Mar 18, 2025 at 01:37:02PM -0400, Andres Freund wrote: > I'd add a few more complications: > > - Create and test a relation that was rewritten, to ensure we test the > relfilenode != oid case and one that isn't rewritten. +1 > - Perhaps create a tablespace? +1, I don't think we have much, if any, coverage of pg_upgrade with non-default tablespaces. > - Do we need a new old cluster for each of the modes? That seems like wasted > time? I guess it's required for --link... It'll also be needed for --swap. We could optionally save the old cluster for a couple of modes if we really wanted to. *shrug* I'll work on the first two... -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-18 17:50 Andres Freund <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Andres Freund @ 2025-03-18 17:50 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On 2025-03-18 12:47:01 -0500, Nathan Bossart wrote: > On Tue, Mar 18, 2025 at 01:37:02PM -0400, Andres Freund wrote: > > - Do we need a new old cluster for each of the modes? That seems like wasted > > time? I guess it's required for --link... > > It'll also be needed for --swap. We could optionally save the old cluster > for a couple of modes if we really wanted to. *shrug* Don't worry about it, I think the template initdb stuff should make it cheap enough... ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-18 19:08 Nathan Bossart <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-18 19:08 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Tue, Mar 18, 2025 at 01:50:10PM -0400, Andres Freund wrote: > On 2025-03-18 12:47:01 -0500, Nathan Bossart wrote: >> On Tue, Mar 18, 2025 at 01:37:02PM -0400, Andres Freund wrote: >> > - Do we need a new old cluster for each of the modes? That seems like wasted >> > time? I guess it's required for --link... >> >> It'll also be needed for --swap. We could optionally save the old cluster >> for a couple of modes if we really wanted to. *shrug* > > Don't worry about it, I think the template initdb stuff should make it cheap enough... Cool. I realize now why there's poor coverage for pg_upgrade with tablespaces: you can't upgrade between the same version with tablespaces (presumably due to the version-specific subdirectory conflict). I don't know if the regression tests leave around any tablespaces for the cross-version pg_upgrade tests, but that's probably the best we can do at the moment. For now, here's a new version of the test with a rewritten table. I also tried to fix the expected error regex to handle some of the other error messages for unsupported modes (as revealed by cfbot). -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-19 02:14 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 2 replies; 38+ messages in thread From: Nathan Bossart @ 2025-03-19 02:14 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Tue, Mar 18, 2025 at 02:08:42PM -0500, Nathan Bossart wrote: > For now, here's a new version of the test with a rewritten table. I also > tried to fix the expected error regex to handle some of the other error > messages for unsupported modes (as revealed by cfbot). And here is a new version of the full patch set. -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-19 15:31 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-19 15:31 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Tue, Mar 18, 2025 at 09:14:22PM -0500, Nathan Bossart wrote: > And here is a new version of the full patch set. I'm currently planning to commit this sometime early-ish next week. One notable loose end is the lack of a pg_upgrade test with a non-default tablespace, but that is an existing problem that IMHO is best handled separately (since we can only test it in cross-version upgrades). -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-19 15:41 Andres Freund <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 0 replies; 38+ messages in thread From: Andres Freund @ 2025-03-19 15:41 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] Hi, On 2025-03-18 21:14:22 -0500, Nathan Bossart wrote: > From 8b6a5e0148c2f7a663f5003f12ae9461d2b06a5c Mon Sep 17 00:00:00 2001 > From: Nathan Bossart <[email protected]> > Date: Tue, 18 Mar 2025 20:58:07 -0500 > Subject: [PATCH v7 1/4] Add test for pg_upgrade file transfer modes. > > This new test checks all of pg_upgrade's file transfer modes. For > each mode, we verify that pg_upgrade either succeeds (and some test > objects successfully reach the new version) or fails with an error > that indicates the mode is not supported on the current platform. LGTM. I'm sure we could do more than the test does today, but I think it's a good improvement. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-19 16:28 Tom Lane <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Tom Lane @ 2025-03-19 16:28 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] Nathan Bossart <[email protected]> writes: > I'm currently planning to commit this sometime early-ish next week. One > notable loose end is the lack of a pg_upgrade test with a non-default > tablespace, but that is an existing problem that IMHO is best handled > separately (since we can only test it in cross-version upgrades). Agreed that that shouldn't block this, but we need some kind of plan for testing it better. regards, tom lane ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-19 16:44 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Andres Freund @ 2025-03-19 16:44 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] Hi, On 2025-03-19 12:28:33 -0400, Tom Lane wrote: > Nathan Bossart <[email protected]> writes: > > I'm currently planning to commit this sometime early-ish next week. One > > notable loose end is the lack of a pg_upgrade test with a non-default > > tablespace, but that is an existing problem that IMHO is best handled > > separately (since we can only test it in cross-version upgrades). > > Agreed that that shouldn't block this, but we need some kind of > plan for testing it better. Yea, this is really suboptimal. Shouldn't allow_in_place_tablespaces be sufficient to deal with that scenario? Or at least it should make it reasonably easy to cope if it doesn't already suffice? Greetings, Andres Freund ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-19 19:32 Nathan Bossart <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-19 19:32 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Wed, Mar 19, 2025 at 12:44:38PM -0400, Andres Freund wrote: > On 2025-03-19 12:28:33 -0400, Tom Lane wrote: >> Nathan Bossart <[email protected]> writes: >> > I'm currently planning to commit this sometime early-ish next week. One >> > notable loose end is the lack of a pg_upgrade test with a non-default >> > tablespace, but that is an existing problem that IMHO is best handled >> > separately (since we can only test it in cross-version upgrades). >> >> Agreed that that shouldn't block this, but we need some kind of >> plan for testing it better. > > Yea, this is really suboptimal. > > Shouldn't allow_in_place_tablespaces be sufficient to deal with that scenario? > Or at least it should make it reasonably easy to cope if it doesn't already > suffice? Unfortunately, pg_upgrade can't yet handle in-place tablespaces. One reason is that pg_tablespace_location() returns a relative path for those (e.g., "pg_tblspc/123456"). We'd also need to adjust init_tablespaces() to not fail if all the tablespaces are in-place. There may be other reasons, too. I'm confident we could get it working, but I'm not too excited about trying to sneak this into v18. In addition to testing with in-place tablespaces, we might also want to teach the transfer modes test to do cross-version testing when possible. In that case, we can test normal (non-in-place) tablespaces. However, that would be limited to the buildfarm. Does this seem like a reasonable plan for v19? -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-19 21:28 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-19 21:28 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Wed, Mar 19, 2025 at 02:32:01PM -0500, Nathan Bossart wrote: > In addition to testing with in-place tablespaces, we might also want to > teach the transfer modes test to do cross-version testing when possible. > In that case, we can test normal (non-in-place) tablespaces. However, that > would be limited to the buildfarm. Actually, this one was pretty easy to do. -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-20 02:02 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-20 02:02 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Wed, Mar 19, 2025 at 04:28:23PM -0500, Nathan Bossart wrote: > On Wed, Mar 19, 2025 at 02:32:01PM -0500, Nathan Bossart wrote: >> In addition to testing with in-place tablespaces, we might also want to >> teach the transfer modes test to do cross-version testing when possible. >> In that case, we can test normal (non-in-place) tablespaces. However, that >> would be limited to the buildfarm. > > Actually, this one was pretty easy to do. And here is yet another new version of the full patch set. I'm planning to commit 0001 (the new pg_upgrade transfer mode test) tomorrow so that I can deal with any buildfarm indigestion before committing swap mode. I did run the test locally for upgrades from v9.6, v13, and v17, but who knows what unique configurations I've failed to anticipate... -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-20 16:11 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-20 16:11 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Wed, Mar 19, 2025 at 09:02:42PM -0500, Nathan Bossart wrote: > On Wed, Mar 19, 2025 at 04:28:23PM -0500, Nathan Bossart wrote: >> On Wed, Mar 19, 2025 at 02:32:01PM -0500, Nathan Bossart wrote: >>> In addition to testing with in-place tablespaces, we might also want to >>> teach the transfer modes test to do cross-version testing when possible. >>> In that case, we can test normal (non-in-place) tablespaces. However, that >>> would be limited to the buildfarm. >> >> Actually, this one was pretty easy to do. > > And here is yet another new version of the full patch set. I'm planning to > commit 0001 (the new pg_upgrade transfer mode test) tomorrow so that I can > deal with any buildfarm indigestion before committing swap mode. I did run > the test locally for upgrades from v9.6, v13, and v17, but who knows what > unique configurations I've failed to anticipate... As promised, I've committed just 0001 for now. I'll watch closely for any issues in the buildfarm. -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-20 20:23 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 38+ messages in thread From: Nathan Bossart @ 2025-03-20 20:23 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Thu, Mar 20, 2025 at 11:11:46AM -0500, Nathan Bossart wrote: > As promised, I've committed just 0001 for now. I'll watch closely for any > issues in the buildfarm. Seeing none, here's is a rebased patch set without 0001. The only changes are some fleshed-out comments and commit messages. I'm still aiming to commit this sometime early next week. -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2025-03-25 21:03 Nathan Bossart <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 38+ messages in thread From: Nathan Bossart @ 2025-03-25 21:03 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Greg Sabino Mullane <[email protected]>; pgsql-hackers; [email protected] On Thu, Mar 20, 2025 at 03:23:13PM -0500, Nathan Bossart wrote: > I'm still aiming to commit this sometime early next week. Committed. Thanks to everyone who chimed in on this thread. While writing the attributions, I noticed that nobody seems to have commented specifically on 0001. The closest thing to a review I see is Greg's note upthread [0]. This patch is a little bigger than what I'd ordinarily feel comfortable with committing unilaterally, but it's been posted in its current form since February 28th, this thread has gotten a decent amount of traffic since then, and it's not a huge change ("9 files changed, 96 insertions(+), 37 deletions(-)"). I'm happy to address any post-commit feedback that folks have. As noted earlier [1], I'm not wild about how it's implemented, but this is the nicest approach I've thought of thus far. I also wanted to draw attention to this note in 0003: /* * XXX: The below line is a hack to deal with the fact that we * presently don't have an easy way to find the corresponding new * tablespace's path. This will need to be fixed if/when we add * pg_upgrade support for in-place tablespaces. */ new_tablespace = old_tablespace; I intend to address this in v19, primarily to enable same-version pg_upgrade testing with non-default tablespaces. My current thinking is that we should have pg_upgrade also gather the new cluster tablespace information and map them to the corresponding tablespaces on the old cluster. This might require some refactoring in pg_upgrade. In any case, I didn't feel this should block the feature for v18. [0] https://postgr.es/m/CAKAnmm%2Bi3Q1pZ05N_b8%3DS3B%3DrztQDn--HoW8BRKVtCg53r8NiQ%40mail.gmail.com [1] https://postgr.es/m/Z9h5Spp76EBygyEL%40nathan -- nathan ^ permalink raw reply [nested|flat] 38+ messages in thread
end of thread, other threads:[~2025-03-25 21:03 UTC | newest] Thread overview: 38+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2018-10-30 07:56 [PATCH v1 1/2] Allow IndexScanDesc allocation in caller-specified context amit <[email protected]> 2022-09-26 21:05 [PATCH v17 08/23] autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C Andres Freund <[email protected]> 2022-10-05 19:24 [PATCH v4 1/4] autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C Andres Freund <[email protected]> 2022-10-05 19:24 [PATCH v2 1/5] autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C Andres Freund <[email protected]> 2022-10-05 19:24 [PATCH v3 1/4] autoconf: Unify CFLAGS_SSE42 and CFLAGS_ARMV8_CRC32C Andres Freund <[email protected]> 2025-02-28 19:40 Re: optimize file transfer in pg_upgrade Robert Haas <[email protected]> 2025-02-28 19:41 ` Re: optimize file transfer in pg_upgrade Robert Haas <[email protected]> 2025-02-28 20:01 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-02-28 20:37 ` Re: optimize file transfer in pg_upgrade Robert Haas <[email protected]> 2025-02-28 20:51 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-05 19:42 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-05 20:12 ` Re: optimize file transfer in pg_upgrade Robert Haas <[email protected]> 2025-03-05 20:40 ` Re: optimize file transfer in pg_upgrade Greg Sabino Mullane <[email protected]> 2025-03-06 02:34 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-17 19:34 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-17 20:04 ` Re: optimize file transfer in pg_upgrade Robert Haas <[email protected]> 2025-03-17 20:30 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-18 13:40 ` Re: optimize file transfer in pg_upgrade Robert Haas <[email protected]> 2025-03-18 14:04 ` Re: optimize file transfer in pg_upgrade Tom Lane <[email protected]> 2025-03-18 14:12 ` Re: optimize file transfer in pg_upgrade Andres Freund <[email protected]> 2025-03-18 17:29 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-18 17:37 ` Re: optimize file transfer in pg_upgrade Andres Freund <[email protected]> 2025-03-18 17:47 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-18 17:50 ` Re: optimize file transfer in pg_upgrade Andres Freund <[email protected]> 2025-03-18 19:08 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-19 02:14 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-19 15:31 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-19 16:28 ` Re: optimize file transfer in pg_upgrade Tom Lane <[email protected]> 2025-03-19 16:44 ` Re: optimize file transfer in pg_upgrade Andres Freund <[email protected]> 2025-03-19 19:32 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-19 21:28 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-20 02:02 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-20 16:11 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-20 20:23 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-25 21:03 ` Re: optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2025-03-19 15:41 ` Re: optimize file transfer in pg_upgrade Andres Freund <[email protected]> 2025-03-18 15:51 ` Re: optimize file transfer in pg_upgrade Álvaro Herrera <[email protected]> 2025-03-17 19:27 ` Re: optimize file transfer in pg_upgrade Bruce Momjian <[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