On Thu, Jul 21, 2022 at 1:34 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > How is it sane to ask for a segment bin for zero pages? Seems like > something should have short-circuited such a case well before here.
It's intended. There are two ways you can arrive here with n == 0: * There's a special case in execParallel.c that creates a DSA segment "in-place" with initial size dsa_minimum_size(). That's because we don't know yet if we have any executor nodes that need a DSA segment (Parallel Hash, Parallel Bitmap Heap Scan), so we create one with the minimum amount of space other than the DSA control meta-data, so you get an in-place segment 0 with 0 usable pages. As soon as someone tries to allocate one byte, the first external DSM segment will be created. * A full segment can be re-binned into slot 0. On Thu, Jul 21, 2022 at 1:48 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Seems like passing a size_t to pg_leftmost_one_pos32 isn't great. > It was just as wrong before (if the caller-supplied argument is > indeed a size_t), but no time like the present to fix it. > > We could have pg_bitutils.h #define pg_leftmost_one_pos_size_t > as the appropriate one of pg_leftmost_one_pos32/64, perhaps. Yeah.
From e2b8448932ec7606c61dd847c3370ae9c31d23e9 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Thu, 21 Jul 2022 18:07:45 +1200 Subject: [PATCH v3 1/2] Extend size_t support in pg_bitutils.h. Use a more compact notation that allows us to add more size_t variants as required. This will be used by a later commit. Reviewed-by: Tom Lane <t...@sss.pgh.pa.us> Discussion: https://postgr.es/m/CA%2BhUKG%2B7dSX1XF8yFGmYk-%3D48dbjH2kmzZj16XvhbrWP-9BzRg%40mail.gmail.com --- src/include/port/pg_bitutils.h | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/src/include/port/pg_bitutils.h b/src/include/port/pg_bitutils.h index 04e58cd1c4..814e0b2dba 100644 --- a/src/include/port/pg_bitutils.h +++ b/src/include/port/pg_bitutils.h @@ -175,16 +175,6 @@ pg_nextpower2_64(uint64 num) return ((uint64) 1) << (pg_leftmost_one_pos64(num) + 1); } -/* - * pg_nextpower2_size_t - * Returns the next higher power of 2 above 'num', for a size_t input. - */ -#if SIZEOF_SIZE_T == 4 -#define pg_nextpower2_size_t(num) pg_nextpower2_32(num) -#else -#define pg_nextpower2_size_t(num) pg_nextpower2_64(num) -#endif - /* * pg_prevpower2_32 * Returns the next lower power of 2 below 'num', or 'num' if it's @@ -211,16 +201,6 @@ pg_prevpower2_64(uint64 num) return ((uint64) 1) << pg_leftmost_one_pos64(num); } -/* - * pg_prevpower2_size_t - * Returns the next lower power of 2 below 'num', for a size_t input. - */ -#if SIZEOF_SIZE_T == 4 -#define pg_prevpower2_size_t(num) pg_prevpower2_32(num) -#else -#define pg_prevpower2_size_t(num) pg_prevpower2_64(num) -#endif - /* * pg_ceil_log2_32 * Returns equivalent of ceil(log2(num)) @@ -299,4 +279,16 @@ pg_rotate_left32(uint32 word, int n) return (word << n) | (word >> (32 - n)); } +/* size_t variants of the above, as required */ + +#if SIZEOF_SIZE_T == 4 +#define pg_leftmost_one_pos_size_t pg_leftmost_one_pos32 +#define pg_nextpower2_size_t pg_nextpower2_32 +#define pg_prevpower2_size_t pg_prevpower2_32 +#else +#define pg_leftmost_one_pos_size_t pg_leftmost_one_pos64 +#define pg_nextpower2_size_t pg_nextpower2_64 +#define pg_prevpower2_size_t pg_prevpower2_64 +#endif + #endif /* PG_BITUTILS_H */ -- 2.36.1
From ad854c0b7fa88a12ec957f9b5bef9b0510e424ac Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Tue, 19 Jul 2022 17:47:10 +1200 Subject: [PATCH v3 2/2] Remove fls function. Commit 4f658dc8 provided the traditional BSD fls() function so it could be used in several places. Later we added a bunch of similar sorts of things in pg_bitutils.h. Let's drop fls.c and the configure probe, and reuse the newer code. Reviewed-by: David Rowley <dgrowle...@gmail.com> Reviewed-by: Tom Lane <t...@sss.pgh.pa.us> Discussion: https://postgr.es/m/CA%2BhUKG%2B7dSX1XF8yFGmYk-%3D48dbjH2kmzZj16XvhbrWP-9BzRg%40mail.gmail.com --- configure | 13 ------ configure.ac | 1 - src/backend/access/hash/hashutil.c | 2 +- src/backend/optimizer/path/allpaths.c | 5 +- src/backend/optimizer/prep/prepunion.c | 2 +- src/backend/utils/mmgr/dsa.c | 14 +++++- src/include/pg_config.h.in | 3 -- src/include/port.h | 4 -- src/port/fls.c | 64 -------------------------- src/tools/msvc/Mkvcbuild.pm | 2 +- src/tools/msvc/Solution.pm | 1 - 11 files changed, 19 insertions(+), 92 deletions(-) delete mode 100644 src/port/fls.c diff --git a/configure b/configure index 59fa82b8d7..33a425562f 100755 --- a/configure +++ b/configure @@ -16771,19 +16771,6 @@ esac fi -ac_fn_c_check_func "$LINENO" "fls" "ac_cv_func_fls" -if test "x$ac_cv_func_fls" = xyes; then : - $as_echo "#define HAVE_FLS 1" >>confdefs.h - -else - case " $LIBOBJS " in - *" fls.$ac_objext "* ) ;; - *) LIBOBJS="$LIBOBJS fls.$ac_objext" - ;; -esac - -fi - ac_fn_c_check_func "$LINENO" "getopt" "ac_cv_func_getopt" if test "x$ac_cv_func_getopt" = xyes; then : $as_echo "#define HAVE_GETOPT 1" >>confdefs.h diff --git a/configure.ac b/configure.ac index 612dabf698..5295cb5806 100644 --- a/configure.ac +++ b/configure.ac @@ -1911,7 +1911,6 @@ fi AC_REPLACE_FUNCS(m4_normalize([ dlopen explicit_bzero - fls getopt getpeereid getrusage diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c index fe37bc47cb..32822dbb6b 100644 --- a/src/backend/access/hash/hashutil.c +++ b/src/backend/access/hash/hashutil.c @@ -436,7 +436,7 @@ _hash_get_oldblock_from_newbucket(Relation rel, Bucket new_bucket) * started. Masking the most significant bit of new bucket would give us * old bucket. */ - mask = (((uint32) 1) << (fls(new_bucket) - 1)) - 1; + mask = (((uint32) 1) << pg_leftmost_one_pos32(new_bucket)) - 1; old_bucket = new_bucket & mask; metabuf = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE); diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 028d9e1680..63f0f6b79c 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -47,6 +47,7 @@ #include "parser/parsetree.h" #include "partitioning/partbounds.h" #include "partitioning/partprune.h" +#include "port/pg_bitutils.h" #include "rewrite/rewriteManip.h" #include "utils/lsyscache.h" @@ -1491,7 +1492,7 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel, if (enable_parallel_append) { parallel_workers = Max(parallel_workers, - fls(list_length(live_childrels))); + pg_leftmost_one_pos32(list_length(live_childrels)) + 1); parallel_workers = Min(parallel_workers, max_parallel_workers_per_gather); } @@ -1542,7 +1543,7 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel, * the planned number of parallel workers. */ parallel_workers = Max(parallel_workers, - fls(list_length(live_childrels))); + pg_leftmost_one_pos32(list_length(live_childrels)) + 1); parallel_workers = Min(parallel_workers, max_parallel_workers_per_gather); Assert(parallel_workers > 0); diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index 2214920dea..043181b586 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -675,7 +675,7 @@ generate_union_paths(SetOperationStmt *op, PlannerInfo *root, if (enable_parallel_append) { parallel_workers = Max(parallel_workers, - fls(list_length(partial_pathlist))); + pg_leftmost_one_pos32(list_length(partial_pathlist)) + 1); parallel_workers = Min(parallel_workers, max_parallel_workers_per_gather); } diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c index b6cb8fa13d..82376fde2d 100644 --- a/src/backend/utils/mmgr/dsa.c +++ b/src/backend/utils/mmgr/dsa.c @@ -51,6 +51,7 @@ #include "postgres.h" #include "port/atomics.h" +#include "port/pg_bitutils.h" #include "storage/dsm.h" #include "storage/ipc.h" #include "storage/lwlock.h" @@ -137,7 +138,18 @@ typedef size_t dsa_segment_index; * free pages? There is no point in looking in segments in lower bins; they * definitely can't service a request for n free pages. */ -#define contiguous_pages_to_segment_bin(n) Min(fls(n), DSA_NUM_SEGMENT_BINS - 1) +static inline size_t +contiguous_pages_to_segment_bin(size_t n) +{ + size_t bin; + + if (n == 0) + bin = 0; + else + bin = pg_leftmost_one_pos_size_t(n) + 1; + + return Min(bin, DSA_NUM_SEGMENT_BINS - 1); +} /* Macros for access to locks. */ #define DSA_AREA_LOCK(area) (&area->control->lock) diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 529fb84a86..86d0b1941b 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -191,9 +191,6 @@ /* Define to 1 if you have the `fdatasync' function. */ #undef HAVE_FDATASYNC -/* Define to 1 if you have the `fls' function. */ -#undef HAVE_FLS - /* Define to 1 if fseeko (and presumably ftello) exists and is declared. */ #undef HAVE_FSEEKO diff --git a/src/include/port.h b/src/include/port.h index e647f62b77..d39b04141f 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -366,10 +366,6 @@ extern int gettimeofday(struct timeval *tp, struct timezone *tzp); #define pgoff_t off_t #endif -#ifndef HAVE_FLS -extern int fls(int mask); -#endif - #ifndef HAVE_GETPEEREID /* On Windows, Perl might have incompatible definitions of uid_t and gid_t. */ #ifndef PLPERL_HAVE_UID_GID diff --git a/src/port/fls.c b/src/port/fls.c deleted file mode 100644 index 19b4221826..0000000000 --- a/src/port/fls.c +++ /dev/null @@ -1,64 +0,0 @@ -/*------------------------------------------------------------------------- - * - * fls.c - * finds the last (most significant) bit that is set - * - * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group - * - * - * IDENTIFICATION - * src/port/fls.c - * - * This file was taken from FreeBSD to provide an implementation of fls() - * for platforms that lack it. Note that the operating system's version may - * be substantially more efficient than ours, since some platforms have an - * assembly instruction that does exactly this. - * - * The FreeBSD copyright terms follow. - */ - -/*- - * Copyright (c) 1990, 1993 - * The Regents of the University of California. All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions - * are met: - * 1. Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * 2. Redistributions in binary form must reproduce the above copyright - * notice, this list of conditions and the following disclaimer in the - * documentation and/or other materials provided with the distribution. - * 4. Neither the name of the University nor the names of its contributors - * may be used to endorse or promote products derived from this software - * without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE - * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS - * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) - * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT - * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY - * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF - * SUCH DAMAGE. - */ - -#include "c.h" - -/* - * Find Last Set bit - */ -int -fls(int mask) -{ - int bit; - - if (mask == 0) - return (0); - for (bit = 1; mask != 1; bit++) - mask = (unsigned int) mask >> 1; - return (bit); -} diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index cc7a908d10..c935f776e5 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -99,7 +99,7 @@ sub mkvcbuild $solution = CreateSolution($vsVersion, $config); our @pgportfiles = qw( - chklocale.c explicit_bzero.c fls.c fdatasync.c + chklocale.c explicit_bzero.c fdatasync.c getpeereid.c getrusage.c inet_aton.c getaddrinfo.c gettimeofday.c inet_net_ntop.c kill.c open.c snprintf.c strlcat.c strlcpy.c dirmod.c noblock.c path.c diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index 3ddcd024a7..23296527ae 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -258,7 +258,6 @@ sub GenerateFiles HAVE_EXECINFO_H => undef, HAVE_EXPLICIT_BZERO => undef, HAVE_FDATASYNC => 1, - HAVE_FLS => undef, HAVE_FSEEKO => 1, HAVE_FUNCNAME__FUNC => undef, HAVE_FUNCNAME__FUNCTION => 1, -- 2.36.1
