public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 07/10] add wal_compression_method: zstd 6+ messages / 3 participants [nested] [flat]
* [PATCH 07/10] add wal_compression_method: zstd @ 2021-03-12 20:43 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 6+ 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 | 18 +- src/backend/access/transam/xlogreader.c | 15 ++ 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, 231 insertions(+), 3 deletions(-) diff --git a/configure b/configure index 8d76be00c1..81e23418b2 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 # @@ -13559,6 +13699,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 bfcdc88be0..d6f6349067 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 # @@ -1436,6 +1451,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 3657f74de9..307eee6626 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 3c15286bd8..4591e476c6 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -48,10 +48,17 @@ #define LZ4_MAX_BLCKSZ 0 #endif +#ifdef USE_ZSTD +#include "zstd.h" +#define ZSTD_MAX_BLCKSZ ZSTD_COMPRESSBOUND(BLCKSZ) +#else +#define ZSTD_MAX_BLCKSZ 0 +#endif + /* Buffer size required to store a compressed version of backup block image */ #define PGLZ_MAX_BLCKSZ PGLZ_MAX_OUTPUT(BLCKSZ) -#define COMPRESS_BUFSIZE Max(Max(PGLZ_MAX_BLCKSZ, ZLIB_MAX_BLCKSZ), LZ4_MAX_BLCKSZ) +#define COMPRESS_BUFSIZE Max(Max(Max(PGLZ_MAX_BLCKSZ, ZLIB_MAX_BLCKSZ), LZ4_MAX_BLCKSZ), ZSTD_MAX_BLCKSZ) /* * For each block reference registered with XLogRegisterBuffer, we fill in @@ -906,6 +913,15 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length, break; #endif +#ifdef USE_ZSTD + case WAL_COMPRESSION_ZSTD: + len = ZSTD_compress(dest, COMPRESS_BUFSIZE, source, orig_len, + ZSTD_CLEVEL_DEFAULT); + if (ZSTD_isError(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 97165f1bb1..0f9d522087 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); @@ -1549,6 +1553,7 @@ struct walcompression walmethods[] = { {"pglz", WAL_COMPRESSION_PGLZ}, {"zlib", WAL_COMPRESSION_ZLIB}, {"lz4", WAL_COMPRESSION_LZ4}, + {"zstd", WAL_COMPRESSION_ZSTD}, }; /* @@ -1621,6 +1626,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 b908fa48de..fa8146645d 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -337,6 +337,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 --jozmn01XJZjDjM3N Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0008-Default-to-zstd.patch" ^ permalink raw reply [nested|flat] 6+ messages in thread
* Fix comments related to pending statistics @ 2024-12-10 14:16 Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Bertrand Drouvot @ 2024-12-10 14:16 UTC (permalink / raw) To: [email protected] Hi hackers, while working on [1], I came across 2 comments in pgstat.h that I think are not correct. 1. One linked to PgStat_TableCounts pending stats, mentioning the use of memcmp() against zeroes to detect whether there are any stats updates to apply. This is not true anymore as of 07e9e28b56. 2. One linked to PgStat_FunctionCounts pending stats, mentioning the use of memcmp() against zeroes to detect whether there are any pending stats: I think it has never been the case. Please find attached a patch to fix those comments. [1]: https://www.postgresql.org/message-id/flat/ZlGYokUIlERemvpB%40ip-10-97-1-34.eu-west-3.compute.intern... Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v1-0001-Fix-comments-related-to-pending-statistics.patch (1.9K, ../../[email protected]/2-v1-0001-Fix-comments-related-to-pending-statistics.patch) download | inline diff: From a7a026fa0f183bf4d66d85ea05463b69422d20a8 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Tue, 10 Dec 2024 12:11:35 +0000 Subject: [PATCH v1] Fix comments related to pending statistics The comment linked to the PgStat_TableCounts pending stats mentioning the use of memcmp() against zeroes to detect whether there are any stats updates to apply is not true anymore as of 07e9e28b56. The one linked to memcmp() usage for the PgStat_FunctionCounts pending stats has probably never been correct. --- src/include/pgstat.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) 100.0% src/include/ diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 59c28b4aca..795e45653e 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -128,8 +128,8 @@ typedef int64 PgStat_Counter; /* ---------- * PgStat_FunctionCounts The actual per-function counts kept by a backend * - * This struct should contain only actual event counters, because we memcmp - * it against zeroes to detect whether there are any pending stats. + * This struct should contain only actual event counters, because pending stats + * always has non-zero content. * * Note that the time counters are in instr_time format here. We convert to * microseconds in PgStat_Counter format when flushing out pending statistics. @@ -172,8 +172,9 @@ typedef struct PgStat_BackendSubEntry /* ---------- * PgStat_TableCounts The actual per-table counts kept by a backend * - * This struct should contain only actual event counters, because we memcmp - * it against zeroes to detect whether there are any stats updates to apply. + * This struct should contain only actual event counters, because we make use + * of pg_memory_is_all_zeros() to detect whether there are any stats updates to + * apply. * It is a component of PgStat_TableStatus (within-backend state). * * Note: for a table, tuples_returned is the number of tuples successfully -- 2.34.1 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix comments related to pending statistics @ 2024-12-11 05:56 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Michael Paquier @ 2024-12-11 05:56 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: [email protected] On Tue, Dec 10, 2024 at 02:16:14PM +0000, Bertrand Drouvot wrote: > 1. One linked to PgStat_TableCounts pending stats, mentioning the use of > memcmp() against zeroes to detect whether there are any stats updates to apply. > > This is not true anymore as of 07e9e28b56. Oops, you're right. The comment has missed the memo about this mention to memcpy(). > > 2. One linked to PgStat_FunctionCounts pending stats, mentioning the use of > memcmp() against zeroes to detect whether there are any pending stats: I think > it has never been the case. * PgStat_FunctionCounts The actual per-function counts kept by a backend * - * This struct should contain only actual event counters, because we memcmp - * it against zeroes to detect whether there are any pending stats. + * This struct should contain only actual event counters, because pending stats + * always has non-zero content. pgstat_function_flush_cb() in pgstat_function.c says since 5891c7a8ed8f, so that seems like a remnant from the original patch that has introduced this inconsistency across its reviews: "/* localent always has non-zero content */" Your suggestion does not look completely right to me. There is nothing preventing us from using something else than event counters since we don't use memcpy() and there is no comparison work, no? It seems to me that we could remove the entire sentence instead. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix comments related to pending statistics @ 2024-12-11 07:32 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Bertrand Drouvot @ 2024-12-11 07:32 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: [email protected] Hi, On Wed, Dec 11, 2024 at 02:56:08PM +0900, Michael Paquier wrote: > On Tue, Dec 10, 2024 at 02:16:14PM +0000, Bertrand Drouvot wrote: > > > > 2. One linked to PgStat_FunctionCounts pending stats, mentioning the use of > > memcmp() against zeroes to detect whether there are any pending stats: I think > > it has never been the case. > > * PgStat_FunctionCounts The actual per-function counts kept by a backend > * > - * This struct should contain only actual event counters, because we memcmp > - * it against zeroes to detect whether there are any pending stats. > + * This struct should contain only actual event counters, because pending stats > + * always has non-zero content. > > pgstat_function_flush_cb() in pgstat_function.c says since > 5891c7a8ed8f, so that seems like a remnant from the original patch > that has introduced this inconsistency across its reviews: > "/* localent always has non-zero content */" Thanks for looking at it! Yeah and same in pgstat_subscription_flush_cb(). > Your suggestion does not look completely right to me. There is > nothing preventing us from using something else than event counters > since we don't use memcpy() and there is no comparison work, no? It > seems to me that we could remove the entire sentence instead. Do you mean also remove the comments in pgstat_function_flush_cb() and pgstat_subscription_flush_cb()? Those comments look fine to me given the places where those pending entries are created meaning in pgstat_init_function_usage() for the functions and pgstat_report_subscription_error() and pgstat_report_subscription_conflict() for the subscriptions. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix comments related to pending statistics @ 2024-12-12 00:20 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Michael Paquier @ 2024-12-12 00:20 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: [email protected] On Wed, Dec 11, 2024 at 07:32:38AM +0000, Bertrand Drouvot wrote: > On Wed, Dec 11, 2024 at 02:56:08PM +0900, Michael Paquier wrote: >> Your suggestion does not look completely right to me. There is >> nothing preventing us from using something else than event counters >> since we don't use memcpy() and there is no comparison work, no? It >> seems to me that we could remove the entire sentence instead. > > Do you mean also remove the comments in pgstat_function_flush_cb() and > pgstat_subscription_flush_cb()? Those comments look fine to me given > the places where those pending entries are created meaning in > pgstat_init_function_usage() for the functions and pgstat_report_subscription_error() > and pgstat_report_subscription_conflict() for the subscriptions. My apologies for the confusion. I see no problem with the existing comments in pgstat_subscription_flush_cb() and pgstat_function_flush_cb() because they still apply. The comment in pgstat.h The only thing we should do here is to remove the comment for PgStat_FunctionCounts because we could add pointers or something else than plain counters in this structure, and fix the comment of PgStat_TableCounts in the lines of what you are suggesting. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix comments related to pending statistics @ 2024-12-12 05:07 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Bertrand Drouvot @ 2024-12-12 05:07 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: [email protected] Hi, On Thu, Dec 12, 2024 at 09:20:15AM +0900, Michael Paquier wrote: > On Wed, Dec 11, 2024 at 07:32:38AM +0000, Bertrand Drouvot wrote: > > On Wed, Dec 11, 2024 at 02:56:08PM +0900, Michael Paquier wrote: > >> Your suggestion does not look completely right to me. There is > >> nothing preventing us from using something else than event counters > >> since we don't use memcpy() and there is no comparison work, no? It > >> seems to me that we could remove the entire sentence instead. > > > > Do you mean also remove the comments in pgstat_function_flush_cb() and > > pgstat_subscription_flush_cb()? Those comments look fine to me given > > the places where those pending entries are created meaning in > > pgstat_init_function_usage() for the functions and pgstat_report_subscription_error() > > and pgstat_report_subscription_conflict() for the subscriptions. > > My apologies for the confusion. I see no problem with the existing > comments in pgstat_subscription_flush_cb() and > pgstat_function_flush_cb() because they still apply. got it, thanks for the clarification. > The only thing we should do here is to remove the comment for > PgStat_FunctionCounts because we could add pointers or something else > than plain counters in this structure, I see what you mean and you are right (I misread the initial comment so updated it wrongly). Removed in v2 attached. > and fix the comment of > PgStat_TableCounts in the lines of what you are suggesting. Yeap. I also just realized that the same kind of comments are missing for PgStat_BgWriterStats and PgStat_CheckpointerStats (for which we make use of pg_memory_is_all_zeros() to detect whether there are any stats updates to apply). Adding those in passing. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v2-0001-Fix-and-add-comments-related-to-pending-statistic.patch (2.8K, ../../[email protected]/2-v2-0001-Fix-and-add-comments-related-to-pending-statistic.patch) download | inline diff: From ad7b732a799381518de93a1857e7323d4985c4e6 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Tue, 10 Dec 2024 12:11:35 +0000 Subject: [PATCH v2] Fix and add comments related to pending statistics The comment linked to the PgStat_TableCounts pending stats mentioning the use of memcmp() against zeroes to detect whether there are any stats updates to apply is not true anymore as of 07e9e28b56. The one linked to memcmp() usage for the PgStat_FunctionCounts pending stats has probably never been correct. Adding comments around two other structs to mention that they should also contain actual event counters because we make use of pg_memory_is_all_zeros() to detect whether there are any stats updates to apply. --- src/include/pgstat.h | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) 100.0% src/include/ diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 59c28b4aca..ce148b9872 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -128,9 +128,6 @@ typedef int64 PgStat_Counter; /* ---------- * PgStat_FunctionCounts The actual per-function counts kept by a backend * - * This struct should contain only actual event counters, because we memcmp - * it against zeroes to detect whether there are any pending stats. - * * Note that the time counters are in instr_time format here. We convert to * microseconds in PgStat_Counter format when flushing out pending statistics. * ---------- @@ -172,8 +169,9 @@ typedef struct PgStat_BackendSubEntry /* ---------- * PgStat_TableCounts The actual per-table counts kept by a backend * - * This struct should contain only actual event counters, because we memcmp - * it against zeroes to detect whether there are any stats updates to apply. + * This struct should contain only actual event counters, because we make use + * of pg_memory_is_all_zeros() to detect whether there are any stats updates to + * apply. * It is a component of PgStat_TableStatus (within-backend state). * * Note: for a table, tuples_returned is the number of tuples successfully @@ -282,6 +280,11 @@ typedef struct PgStat_ArchiverStats TimestampTz stat_reset_timestamp; } PgStat_ArchiverStats; +/* + * This struct should contain only actual event counters, because we make use + * of pg_memory_is_all_zeros() to detect whether there are any stats updates to + * apply. + */ typedef struct PgStat_BgWriterStats { PgStat_Counter buf_written_clean; @@ -290,6 +293,11 @@ typedef struct PgStat_BgWriterStats TimestampTz stat_reset_timestamp; } PgStat_BgWriterStats; +/* + * This struct should contain only actual event counters, because we make use + * of pg_memory_is_all_zeros() to detect whether there are any stats updates to + * apply. + */ typedef struct PgStat_CheckpointerStats { PgStat_Counter num_timed; -- 2.34.1 ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-12-12 05:07 UTC | newest] Thread overview: 6+ 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]> 2024-12-10 14:16 Fix comments related to pending statistics Bertrand Drouvot <[email protected]> 2024-12-11 05:56 ` Re: Fix comments related to pending statistics Michael Paquier <[email protected]> 2024-12-11 07:32 ` Re: Fix comments related to pending statistics Bertrand Drouvot <[email protected]> 2024-12-12 00:20 ` Re: Fix comments related to pending statistics Michael Paquier <[email protected]> 2024-12-12 05:07 ` Re: Fix comments related to pending statistics Bertrand Drouvot <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox