public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 7/8] add wal_compression_method=zstd
4+ messages / 4 participants
[nested] [flat]
* [PATCH 7/8] add wal_compression_method=zstd
@ 2021-03-12 20:43 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Justin Pryzby @ 2021-03-12 20:43 UTC (permalink / raw)
---
configure | 163 ++++++++++++++++++++++++
configure.ac | 26 ++++
src/backend/access/transam/xlog.c | 3 +
src/backend/access/transam/xloginsert.c | 20 +++
src/backend/access/transam/xlogreader.c | 15 +++
src/include/access/xlog_internal.h | 1 +
src/include/pg_config.h.in | 3 +
src/tools/msvc/Solution.pm | 1 +
8 files changed, 232 insertions(+)
diff --git a/configure b/configure
index 3afa1e87e3..c7c4c951a7 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 support
--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..abac4b28f6 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 support],
+ [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/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f33355e44a..f2749ebf45 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 38ac5f9e94..852e5ef7af 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,22 @@ 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, ZSTD_CLEVEL_DEFAULT);
+ 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 920c47e4d7..6ce2fbfb13 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 "???";
}
@@ -1619,6 +1625,15 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
break;
#endif
+#ifdef USE_ZSTD
+ case WAL_COMPRESSION_ZSTD:
+ decomp_result = ZSTD_decompress(tmp.data, BLCKSZ, 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/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 0993de784e..30f663f448 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 = 0,
WAL_COMPRESSION_ZLIB = 1,
WAL_COMPRESSION_LZ4 = 2,
+ WAL_COMPRESSION_ZSTD = 3,
} 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..7dfa0f31f6 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -307,6 +307,7 @@ sub GenerateFiles
HAVE_LIBXML2 => undef,
HAVE_LIBXSLT => undef,
HAVE_LIBZ => $self->{options}->{zlib} ? 1 : undef,
+ HAVE_LIBZSTD => $self->{options}->{zstd} ? 1 : undef,
HAVE_LINK => undef,
HAVE_LOCALE_T => 1,
HAVE_LONG_INT_64 => undef,
--
2.17.0
--f0KYrhQ4vYSV2aJu--
^ permalink raw reply [nested|flat] 4+ messages in thread
* pg_upgrade check for invalid databases
@ 2024-09-30 00:45 Thomas Krennwallner <[email protected]>
2024-09-30 14:12 ` Re: pg_upgrade check for invalid databases Nathan Bossart <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Krennwallner @ 2024-09-30 00:45 UTC (permalink / raw)
To: pgsql-hackers
Hi,
if a cluster contains invalid databases that we cannot connect to
anymore, pg_upgrade would currently fail when trying to connect to the
first encountered invalid database with
Performing Consistency Checks
-----------------------------
Checking cluster versions ok
connection failure: connection to server on socket "/tmp/.s.PGSQL.50432"
failed: FATAL: cannot connect to invalid database "foo"
HINT: Use DROP DATABASE to drop invalid databases.
Failure, exiting
If there is more than one invalid database, we need to run pg_upgrade
more than once (unless the user inspects pg_database).
I attached two small patches for PG 17 and PG 18 (can be easily
backported to all previous versions upon request). Instead of just
failing to connect with an error, we collect all invalid databases in a
report file invalid_databases.txt:
Performing Consistency Checks
-----------------------------
Checking cluster versions ok
Checking for invalid databases fatal
Your installation contains invalid databases as a consequence of
interrupted DROP DATABASE. They are now marked as corrupted databases
that cannot be connected to anymore. Consider removing them using
DROP DATABASE ...;
A list of invalid databases is in the file:
/usr/local/pgsql/data/18/pg_upgrade_output.d/20240929T200559.707/invalid_databases.txt
Failure, exiting
Any thoughts on the proposed patches?
Attachments:
[text/x-patch] pg18-v1-0001-pg_upgrade-Add-check-for-invalid-databases.patch (8.2K, ../../[email protected]/2-pg18-v1-0001-pg_upgrade-Add-check-for-invalid-databases.patch)
download | inline diff:
From c44f723c033573fa5da72721595bc135a00e3cbf Mon Sep 17 00:00:00 2001
From: Thomas Krennwallner <[email protected]>
Date: Fri, 20 Sep 2024 17:33:05 -0400
Subject: [PATCH] pg_upgrade: Add check for invalid databases.
Currently, pg_upgrade fails to connect to the first invalid database
it encounters in get_loadable_libraries() and then aborts. While we
print a fatal message with a hint what should be done, the output is
terse and does not report further invalid databases present in an
installation.
Instead of just exiting on first error, we collect all
pg_database.datconnlimit values in get_db_infos(), ignore invalid
databases in process_slot(), and then perform check_for_invalid_dbs(),
which collects all invalid databases in a report file
invalid_databases.txt.
---
src/bin/pg_upgrade/check.c | 62 ++++++++++++++++++++++++++
src/bin/pg_upgrade/info.c | 5 ++-
src/bin/pg_upgrade/pg_upgrade.h | 2 +
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 2 +-
src/bin/pg_upgrade/task.c | 32 +++++++++++--
5 files changed, 98 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 12735a4268..b5b0f7cd6f 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -12,6 +12,7 @@
#include "catalog/pg_authid_d.h"
#include "catalog/pg_class_d.h"
#include "catalog/pg_collation.h"
+#include "catalog/pg_database.h"
#include "fe_utils/string_utils.h"
#include "mb/pg_wchar.h"
#include "pg_upgrade.h"
@@ -19,6 +20,7 @@
static void check_new_cluster_is_empty(void);
static void check_is_install_user(ClusterInfo *cluster);
static void check_proper_datallowconn(ClusterInfo *cluster);
+static void check_for_invalid_dbs(ClusterInfo *cluster);
static void check_for_prepared_transactions(ClusterInfo *cluster);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_user_defined_postfix_ops(ClusterInfo *cluster);
@@ -598,6 +600,13 @@ check_and_dump_old_cluster(void)
*/
get_db_rel_and_slot_infos(&old_cluster);
+ /*
+ * Verify that the old cluster does not reference invalid databases.
+ * Connections to such databases are not allowed and break the following
+ * checks.
+ */
+ check_for_invalid_dbs(&old_cluster);
+
init_tablespaces();
get_loadable_libraries();
@@ -691,6 +700,13 @@ check_new_cluster(void)
{
get_db_rel_and_slot_infos(&new_cluster);
+ /*
+ * Verify that the new cluster does not reference invalid databases.
+ * Connections to such databases are not allowed and break the following
+ * checks.
+ */
+ check_for_invalid_dbs(&new_cluster);
+
check_new_cluster_is_empty();
check_loadable_libraries();
@@ -1087,6 +1103,52 @@ check_is_install_user(ClusterInfo *cluster)
check_ok();
}
+/*
+ * check_for_invalid_dbs
+ *
+ * Ensure that all databases are valid as connections to invalid databases
+ * are not allowed.
+ */
+static void
+check_for_invalid_dbs(ClusterInfo *cluster)
+{
+ int dbnum;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for invalid databases");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "invalid_databases.txt");
+
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
+
+ if (active_db->db_connlimit == DATCONNLIMIT_INVALID_DB)
+ {
+ if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", output_path);
+ fprintf(script, "%s\n", active_db->db_name);
+ }
+ }
+
+ if (script)
+ {
+ fclose(script);
+ pg_log(PG_REPORT, "fatal");
+ pg_fatal("Your installation contains invalid databases as a consequence of\n"
+ "interrupted DROP DATABASE. They are now marked as corrupted databases\n"
+ "that cannot be connected to anymore. Consider removing them using\n"
+ " DROP DATABASE ...;\n"
+ "A list of invalid databases is in the file:\n"
+ " %s", output_path);
+ }
+ else
+ check_ok();
+}
+
/*
* check_proper_datallowconn
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index f83ded89cb..0e2bd40d1a 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -409,12 +409,13 @@ get_db_infos(ClusterInfo *cluster)
int tupnum;
DbInfo *dbinfos;
int i_datname,
+ i_datconnlimit,
i_oid,
i_spclocation;
char query[QUERY_ALLOC];
snprintf(query, sizeof(query),
- "SELECT d.oid, d.datname, d.encoding, d.datcollate, d.datctype, ");
+ "SELECT d.oid, d.datname, d.encoding, d.datcollate, d.datctype, d.datconnlimit, ");
if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
snprintf(query + strlen(query), sizeof(query) - strlen(query),
"datlocprovider, datlocale, ");
@@ -436,6 +437,7 @@ get_db_infos(ClusterInfo *cluster)
i_oid = PQfnumber(res, "oid");
i_datname = PQfnumber(res, "datname");
+ i_datconnlimit = PQfnumber(res, "datconnlimit");
i_spclocation = PQfnumber(res, "spclocation");
ntups = PQntuples(res);
@@ -445,6 +447,7 @@ get_db_infos(ClusterInfo *cluster)
{
dbinfos[tupnum].db_oid = atooid(PQgetvalue(res, tupnum, i_oid));
dbinfos[tupnum].db_name = pg_strdup(PQgetvalue(res, tupnum, i_datname));
+ dbinfos[tupnum].db_connlimit = atoi(PQgetvalue(res, tupnum, i_datconnlimit));
snprintf(dbinfos[tupnum].db_tablespace, sizeof(dbinfos[tupnum].db_tablespace), "%s",
PQgetvalue(res, tupnum, i_spclocation));
}
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 53f693c2d4..b1c77ca6de 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -194,6 +194,8 @@ typedef struct
char *db_name; /* database name */
char db_tablespace[MAXPGPATH]; /* database default tablespace
* path */
+ int db_connlimit; /* database invalid if set to
+ * DATCONNLIMIT_INVALID_DB */
RelInfoArr rel_arr; /* array of all user relinfos */
LogicalSlotInfoArr slot_arr; /* array of all LogicalSlotInfo */
} DbInfo;
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 17af2ce61e..c7da6d27b2 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -424,7 +424,7 @@ SKIP:
$mode, '--check',
],
1,
- [qr/invalid/], # pg_upgrade prints errors on stdout :(
+ [qr/invalid_databases\.txt/], # pg_upgrade prints errors on stdout :(
[qr/^$/],
'invalid database causes failure');
rmtree($newnode->data_dir . "/pg_upgrade_output.d");
diff --git a/src/bin/pg_upgrade/task.c b/src/bin/pg_upgrade/task.c
index ba1726c25e..90de2070a1 100644
--- a/src/bin/pg_upgrade/task.c
+++ b/src/bin/pg_upgrade/task.c
@@ -44,6 +44,7 @@
#include "postgres_fe.h"
+#include "catalog/pg_database.h"
#include "common/connect.h"
#include "fe_utils/string_utils.h"
#include "pg_upgrade.h"
@@ -228,6 +229,18 @@ process_query_result(const ClusterInfo *cluster, UpgradeTaskSlot *slot,
PQclear(res);
}
+/*
+ * Closes the connection managed by the slot and frees the slot for immediate reuse.
+ */
+static void
+free_slot(UpgradeTaskSlot *slot)
+{
+ if (slot->conn)
+ PQfinish(slot->conn);
+ memset(slot, 0, sizeof(UpgradeTaskSlot));
+ slot->ready = true;
+}
+
/*
* Advances the state machine for a given slot as necessary.
*/
@@ -255,6 +268,21 @@ process_slot(const ClusterInfo *cluster, UpgradeTaskSlot *slot, const UpgradeTas
* new connection.
*/
slot->db_idx = dbs_processing++;
+
+ /*
+ * Do not try to connect to invalid databases here,
+ * check_for_invalid_dbs() will report them later. Skip this
+ * database instead, free the slot and start initiating the next
+ * connection.
+ */
+ if (cluster->dbarr.dbs[slot->db_idx].db_connlimit == DATCONNLIMIT_INVALID_DB)
+ {
+ dbs_complete++;
+ free_slot(slot);
+ process_slot(cluster, slot, task);
+ return;
+ }
+
slot->state = CONNECTING;
start_conn(cluster, slot);
@@ -314,9 +342,7 @@ process_slot(const ClusterInfo *cluster, UpgradeTaskSlot *slot, const UpgradeTas
* through the slots.
*/
dbs_complete++;
- PQfinish(slot->conn);
- memset(slot, 0, sizeof(UpgradeTaskSlot));
- slot->ready = true;
+ free_slot(slot);
process_slot(cluster, slot, task);
--
2.46.1
[text/x-patch] pg17-v1-0001-pg_upgrade-Add-check-for-invalid-databases.patch (7.1K, ../../[email protected]/3-pg17-v1-0001-pg_upgrade-Add-check-for-invalid-databases.patch)
download | inline diff:
From bd6da1bbbfe207e91c7ad4fff008da70a99d684c Mon Sep 17 00:00:00 2001
From: Thomas Krennwallner <[email protected]>
Date: Thu, 12 Sep 2024 16:29:06 -0400
Subject: [PATCH] pg_upgrade: Add check for invalid databases.
Currently, pg_upgrade fails to connect to the first invalid database
it encounters in get_loadable_libraries() and then aborts. While we
print a fatal message with a hint what should be done, the output is
terse and does not report further invalid databases present in an
installation.
Instead of just exiting on first error, we collect all
pg_database.datconnlimit values in get_db_infos(), ignore invalid
databases in get_db_rel_and_slot_infos(), and then perform
check_for_invalid_dbs(), which collects all invalid databases in a
report file invalid_databases.txt.
---
src/bin/pg_upgrade/check.c | 62 ++++++++++++++++++++++++++
src/bin/pg_upgrade/info.c | 13 +++++-
src/bin/pg_upgrade/pg_upgrade.h | 2 +
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 2 +-
4 files changed, 77 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 1cf84cc1bb..d56ea52c27 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -12,6 +12,7 @@
#include "catalog/pg_authid_d.h"
#include "catalog/pg_class_d.h"
#include "catalog/pg_collation.h"
+#include "catalog/pg_database.h"
#include "fe_utils/string_utils.h"
#include "mb/pg_wchar.h"
#include "pg_upgrade.h"
@@ -19,6 +20,7 @@
static void check_new_cluster_is_empty(void);
static void check_is_install_user(ClusterInfo *cluster);
static void check_proper_datallowconn(ClusterInfo *cluster);
+static void check_for_invalid_dbs(ClusterInfo *cluster);
static void check_for_prepared_transactions(ClusterInfo *cluster);
static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
static void check_for_user_defined_postfix_ops(ClusterInfo *cluster);
@@ -586,6 +588,13 @@ check_and_dump_old_cluster(bool live_check)
*/
get_db_rel_and_slot_infos(&old_cluster, live_check);
+ /*
+ * Verify that the old cluster does not reference invalid databases.
+ * Connections to such databases are not allowed and break the following
+ * checks.
+ */
+ check_for_invalid_dbs(&old_cluster);
+
init_tablespaces();
get_loadable_libraries();
@@ -679,6 +688,13 @@ check_new_cluster(void)
{
get_db_rel_and_slot_infos(&new_cluster, false);
+ /*
+ * Verify that the new cluster does not reference invalid databases.
+ * Connections to such databases are not allowed and break the following
+ * checks.
+ */
+ check_for_invalid_dbs(&new_cluster);
+
check_new_cluster_is_empty();
check_loadable_libraries();
@@ -1075,6 +1091,52 @@ check_is_install_user(ClusterInfo *cluster)
check_ok();
}
+/*
+ * check_for_invalid_dbs
+ *
+ * Ensure that all databases are valid as connections to invalid databases
+ * are not allowed.
+ */
+static void
+check_for_invalid_dbs(ClusterInfo *cluster)
+{
+ int dbnum;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for invalid databases");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "invalid_databases.txt");
+
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
+
+ if (active_db->db_connlimit == DATCONNLIMIT_INVALID_DB)
+ {
+ if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", output_path);
+ fprintf(script, "%s\n", active_db->db_name);
+ }
+ }
+
+ if (script)
+ {
+ fclose(script);
+ pg_log(PG_REPORT, "fatal");
+ pg_fatal("Your installation contains invalid databases as a consequence of\n"
+ "interrupted DROP DATABASE. They are now marked as corrupted databases\n"
+ "that cannot be connected to anymore. Consider removing them using\n"
+ " DROP DATABASE ...;\n"
+ "A list of invalid databases is in the file:\n"
+ " %s", output_path);
+ }
+ else
+ check_ok();
+}
+
/*
* check_proper_datallowconn
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index 5c041fa06e..871b1fc30a 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -11,6 +11,7 @@
#include "access/transam.h"
#include "catalog/pg_class_d.h"
+#include "catalog/pg_database.h"
#include "pg_upgrade.h"
static void create_rel_filename_map(const char *old_data, const char *new_data,
@@ -290,6 +291,13 @@ get_db_rel_and_slot_infos(ClusterInfo *cluster, bool live_check)
{
DbInfo *pDbInfo = &cluster->dbarr.dbs[dbnum];
+ /*
+ * Do not try to connect to invalid databases here,
+ * check_for_invalid_dbs() will report them later.
+ */
+ if (pDbInfo->db_connlimit == DATCONNLIMIT_INVALID_DB)
+ continue;
+
get_rel_infos(cluster, pDbInfo);
if (cluster == &old_cluster)
@@ -384,12 +392,13 @@ get_db_infos(ClusterInfo *cluster)
int tupnum;
DbInfo *dbinfos;
int i_datname,
+ i_datconnlimit,
i_oid,
i_spclocation;
char query[QUERY_ALLOC];
snprintf(query, sizeof(query),
- "SELECT d.oid, d.datname, d.encoding, d.datcollate, d.datctype, ");
+ "SELECT d.oid, d.datname, d.encoding, d.datcollate, d.datctype, d.datconnlimit, ");
if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
snprintf(query + strlen(query), sizeof(query) - strlen(query),
"datlocprovider, datlocale, ");
@@ -411,6 +420,7 @@ get_db_infos(ClusterInfo *cluster)
i_oid = PQfnumber(res, "oid");
i_datname = PQfnumber(res, "datname");
+ i_datconnlimit = PQfnumber(res, "datconnlimit");
i_spclocation = PQfnumber(res, "spclocation");
ntups = PQntuples(res);
@@ -420,6 +430,7 @@ get_db_infos(ClusterInfo *cluster)
{
dbinfos[tupnum].db_oid = atooid(PQgetvalue(res, tupnum, i_oid));
dbinfos[tupnum].db_name = pg_strdup(PQgetvalue(res, tupnum, i_datname));
+ dbinfos[tupnum].db_connlimit = atoi(PQgetvalue(res, tupnum, i_datconnlimit));
snprintf(dbinfos[tupnum].db_tablespace, sizeof(dbinfos[tupnum].db_tablespace), "%s",
PQgetvalue(res, tupnum, i_spclocation));
}
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index e2b99b49fa..ed5d45fa06 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -195,6 +195,8 @@ typedef struct
char *db_name; /* database name */
char db_tablespace[MAXPGPATH]; /* database default tablespace
* path */
+ int db_connlimit; /* database invalid if set to
+ * DATCONNLIMIT_INVALID_DB */
RelInfoArr rel_arr; /* array of all user relinfos */
LogicalSlotInfoArr slot_arr; /* array of all LogicalSlotInfo */
} DbInfo;
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 17af2ce61e..c7da6d27b2 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -424,7 +424,7 @@ SKIP:
$mode, '--check',
],
1,
- [qr/invalid/], # pg_upgrade prints errors on stdout :(
+ [qr/invalid_databases\.txt/], # pg_upgrade prints errors on stdout :(
[qr/^$/],
'invalid database causes failure');
rmtree($newnode->data_dir . "/pg_upgrade_output.d");
--
2.46.1
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: pg_upgrade check for invalid databases
2024-09-30 00:45 pg_upgrade check for invalid databases Thomas Krennwallner <[email protected]>
@ 2024-09-30 14:12 ` Nathan Bossart <[email protected]>
2024-09-30 14:55 ` Re: pg_upgrade check for invalid databases Tom Lane <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Nathan Bossart @ 2024-09-30 14:12 UTC (permalink / raw)
To: Thomas Krennwallner <[email protected]>; +Cc: pgsql-hackers
On Sun, Sep 29, 2024 at 08:45:50PM -0400, Thomas Krennwallner wrote:
> if a cluster contains invalid databases that we cannot connect to anymore,
> pg_upgrade would currently fail when trying to connect to the first
> encountered invalid database with
>
> [...]
>
> If there is more than one invalid database, we need to run pg_upgrade more
> than once (unless the user inspects pg_database).
>
> I attached two small patches for PG 17 and PG 18 (can be easily backported
> to all previous versions upon request). Instead of just failing to connect
> with an error, we collect all invalid databases in a report file
> invalid_databases.txt:
Should we have pg_upgrade skip invalid databases? If the only valid action
is to drop them, IMHO it seems unnecessary to require users to manually
drop them before retrying pg_upgrade.
--
nathan
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: pg_upgrade check for invalid databases
2024-09-30 00:45 pg_upgrade check for invalid databases Thomas Krennwallner <[email protected]>
2024-09-30 14:12 ` Re: pg_upgrade check for invalid databases Nathan Bossart <[email protected]>
@ 2024-09-30 14:55 ` Tom Lane <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Tom Lane @ 2024-09-30 14:55 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Thomas Krennwallner <[email protected]>; pgsql-hackers
Nathan Bossart <[email protected]> writes:
> Should we have pg_upgrade skip invalid databases? If the only valid action
> is to drop them, IMHO it seems unnecessary to require users to manually
> drop them before retrying pg_upgrade.
I was thinking the same. But I wonder if there is any chance of
losing data that could be recoverable. It feels like this should
not be a default behavior.
TBH I'm not finding anything very much wrong with the current
behavior... this has to be a rare situation, do we need to add
debatable behavior to make it easier?
regards, tom lane
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-09-30 14:55 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12 20:43 [PATCH 7/8] add wal_compression_method=zstd Justin Pryzby <[email protected]>
2024-09-30 00:45 pg_upgrade check for invalid databases Thomas Krennwallner <[email protected]>
2024-09-30 14:12 ` Re: pg_upgrade check for invalid databases Nathan Bossart <[email protected]>
2024-09-30 14:55 ` Re: pg_upgrade check for invalid databases 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