public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 07/10] add wal_compression_method: zstd 5+ messages / 4 participants [nested] [flat]
* [PATCH 07/10] add wal_compression_method: zstd @ 2021-03-12 20:43 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2021-03-12 20:43 UTC (permalink / raw) --- configure | 163 ++++++++++++++++++ configure.ac | 26 +++ doc/src/sgml/config.sgml | 2 +- src/backend/access/transam/xlog.c | 3 + src/backend/access/transam/xloginsert.c | 19 ++ src/backend/access/transam/xlogreader.c | 16 ++ src/backend/utils/misc/postgresql.conf.sample | 2 +- src/include/access/xlog_internal.h | 1 + src/include/pg_config.h.in | 3 + src/tools/msvc/Solution.pm | 1 + 10 files changed, 234 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 3afa1e87e3..20c6e08c02 100755 --- a/configure +++ b/configure @@ -699,6 +699,9 @@ with_gnu_ld LD LDFLAGS_SL LDFLAGS_EX +ZSTD_LIBS +ZSTD_CFLAGS +with_zstd LZ4_LIBS LZ4_CFLAGS with_lz4 @@ -868,6 +871,7 @@ with_libxslt with_system_tzdata with_zlib with_lz4 +with_zstd with_gnu_ld with_ssl with_openssl @@ -897,6 +901,8 @@ XML2_CFLAGS XML2_LIBS LZ4_CFLAGS LZ4_LIBS +ZSTD_CFLAGS +ZSTD_LIBS LDFLAGS_EX LDFLAGS_SL PERL @@ -1576,6 +1582,7 @@ Optional Packages: use system time zone data in DIR --without-zlib do not use Zlib --without-lz4 build without LZ4 support + --with-zstd build with Zstd compression library --with-gnu-ld assume the C compiler uses GNU ld [default=no] --with-ssl=LIB use LIB for SSL/TLS support (openssl) --with-openssl obsolete spelling of --with-ssl=openssl @@ -1605,6 +1612,8 @@ Some influential environment variables: XML2_LIBS linker flags for XML2, overriding pkg-config LZ4_CFLAGS C compiler flags for LZ4, overriding pkg-config LZ4_LIBS linker flags for LZ4, overriding pkg-config + ZSTD_CFLAGS C compiler flags for ZSTD, overriding pkg-config + ZSTD_LIBS linker flags for ZSTD, overriding pkg-config LDFLAGS_EX extra linker flags for linking executables only LDFLAGS_SL extra linker flags for linking shared libraries only PERL Perl program @@ -8705,6 +8714,137 @@ fi CFLAGS="$LZ4_CFLAGS $CFLAGS" fi +# +# ZSTD +# +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to build with zstd support" >&5 +$as_echo_n "checking whether to build with zstd support... " >&6; } + + + +# Check whether --with-zstd was given. +if test "${with_zstd+set}" = set; then : + withval=$with_zstd; + case $withval in + yes) + +$as_echo "#define USE_ZSTD 1" >>confdefs.h + + ;; + no) + : + ;; + *) + as_fn_error $? "no argument expected for --with-zstd option" "$LINENO" 5 + ;; + esac + +else + with_zstd=no + +fi + + +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $with_zstd" >&5 +$as_echo "$with_zstd" >&6; } + + +if test "$with_zstd" = yes; then + +pkg_failed=no +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for libzstd" >&5 +$as_echo_n "checking for libzstd... " >&6; } + +if test -n "$ZSTD_CFLAGS"; then + pkg_cv_ZSTD_CFLAGS="$ZSTD_CFLAGS" + elif test -n "$PKG_CONFIG"; then + if test -n "$PKG_CONFIG" && \ + { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libzstd\""; } >&5 + ($PKG_CONFIG --exists --print-errors "libzstd") 2>&5 + ac_status=$? + $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5 + test $ac_status = 0; }; then + pkg_cv_ZSTD_CFLAGS=`$PKG_CONFIG --cflags "libzstd" 2>/dev/null` + test "x$?" != "x0" && pkg_failed=yes +else + pkg_failed=yes +fi + else + pkg_failed=untried +fi +if test -n "$ZSTD_LIBS"; then + pkg_cv_ZSTD_LIBS="$ZSTD_LIBS" + elif test -n "$PKG_CONFIG"; then + if test -n "$PKG_CONFIG" && \ + { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libzstd\""; } >&5 + ($PKG_CONFIG --exists --print-errors "libzstd") 2>&5 + ac_status=$? + $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5 + test $ac_status = 0; }; then + pkg_cv_ZSTD_LIBS=`$PKG_CONFIG --libs "libzstd" 2>/dev/null` + test "x$?" != "x0" && pkg_failed=yes +else + pkg_failed=yes +fi + else + pkg_failed=untried +fi + + + +if test $pkg_failed = yes; then + { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 +$as_echo "no" >&6; } + +if $PKG_CONFIG --atleast-pkgconfig-version 0.20; then + _pkg_short_errors_supported=yes +else + _pkg_short_errors_supported=no +fi + if test $_pkg_short_errors_supported = yes; then + ZSTD_PKG_ERRORS=`$PKG_CONFIG --short-errors --print-errors --cflags --libs "libzstd" 2>&1` + else + ZSTD_PKG_ERRORS=`$PKG_CONFIG --print-errors --cflags --libs "libzstd" 2>&1` + fi + # Put the nasty error message in config.log where it belongs + echo "$ZSTD_PKG_ERRORS" >&5 + + as_fn_error $? "Package requirements (libzstd) were not met: + +$ZSTD_PKG_ERRORS + +Consider adjusting the PKG_CONFIG_PATH environment variable if you +installed software in a non-standard prefix. + +Alternatively, you may set the environment variables ZSTD_CFLAGS +and ZSTD_LIBS to avoid the need to call pkg-config. +See the pkg-config man page for more details." "$LINENO" 5 +elif test $pkg_failed = untried; then + { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 +$as_echo "no" >&6; } + { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5 +$as_echo "$as_me: error: in \`$ac_pwd':" >&2;} +as_fn_error $? "The pkg-config script could not be found or is too old. Make sure it +is in your PATH or set the PKG_CONFIG environment variable to the full +path to pkg-config. + +Alternatively, you may set the environment variables ZSTD_CFLAGS +and ZSTD_LIBS to avoid the need to call pkg-config. +See the pkg-config man page for more details. + +To get pkg-config, see <http://pkg-config.freedesktop.org/;. +See \`config.log' for more details" "$LINENO" 5; } +else + ZSTD_CFLAGS=$pkg_cv_ZSTD_CFLAGS + ZSTD_LIBS=$pkg_cv_ZSTD_LIBS + { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5 +$as_echo "yes" >&6; } + +fi + LIBS="$ZSTD_LIBS $LIBS" + CFLAGS="$ZSTD_CFLAGS $CFLAGS" +fi + # # Assignments # @@ -13556,6 +13696,29 @@ done CPPFLAGS=$ac_save_CPPFLAGS fi +if test "$with_zstd" = yes; then + ac_save_CPPFLAGS=$CPPFLAGS + CPPFLAGS="$ZSTD_CFLAGS $CPPFLAGS" + + # Verify we have zstd's header files + for ac_header in zstd.h +do : + ac_fn_c_check_header_mongrel "$LINENO" "zstd.h" "ac_cv_header_zstd_h" "$ac_includes_default" +if test "x$ac_cv_header_zstd_h" = xyes; then : + cat >>confdefs.h <<_ACEOF +#define HAVE_ZSTD_H 1 +_ACEOF + +else + as_fn_error $? "zstd.h header file is required for zstd" "$LINENO" 5 +fi + +done + + + CPPFLAGS=$ac_save_CPPFLAGS +fi + if test "$with_gssapi" = yes ; then for ac_header in gssapi/gssapi.h do : diff --git a/configure.ac b/configure.ac index d62ea5f742..c4956745ce 100644 --- a/configure.ac +++ b/configure.ac @@ -1001,6 +1001,21 @@ if test "$with_lz4" = yes; then CFLAGS="$LZ4_CFLAGS $CFLAGS" fi +# +# ZSTD +# +AC_MSG_CHECKING([whether to build with zstd support]) +PGAC_ARG_BOOL(with, zstd, no, [build with Zstd compression library], + [AC_DEFINE([USE_ZSTD], 1, [Define to 1 to build with zstd support. (--with-zstd)])]) +AC_MSG_RESULT([$with_zstd]) +AC_SUBST(with_zstd) + +if test "$with_zstd" = yes; then + PKG_CHECK_MODULES(ZSTD, libzstd) + LIBS="$ZSTD_LIBS $LIBS" + CFLAGS="$ZSTD_CFLAGS $CFLAGS" +fi + # # Assignments # @@ -1433,6 +1448,17 @@ if test "$with_lz4" = yes; then CPPFLAGS=$ac_save_CPPFLAGS fi +if test "$with_zstd" = yes; then + ac_save_CPPFLAGS=$CPPFLAGS + CPPFLAGS="$ZSTD_CFLAGS $CPPFLAGS" + + # Verify we have zstd's header files + AC_CHECK_HEADERS(zstd.h, [], + [AC_MSG_ERROR([zstd.h header file is required for zstd])]) + + CPPFLAGS=$ac_save_CPPFLAGS +fi + if test "$with_gssapi" = yes ; then AC_CHECK_HEADERS(gssapi/gssapi.h, [], [AC_CHECK_HEADERS(gssapi.h, [], [AC_MSG_ERROR([gssapi.h header file is required for GSSAPI])])]) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 257775c83b..94dd6ef3e9 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -3083,7 +3083,7 @@ include_dir 'conf.d' This parameter selects the compression method used to compress WAL when <varname>wal_compression</varname> is enabled. The supported methods are pglz, zlib, and (if configured when - <productname>PostgreSQL</productname> was built) lz4. + <productname>PostgreSQL</productname> was built) lz4 and zstd. The default value is <literal>pglz</literal>. Only superusers can change this setting. </para> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 609d954a3a..9bac79b579 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -189,6 +189,9 @@ const struct config_enum_entry wal_compression_options[] = { #endif #ifdef USE_LZ4 {"lz4", WAL_COMPRESSION_LZ4, false}, +#endif +#ifdef USE_ZSTD + {"zstd", WAL_COMPRESSION_ZSTD, false}, #endif {NULL, 0, false} }; diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index 9fe5c30236..5d1ae37dae 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -41,6 +41,10 @@ #include "lz4.h" #endif +#ifdef USE_ZSTD +#include "zstd.h" +#endif + /* Buffer size required to store a compressed version of backup block image */ #define PGLZ_MAX_BLCKSZ PGLZ_MAX_OUTPUT(BLCKSZ) @@ -896,6 +900,21 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length, break; #endif +#ifdef USE_ZSTD + case WAL_COMPRESSION_ZSTD: + len = ZSTD_compress(dest, PGLZ_MAX_BLCKSZ, source, orig_len, 1); + if (ZSTD_isError(len)) + { + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("failed compressing zstd: %s", + ZSTD_getErrorName(len)))); + len = -1; + } + + break; +#endif + default: /* * It should be impossible to get here for unsupported algorithms, diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index fa1e38a810..caa1031d63 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -41,6 +41,10 @@ #include "lz4.h" #endif +#ifdef USE_ZSTD +#include "zstd.h" +#endif + static void report_invalid_record(XLogReaderState *state, const char *fmt,...) pg_attribute_printf(2, 3); static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength); @@ -1564,6 +1568,8 @@ wal_compression_name(WalCompression compression) return "zlib"; case WAL_COMPRESSION_LZ4: return "lz4"; + case WAL_COMPRESSION_ZSTD: + return "zstd"; default: return "???"; } @@ -1621,6 +1627,16 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page) break; #endif +#ifdef USE_ZSTD + case WAL_COMPRESSION_ZSTD: + decomp_result = ZSTD_decompress(tmp.data, BLCKSZ-bkpb->hole_length, + ptr, bkpb->bimg_len); + // XXX: ZSTD_getErrorName + if (ZSTD_isError(decomp_result)) + decomp_result = -1; + break; +#endif + default: report_invalid_record(record, "image at %X/%X is compressed with unsupported codec, block %d (%d/%s)", (uint32) (record->ReadRecPtr >> 32), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 76f494cb9b..d372e2a817 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -213,7 +213,7 @@ # open_sync #full_page_writes = on # recover from partial page writes #wal_compression = off # enable compression of full-page writes -#wal_compression_method = pglz # pglz, zlib, lz4 +#wal_compression_method = pglz # pglz, zlib, lz4, zstd #wal_log_hints = off # also do full page writes of non-critical updates # (change requires restart) #wal_init_zero = on # zero-fill new WAL files diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index e70886b81c..48b16b6083 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -329,6 +329,7 @@ typedef enum WalCompression WAL_COMPRESSION_PGLZ, WAL_COMPRESSION_ZLIB, WAL_COMPRESSION_LZ4, + WAL_COMPRESSION_ZSTD, } WalCompression; extern const char *wal_compression_name(WalCompression compression); diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 0a6422da4f..ad26393352 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -902,6 +902,9 @@ /* Define to 1 to build with LZ4 support (--with-lz4) */ #undef USE_LZ4 +/* Define to 1 if you have the `zstd' library (-lzstd). */ +#undef USE_ZSTD + /* Define to select named POSIX semaphores. */ #undef USE_NAMED_POSIX_SEMAPHORES diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index 14605371bb..fff0212087 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -486,6 +486,7 @@ sub GenerateFiles USE_LIBXML => undef, USE_LIBXSLT => undef, USE_LZ4 => undef, + USE_ZSTD => $self->{options}->{zstd} ? 1 : undef, USE_LDAP => $self->{options}->{ldap} ? 1 : undef, USE_LLVM => undef, USE_NAMED_POSIX_SEMAPHORES => undef, -- 2.17.0 --0qVF/w3MHQqLSynd Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0008-Default-to-zstd.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Remove vardata parameters from eqjoinsel_inner @ 2025-02-21 10:04 Ilia Evdokimov <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Ilia Evdokimov @ 2025-02-21 10:04 UTC (permalink / raw) To: pgsql-hackers Hi hackers, When calculating selectivity for an inner equijoin, we call eqjoinsel_inner, which uses unused parameters vardata1 and vardata2. These parameters might have been left behind accidentally when we moved getting sslots out of the function. I suggest removing them, as they can be added back at any time if needed. I attached patch with fixes. -- Best regards, Ilia Evdokimov, Tantor Labs LLC. Attachments: [text/x-patch] v1-0001-Remove-unused-vardata-parameters-in-eqjoinsel_inner.patch (1.6K, ../../[email protected]/2-v1-0001-Remove-unused-vardata-parameters-in-eqjoinsel_inner.patch) download | inline diff: From 1bf58c5725ab9a906f57117a2c2e1ee7f287f5d5 Mon Sep 17 00:00:00 2001 From: Ilia Evdokimov <[email protected]> Date: Fri, 21 Feb 2025 12:38:13 +0300 Subject: [PATCH v1] Remove unused vardata parameters in eqjoinsel_inner. The vardata parameters in eqjoinsel_inner are unused, so remove them. This is a static function, so the parameters can easily be added again if they are ever needed. --- src/backend/utils/adt/selfuncs.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index c2918c9c83..63b408bdcc 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -150,7 +150,6 @@ get_index_stats_hook_type get_index_stats_hook = NULL; static double eqsel_internal(PG_FUNCTION_ARGS, bool negate); static double eqjoinsel_inner(Oid opfuncoid, Oid collation, - VariableStatData *vardata1, VariableStatData *vardata2, double nd1, double nd2, bool isdefault1, bool isdefault2, AttStatsSlot *sslot1, AttStatsSlot *sslot2, @@ -2348,7 +2347,6 @@ eqjoinsel(PG_FUNCTION_ARGS) /* We need to compute the inner-join selectivity in all cases */ selec_inner = eqjoinsel_inner(opfuncoid, collation, - &vardata1, &vardata2, nd1, nd2, isdefault1, isdefault2, &sslot1, &sslot2, @@ -2436,7 +2434,6 @@ eqjoinsel(PG_FUNCTION_ARGS) */ static double eqjoinsel_inner(Oid opfuncoid, Oid collation, - VariableStatData *vardata1, VariableStatData *vardata2, double nd1, double nd2, bool isdefault1, bool isdefault2, AttStatsSlot *sslot1, AttStatsSlot *sslot2, -- 2.34.1 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Remove vardata parameters from eqjoinsel_inner @ 2025-03-27 07:48 Richard Guo <[email protected]> parent: Ilia Evdokimov <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Richard Guo @ 2025-03-27 07:48 UTC (permalink / raw) To: Ilia Evdokimov <[email protected]>; +Cc: pgsql-hackers On Fri, Feb 21, 2025 at 7:04 PM Ilia Evdokimov <[email protected]> wrote: > When calculating selectivity for an inner equijoin, we call > eqjoinsel_inner, which uses unused parameters vardata1 and vardata2. > These parameters might have been left behind accidentally when we moved > getting sslots out of the function. I suggest removing them, as they can > be added back at any time if needed. I attached patch with fixes. Yeah, these parameters haven't been used since a314c3407, when we moved get_variable_numdistinct and get_attstatsslot out of eqjoinsel_inner and eqjoinsel_semi to avoid repetitive information lookup when we call both eqjoinsel_inner and eqjoinsel_semi. I'm wondering whether we should also remove parameter vardata1 from eqjoinsel_semi. vardata2 is still needed though to clamp nd2 to be not more than the rel's row estimate. Thanks Richard ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Remove vardata parameters from eqjoinsel_inner @ 2025-03-27 11:00 Ilia Evdokimov <[email protected]> parent: Richard Guo <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Ilia Evdokimov @ 2025-03-27 11:00 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: pgsql-hackers On 27.03.2025 10:48, Richard Guo wrote: > I'm wondering whether we should also remove parameter vardata1 from > eqjoinsel_semi. vardata2 is still needed though to clamp nd2 to be > not more than the rel's row estimate. > > Thanks > Richard Indeed, the parameter vardata1 in eqjoinsel_semi() is currently unused and could logically be removed. However, simply leaving a single parameter named vardata2 would appear strange and unintuitive, as it implicitly suggests the existence of a corresponding "first" parameter. I suggest renaming vardata2 to something more descriptive, such as rhs_vardata, clearly indicating its role related specifically to the right side of the join condition. I attached v2 patch with changes. Any thoughts? -- Best regards, Ilia Evdokimov, Tantor Labs LLC. Attachments: [text/x-patch] v2-0001-Remove-unused-vardata-parameters-in-eqjoinsel.patch (3.3K, ../../[email protected]/3-v2-0001-Remove-unused-vardata-parameters-in-eqjoinsel.patch) download | inline diff: From 6ec99d712d7fda3cc18a8700318158f54bae4c55 Mon Sep 17 00:00:00 2001 From: Evdokimov Ilia <[email protected]> Date: Thu, 27 Mar 2025 13:59:02 +0300 Subject: [PATCH v2] Remove unused vardata parameters in eqjoinsel_inner and eqjoinsel_semi --- src/backend/utils/adt/selfuncs.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 5b35debc8ff..6d428d65d64 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -150,14 +150,13 @@ get_index_stats_hook_type get_index_stats_hook = NULL; static double eqsel_internal(PG_FUNCTION_ARGS, bool negate); static double eqjoinsel_inner(Oid opfuncoid, Oid collation, - VariableStatData *vardata1, VariableStatData *vardata2, double nd1, double nd2, bool isdefault1, bool isdefault2, AttStatsSlot *sslot1, AttStatsSlot *sslot2, Form_pg_statistic stats1, Form_pg_statistic stats2, bool have_mcvs1, bool have_mcvs2); static double eqjoinsel_semi(Oid opfuncoid, Oid collation, - VariableStatData *vardata1, VariableStatData *vardata2, + VariableStatData *rhs_vardata, double nd1, double nd2, bool isdefault1, bool isdefault2, AttStatsSlot *sslot1, AttStatsSlot *sslot2, @@ -2348,7 +2347,6 @@ eqjoinsel(PG_FUNCTION_ARGS) /* We need to compute the inner-join selectivity in all cases */ selec_inner = eqjoinsel_inner(opfuncoid, collation, - &vardata1, &vardata2, nd1, nd2, isdefault1, isdefault2, &sslot1, &sslot2, @@ -2375,7 +2373,7 @@ eqjoinsel(PG_FUNCTION_ARGS) if (!join_is_reversed) selec = eqjoinsel_semi(opfuncoid, collation, - &vardata1, &vardata2, + &vardata2, nd1, nd2, isdefault1, isdefault2, &sslot1, &sslot2, @@ -2388,7 +2386,7 @@ eqjoinsel(PG_FUNCTION_ARGS) Oid commopfuncoid = OidIsValid(commop) ? get_opcode(commop) : InvalidOid; selec = eqjoinsel_semi(commopfuncoid, collation, - &vardata2, &vardata1, + &vardata1, nd2, nd1, isdefault2, isdefault1, &sslot2, &sslot1, @@ -2436,7 +2434,6 @@ eqjoinsel(PG_FUNCTION_ARGS) */ static double eqjoinsel_inner(Oid opfuncoid, Oid collation, - VariableStatData *vardata1, VariableStatData *vardata2, double nd1, double nd2, bool isdefault1, bool isdefault2, AttStatsSlot *sslot1, AttStatsSlot *sslot2, @@ -2633,7 +2630,7 @@ eqjoinsel_inner(Oid opfuncoid, Oid collation, */ static double eqjoinsel_semi(Oid opfuncoid, Oid collation, - VariableStatData *vardata1, VariableStatData *vardata2, + VariableStatData *rhs_vardata, double nd1, double nd2, bool isdefault1, bool isdefault2, AttStatsSlot *sslot1, AttStatsSlot *sslot2, @@ -2662,11 +2659,11 @@ eqjoinsel_semi(Oid opfuncoid, Oid collation, * great, maybe, but it didn't come out of nowhere either. This is most * helpful when the inner relation is empty and consequently has no stats. */ - if (vardata2->rel) + if (rhs_vardata->rel) { - if (nd2 >= vardata2->rel->rows) + if (nd2 >= rhs_vardata->rel->rows) { - nd2 = vardata2->rel->rows; + nd2 = rhs_vardata->rel->rows; isdefault2 = false; } } -- 2.34.1 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Remove vardata parameters from eqjoinsel_inner @ 2025-03-27 13:40 Tom Lane <[email protected]> parent: Richard Guo <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Tom Lane @ 2025-03-27 13:40 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: Ilia Evdokimov <[email protected]>; pgsql-hackers Richard Guo <[email protected]> writes: > On Fri, Feb 21, 2025 at 7:04 PM Ilia Evdokimov > <[email protected]> wrote: >> When calculating selectivity for an inner equijoin, we call >> eqjoinsel_inner, which uses unused parameters vardata1 and vardata2. >> These parameters might have been left behind accidentally when we moved >> getting sslots out of the function. I suggest removing them, as they can >> be added back at any time if needed. I attached patch with fixes. > Yeah, these parameters haven't been used since a314c3407, when we > moved get_variable_numdistinct and get_attstatsslot out of > eqjoinsel_inner and eqjoinsel_semi to avoid repetitive information > lookup when we call both eqjoinsel_inner and eqjoinsel_semi. > I'm wondering whether we should also remove parameter vardata1 from > eqjoinsel_semi. vardata2 is still needed though to clamp nd2 to be > not more than the rel's row estimate. I do not believe this change is worth the code churn. In the first place, we may well need those values again someday. In the second place, the savings would be negligible. (In fact, since eqjoinsel_inner probably gets inlined at its sole call site, the savings would likely be completely nonexistent.) If the caller could avoid calculating the vardata info at all, that would be worth thinking about, but I think it can't. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-03-27 13:40 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-12 20:43 [PATCH 07/10] add wal_compression_method: zstd Justin Pryzby <[email protected]> 2025-02-21 10:04 Remove vardata parameters from eqjoinsel_inner Ilia Evdokimov <[email protected]> 2025-03-27 07:48 ` Re: Remove vardata parameters from eqjoinsel_inner Richard Guo <[email protected]> 2025-03-27 11:00 ` Re: Remove vardata parameters from eqjoinsel_inner Ilia Evdokimov <[email protected]> 2025-03-27 13:40 ` Re: Remove vardata parameters from eqjoinsel_inner Tom Lane <[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