public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 4/5] pg_rewind: Refactor the abstraction to fetch from local/libpq source.
6+ messages / 4 participants
[nested] [flat]
* [PATCH v2 4/5] pg_rewind: Refactor the abstraction to fetch from local/libpq source.
@ 2020-08-19 12:34 Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Heikki Linnakangas @ 2020-08-19 12:34 UTC (permalink / raw)
There copy_executeFileMap() and libpq_executeFileMap() contained basically
the same logic, just calling different functions to fetch the source files.
Refactor so that the common logic is in one place, execute_file_actions().
This makes the abstraction of a "source" server more clear, by introducing
a common abstract class, borrowing the object-oriented programming term,
that represents all the operations that can be done on the source server.
There are two implementations of it, one for fetching via libpq, and
another to fetch from a local directory. This adds some code, but makes it
easier to understand what's going on.
---
src/bin/pg_rewind/copy_fetch.c | 239 +++++----------------
src/bin/pg_rewind/fetch.c | 97 ++++++---
src/bin/pg_rewind/fetch.h | 76 +++++--
src/bin/pg_rewind/file_ops.c | 129 +++++++++++-
src/bin/pg_rewind/file_ops.h | 3 +
src/bin/pg_rewind/libpq_fetch.c | 361 +++++++++++++++-----------------
src/bin/pg_rewind/pg_rewind.c | 70 +++++--
src/bin/pg_rewind/pg_rewind.h | 5 -
8 files changed, 527 insertions(+), 453 deletions(-)
diff --git a/src/bin/pg_rewind/copy_fetch.c b/src/bin/pg_rewind/copy_fetch.c
index 61aed8018b6..9927a45a07a 100644
--- a/src/bin/pg_rewind/copy_fetch.c
+++ b/src/bin/pg_rewind/copy_fetch.c
@@ -1,7 +1,7 @@
/*-------------------------------------------------------------------------
*
* copy_fetch.c
- * Functions for using a data directory as the source.
+ * Functions for using a local data directory as the source.
*
* Portions Copyright (c) 2013-2020, PostgreSQL Global Development Group
*
@@ -9,8 +9,6 @@
*/
#include "postgres_fe.h"
-#include <sys/stat.h>
-#include <dirent.h>
#include <fcntl.h>
#include <unistd.h>
@@ -20,146 +18,70 @@
#include "filemap.h"
#include "pg_rewind.h"
-static void recurse_dir(const char *datadir, const char *path,
- process_file_callback_t callback);
-
-static void execute_pagemap(datapagemap_t *pagemap, const char *path);
-
-/*
- * Traverse through all files in a data directory, calling 'callback'
- * for each file.
- */
-void
-traverse_datadir(const char *datadir, process_file_callback_t callback)
+typedef struct
{
- recurse_dir(datadir, NULL, callback);
-}
-
-/*
- * recursive part of traverse_datadir
- *
- * parentpath is the current subdirectory's path relative to datadir,
- * or NULL at the top level.
- */
-static void
-recurse_dir(const char *datadir, const char *parentpath,
- process_file_callback_t callback)
+ rewind_source common; /* common interface functions */
+
+ const char *datadir; /* path to the source data directory */
+} local_source;
+
+static void local_traverse_files(rewind_source *source,
+ process_file_callback_t callback);
+static char *local_fetch_file(rewind_source *source, const char *path,
+ size_t *filesize);
+static void local_fetch_file_range(rewind_source *source, const char *path,
+ uint64 off, size_t len);
+static void local_finish_fetch(rewind_source *source);
+static void local_destroy(rewind_source *source);
+
+rewind_source *
+init_local_source(const char *datadir)
{
- DIR *xldir;
- struct dirent *xlde;
- char fullparentpath[MAXPGPATH];
+ local_source *src;
- if (parentpath)
- snprintf(fullparentpath, MAXPGPATH, "%s/%s", datadir, parentpath);
- else
- snprintf(fullparentpath, MAXPGPATH, "%s", datadir);
+ src = pg_malloc0(sizeof(local_source));
- xldir = opendir(fullparentpath);
- if (xldir == NULL)
- pg_fatal("could not open directory \"%s\": %m",
- fullparentpath);
+ src->common.traverse_files = local_traverse_files;
+ src->common.fetch_file = local_fetch_file;
+ src->common.queue_fetch_range = local_fetch_file_range;
+ src->common.finish_fetch = local_finish_fetch;
+ src->common.get_current_wal_insert_lsn = NULL;
+ src->common.destroy = local_destroy;
- while (errno = 0, (xlde = readdir(xldir)) != NULL)
- {
- struct stat fst;
- char fullpath[MAXPGPATH * 2];
- char path[MAXPGPATH * 2];
+ src->datadir = datadir;
- if (strcmp(xlde->d_name, ".") == 0 ||
- strcmp(xlde->d_name, "..") == 0)
- continue;
-
- snprintf(fullpath, sizeof(fullpath), "%s/%s", fullparentpath, xlde->d_name);
-
- if (lstat(fullpath, &fst) < 0)
- {
- if (errno == ENOENT)
- {
- /*
- * File doesn't exist anymore. This is ok, if the new primary
- * is running and the file was just removed. If it was a data
- * file, there should be a WAL record of the removal. If it
- * was something else, it couldn't have been anyway.
- *
- * TODO: But complain if we're processing the target dir!
- */
- }
- else
- pg_fatal("could not stat file \"%s\": %m",
- fullpath);
- }
-
- if (parentpath)
- snprintf(path, sizeof(path), "%s/%s", parentpath, xlde->d_name);
- else
- snprintf(path, sizeof(path), "%s", xlde->d_name);
-
- if (S_ISREG(fst.st_mode))
- callback(path, FILE_TYPE_REGULAR, fst.st_size, NULL);
- else if (S_ISDIR(fst.st_mode))
- {
- callback(path, FILE_TYPE_DIRECTORY, 0, NULL);
- /* recurse to handle subdirectories */
- recurse_dir(datadir, path, callback);
- }
-#ifndef WIN32
- else if (S_ISLNK(fst.st_mode))
-#else
- else if (pgwin32_is_junction(fullpath))
-#endif
- {
-#if defined(HAVE_READLINK) || defined(WIN32)
- char link_target[MAXPGPATH];
- int len;
-
- len = readlink(fullpath, link_target, sizeof(link_target));
- if (len < 0)
- pg_fatal("could not read symbolic link \"%s\": %m",
- fullpath);
- if (len >= sizeof(link_target))
- pg_fatal("symbolic link \"%s\" target is too long",
- fullpath);
- link_target[len] = '\0';
-
- callback(path, FILE_TYPE_SYMLINK, 0, link_target);
-
- /*
- * If it's a symlink within pg_tblspc, we need to recurse into it,
- * to process all the tablespaces. We also follow a symlink if
- * it's for pg_wal. Symlinks elsewhere are ignored.
- */
- if ((parentpath && strcmp(parentpath, "pg_tblspc") == 0) ||
- strcmp(path, "pg_wal") == 0)
- recurse_dir(datadir, path, callback);
-#else
- pg_fatal("\"%s\" is a symbolic link, but symbolic links are not supported on this platform",
- fullpath);
-#endif /* HAVE_READLINK */
- }
- }
+ return &src->common;
+}
- if (errno)
- pg_fatal("could not read directory \"%s\": %m",
- fullparentpath);
+static void
+local_traverse_files(rewind_source *source, process_file_callback_t callback)
+{
+ traverse_datadir(((local_source *) source)->datadir, &process_source_file);
+}
- if (closedir(xldir))
- pg_fatal("could not close directory \"%s\": %m",
- fullparentpath);
+static char *
+local_fetch_file(rewind_source *source, const char *path, size_t *filesize)
+{
+ return slurpFile(((local_source *) source)->datadir, path, filesize);
}
/*
- * Copy a file from source to target, between 'begin' and 'end' offsets.
+ * Copy a file from source to target, starting at 'off', for 'len' bytes.
*
* If 'trunc' is true, any existing file with the same name is truncated.
*/
static void
-rewind_copy_file_range(const char *path, off_t begin, off_t end, bool trunc)
+local_fetch_file_range(rewind_source *source, const char *path, uint64 off,
+ size_t len)
{
+ const char *datadir = ((local_source *) source)->datadir;
PGAlignedBlock buf;
char srcpath[MAXPGPATH];
int srcfd;
+ uint64 begin = off;
+ uint64 end = off + len;
- snprintf(srcpath, sizeof(srcpath), "%s/%s", datadir_source, path);
+ snprintf(srcpath, sizeof(srcpath), "%s/%s", datadir, path);
srcfd = open(srcpath, O_RDONLY | PG_BINARY, 0);
if (srcfd < 0)
@@ -169,7 +91,7 @@ rewind_copy_file_range(const char *path, off_t begin, off_t end, bool trunc)
if (lseek(srcfd, begin, SEEK_SET) == -1)
pg_fatal("could not seek in source file: %m");
- open_target_file(path, trunc);
+ open_target_file(path, false);
while (end - begin > 0)
{
@@ -197,70 +119,17 @@ rewind_copy_file_range(const char *path, off_t begin, off_t end, bool trunc)
pg_fatal("could not close file \"%s\": %m", srcpath);
}
-/*
- * Copy all relation data files from datadir_source to datadir_target, which
- * are marked in the given data page map.
- */
-void
-copy_executeFileMap(filemap_t *map)
+static void
+local_finish_fetch(rewind_source *source)
{
- file_entry_t *entry;
- int i;
-
- for (i = 0; i < map->nactions; i++)
- {
- entry = map->actions[i];
- execute_pagemap(&entry->target_modified_pages, entry->path);
-
- switch (entry->action)
- {
- case FILE_ACTION_NONE:
- /* ok, do nothing.. */
- break;
-
- case FILE_ACTION_COPY:
- rewind_copy_file_range(entry->path, 0, entry->source_size, true);
- break;
-
- case FILE_ACTION_TRUNCATE:
- truncate_target_file(entry->path, entry->source_size);
- break;
-
- case FILE_ACTION_COPY_TAIL:
- rewind_copy_file_range(entry->path, entry->target_size,
- entry->source_size, false);
- break;
-
- case FILE_ACTION_CREATE:
- create_target(entry);
- break;
-
- case FILE_ACTION_REMOVE:
- remove_target(entry);
- break;
-
- case FILE_ACTION_UNDECIDED:
- pg_fatal("no action decided for \"%s\"", entry->path);
- break;
- }
- }
-
- close_target_file();
+ /*
+ * Nothing to do, local_fetch_file_range() performs the fetching
+ * immediately.
+ */
}
static void
-execute_pagemap(datapagemap_t *pagemap, const char *path)
+local_destroy(rewind_source *source)
{
- datapagemap_iterator_t *iter;
- BlockNumber blkno;
- off_t offset;
-
- iter = datapagemap_iterate(pagemap);
- while (datapagemap_next(iter, &blkno))
- {
- offset = blkno * BLCKSZ;
- rewind_copy_file_range(path, offset, offset + BLCKSZ, false);
- /* Ok, this block has now been copied from new data dir to old */
- }
- pg_free(iter);
+ pfree(source);
}
diff --git a/src/bin/pg_rewind/fetch.c b/src/bin/pg_rewind/fetch.c
index f41d0f295ea..c8ee38f8e0b 100644
--- a/src/bin/pg_rewind/fetch.c
+++ b/src/bin/pg_rewind/fetch.c
@@ -24,37 +24,78 @@
#include "filemap.h"
#include "pg_rewind.h"
-void
-fetchSourceFileList(void)
-{
- if (datadir_source)
- traverse_datadir(datadir_source, &process_source_file);
- else
- libpqProcessFileList();
-}
-
/*
- * Fetch all relation data files that are marked in the given data page map.
+ * Execute the actions in the file map, fetching data from the source
+ * system as needed.
*/
void
-execute_file_actions(filemap_t *filemap)
+execute_file_actions(filemap_t *filemap, rewind_source *source)
{
- if (datadir_source)
- copy_executeFileMap(filemap);
- else
- libpq_executeFileMap(filemap);
-}
+ int i;
-/*
- * Fetch a single file into a malloc'd buffer. The file size is returned
- * in *filesize. The returned buffer is always zero-terminated, which is
- * handy for text files.
- */
-char *
-fetchFile(const char *filename, size_t *filesize)
-{
- if (datadir_source)
- return slurpFile(datadir_source, filename, filesize);
- else
- return libpqGetFile(filename, filesize);
+ for (i = 0; i < filemap->nactions; i++)
+ {
+ file_entry_t *entry = filemap->actions[i];
+ datapagemap_iterator_t *iter;
+ BlockNumber blkno;
+ off_t offset;
+
+ /*
+ * If this is a relation file, copy the modified blocks.
+ *
+ * This is in addition to any other changes.
+ */
+ iter = datapagemap_iterate(&entry->target_modified_pages);
+ while (datapagemap_next(iter, &blkno))
+ {
+ offset = blkno * BLCKSZ;
+
+ source->queue_fetch_range(source, entry->path, offset, BLCKSZ);
+ }
+ pg_free(iter);
+
+ switch (entry->action)
+ {
+ case FILE_ACTION_NONE:
+ /* nothing else to do */
+ break;
+
+ case FILE_ACTION_COPY:
+ /* Truncate the old file out of the way, if any */
+ open_target_file(entry->path, true);
+ source->queue_fetch_range(source, entry->path,
+ 0, entry->source_size);
+ break;
+
+ case FILE_ACTION_TRUNCATE:
+ truncate_target_file(entry->path, entry->source_size);
+ break;
+
+ case FILE_ACTION_COPY_TAIL:
+ source->queue_fetch_range(source, entry->path,
+ entry->target_size,
+ entry->source_size - entry->target_size);
+ break;
+
+ case FILE_ACTION_REMOVE:
+ remove_target(entry);
+ break;
+
+ case FILE_ACTION_CREATE:
+ create_target(entry);
+ break;
+
+ case FILE_ACTION_UNDECIDED:
+ pg_fatal("no action decided for \"%s\"", entry->path);
+ break;
+ }
+ }
+
+ /*
+ * We've now copied the list of file ranges that we need to fetch to the
+ * temporary table. Now, actually fetch all of those ranges. XXX
+ */
+ source->finish_fetch(source);
+
+ close_target_file();
}
diff --git a/src/bin/pg_rewind/fetch.h b/src/bin/pg_rewind/fetch.h
index b20df8b1537..8be1a9582de 100644
--- a/src/bin/pg_rewind/fetch.h
+++ b/src/bin/pg_rewind/fetch.h
@@ -1,12 +1,12 @@
/*-------------------------------------------------------------------------
*
* fetch.h
- * Fetching data from a local or remote data directory.
+ * Abstraction for fetching from source server.
*
- * This file includes the prototypes for functions used to copy files from
- * one data directory to another. The source to copy from can be a local
- * directory (copy method), or a remote PostgreSQL server (libpq fetch
- * method).
+ * The source server can be either a libpq connection to a live system, or
+ * a local data directory. The 'rewind_source' struct abstracts the
+ * operations to fetch data from the source system, so that the rest of
+ * the code doesn't need to care what kind of a source its dealing with.
*
* Copyright (c) 2013-2020, PostgreSQL Global Development Group
*
@@ -16,29 +16,63 @@
#define FETCH_H
#include "access/xlogdefs.h"
-
+#include "file_ops.h"
#include "filemap.h"
+#include "libpq-fe.h"
+
+typedef struct rewind_source
+{
+ /*
+ * Traverse all files in the source data directory, and call 'callback'
+ * on each file.
+ */
+ void (*traverse_files) (struct rewind_source *,
+ process_file_callback_t callback);
+
+ /*
+ * Fetch a single file into a malloc'd buffer. The file size is returned
+ * in *filesize. The returned buffer is always zero-terminated, which is
+ * handy for text files.
+ */
+ char *(*fetch_file) (struct rewind_source *, const char *path,
+ size_t *filesize);
+
+ /*
+ * Request to fetch (part of) a file in the source system, and write it
+ * the corresponding file in the target system. The source implementation
+ * may queue up the request and execute it later when convenient. Call
+ * finish_fetch() to flush the queue and execute all requests.
+ */
+ void (*queue_fetch_range) (struct rewind_source *, const char *path,
+ uint64 offset, size_t len);
+
+ /*
+ * Execute all requests queued up with queue_fetch_range().
+ */
+ void (*finish_fetch) (struct rewind_source *);
+
+ /*
+ * Get the current WAL insert position in the source system.
+ */
+ XLogRecPtr (*get_current_wal_insert_lsn) (struct rewind_source *);
+
+ /*
+ * Free this rewind_source object.
+ */
+ void (*destroy) (struct rewind_source *);
+
+} rewind_source;
+
/*
- * Common interface. Calls the copy or libpq method depending on global
- * config options.
+ * Execute all the actions in 'filemap'.
*/
-extern void fetchSourceFileList(void);
-extern char *fetchFile(const char *filename, size_t *filesize);
-extern void execute_file_actions(filemap_t *filemap);
+extern void execute_file_actions(filemap_t *filemap, rewind_source *source);
/* in libpq_fetch.c */
-extern void libpqProcessFileList(void);
-extern char *libpqGetFile(const char *filename, size_t *filesize);
-extern void libpq_executeFileMap(filemap_t *map);
-
-extern void libpqConnect(const char *connstr);
-extern XLogRecPtr libpqGetCurrentXlogInsertLocation(void);
+extern rewind_source *init_libpq_source(PGconn *conn);
/* in copy_fetch.c */
-extern void copy_executeFileMap(filemap_t *map);
-
-typedef void (*process_file_callback_t) (const char *path, file_type_t type, size_t size, const char *link_target);
-extern void traverse_datadir(const char *datadir, process_file_callback_t callback);
+extern rewind_source *init_local_source(const char *datadir);
#endif /* FETCH_H */
diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c
index ec37d0b2e0d..4ae343888ee 100644
--- a/src/bin/pg_rewind/file_ops.c
+++ b/src/bin/pg_rewind/file_ops.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <sys/stat.h>
+#include <dirent.h>
#include <fcntl.h>
#include <unistd.h>
@@ -35,6 +36,9 @@ static void remove_target_dir(const char *path);
static void create_target_symlink(const char *path, const char *link);
static void remove_target_symlink(const char *path);
+static void recurse_dir(const char *datadir, const char *parentpath,
+ process_file_callback_t callback);
+
/*
* Open a target file for writing. If 'trunc' is true and the file already
* exists, it will be truncated.
@@ -305,9 +309,6 @@ sync_target_dir(void)
* buffer is actually *filesize + 1. That's handy when reading a text file.
* This function can be used to read binary files as well, you can just
* ignore the zero-terminator in that case.
- *
- * This function is used to implement the fetchFile function in the "fetch"
- * interface (see fetch.c), but is also called directly.
*/
char *
slurpFile(const char *datadir, const char *path, size_t *filesize)
@@ -352,3 +353,125 @@ slurpFile(const char *datadir, const char *path, size_t *filesize)
*filesize = len;
return buffer;
}
+
+/*
+ * Traverse through all files in a data directory, calling 'callback'
+ * for each file.
+ */
+void
+traverse_datadir(const char *datadir, process_file_callback_t callback)
+{
+ recurse_dir(datadir, NULL, callback);
+}
+
+/*
+ * recursive part of traverse_datadir
+ *
+ * parentpath is the current subdirectory's path relative to datadir,
+ * or NULL at the top level.
+ */
+static void
+recurse_dir(const char *datadir, const char *parentpath,
+ process_file_callback_t callback)
+{
+ DIR *xldir;
+ struct dirent *xlde;
+ char fullparentpath[MAXPGPATH];
+
+ if (parentpath)
+ snprintf(fullparentpath, MAXPGPATH, "%s/%s", datadir, parentpath);
+ else
+ snprintf(fullparentpath, MAXPGPATH, "%s", datadir);
+
+ xldir = opendir(fullparentpath);
+ if (xldir == NULL)
+ pg_fatal("could not open directory \"%s\": %m",
+ fullparentpath);
+
+ while (errno = 0, (xlde = readdir(xldir)) != NULL)
+ {
+ struct stat fst;
+ char fullpath[MAXPGPATH * 2];
+ char path[MAXPGPATH * 2];
+
+ if (strcmp(xlde->d_name, ".") == 0 ||
+ strcmp(xlde->d_name, "..") == 0)
+ continue;
+
+ snprintf(fullpath, sizeof(fullpath), "%s/%s", fullparentpath, xlde->d_name);
+
+ if (lstat(fullpath, &fst) < 0)
+ {
+ if (errno == ENOENT)
+ {
+ /*
+ * File doesn't exist anymore. This is ok, if the new primary
+ * is running and the file was just removed. If it was a data
+ * file, there should be a WAL record of the removal. If it
+ * was something else, it couldn't have been anyway.
+ *
+ * TODO: But complain if we're processing the target dir!
+ */
+ }
+ else
+ pg_fatal("could not stat file \"%s\": %m",
+ fullpath);
+ }
+
+ if (parentpath)
+ snprintf(path, sizeof(path), "%s/%s", parentpath, xlde->d_name);
+ else
+ snprintf(path, sizeof(path), "%s", xlde->d_name);
+
+ if (S_ISREG(fst.st_mode))
+ callback(path, FILE_TYPE_REGULAR, fst.st_size, NULL);
+ else if (S_ISDIR(fst.st_mode))
+ {
+ callback(path, FILE_TYPE_DIRECTORY, 0, NULL);
+ /* recurse to handle subdirectories */
+ recurse_dir(datadir, path, callback);
+ }
+#ifndef WIN32
+ else if (S_ISLNK(fst.st_mode))
+#else
+ else if (pgwin32_is_junction(fullpath))
+#endif
+ {
+#if defined(HAVE_READLINK) || defined(WIN32)
+ char link_target[MAXPGPATH];
+ int len;
+
+ len = readlink(fullpath, link_target, sizeof(link_target));
+ if (len < 0)
+ pg_fatal("could not read symbolic link \"%s\": %m",
+ fullpath);
+ if (len >= sizeof(link_target))
+ pg_fatal("symbolic link \"%s\" target is too long",
+ fullpath);
+ link_target[len] = '\0';
+
+ callback(path, FILE_TYPE_SYMLINK, 0, link_target);
+
+ /*
+ * If it's a symlink within pg_tblspc, we need to recurse into it,
+ * to process all the tablespaces. We also follow a symlink if
+ * it's for pg_wal. Symlinks elsewhere are ignored.
+ */
+ if ((parentpath && strcmp(parentpath, "pg_tblspc") == 0) ||
+ strcmp(path, "pg_wal") == 0)
+ recurse_dir(datadir, path, callback);
+#else
+ pg_fatal("\"%s\" is a symbolic link, but symbolic links are not supported on this platform",
+ fullpath);
+#endif /* HAVE_READLINK */
+ }
+ }
+
+ if (errno)
+ pg_fatal("could not read directory \"%s\": %m",
+ fullparentpath);
+
+ if (closedir(xldir))
+ pg_fatal("could not close directory \"%s\": %m",
+ fullparentpath);
+}
diff --git a/src/bin/pg_rewind/file_ops.h b/src/bin/pg_rewind/file_ops.h
index d8466385cf5..c7630859768 100644
--- a/src/bin/pg_rewind/file_ops.h
+++ b/src/bin/pg_rewind/file_ops.h
@@ -23,4 +23,7 @@ extern void sync_target_dir(void);
extern char *slurpFile(const char *datadir, const char *path, size_t *filesize);
+typedef void (*process_file_callback_t) (const char *path, file_type_t type, size_t size, const char *link_target);
+extern void traverse_datadir(const char *datadir, process_file_callback_t callback);
+
#endif /* FILE_OPS_H */
diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetch.c
index 9c541bb73d5..52c4e147e10 100644
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -1,7 +1,7 @@
/*-------------------------------------------------------------------------
*
* libpq_fetch.c
- * Functions for fetching files from a remote server.
+ * Functions for fetching files from a remote server via libpq.
*
* Copyright (c) 2013-2020, PostgreSQL Global Development Group
*
@@ -9,11 +9,6 @@
*/
#include "postgres_fe.h"
-#include <sys/stat.h>
-#include <dirent.h>
-#include <fcntl.h>
-#include <unistd.h>
-
#include "catalog/pg_type_d.h"
#include "common/connect.h"
#include "datapagemap.h"
@@ -23,8 +18,6 @@
#include "pg_rewind.h"
#include "port/pg_bswap.h"
-PGconn *conn = NULL;
-
/*
* Files are fetched max CHUNKSIZE bytes at a time.
*
@@ -34,30 +27,73 @@ PGconn *conn = NULL;
*/
#define CHUNKSIZE 1000000
-static void receiveFileChunks(const char *sql);
-static void execute_pagemap(datapagemap_t *pagemap, const char *path);
-static char *run_simple_query(const char *sql);
-static void run_simple_command(const char *sql);
+typedef struct
+{
+ rewind_source common; /* common interface functions */
+
+ PGconn *conn;
+} libpq_source;
+
+static void init_libpq_conn(PGconn *conn);
+static char *run_simple_query(PGconn *conn, const char *sql);
+static void run_simple_command(PGconn *conn, const char *sql);
+
+/* public interface functions */
+static void libpq_traverse_files(rewind_source *source,
+ process_file_callback_t callback);
+static char *libpq_fetch_file(rewind_source *source, const char *path,
+ size_t *filesize);
+static void libpq_queue_fetch_range(rewind_source *source, const char *path,
+ uint64 off, size_t len);
+static void libpq_finish_fetch(rewind_source *source);
+static void libpq_destroy(rewind_source *source);
+static XLogRecPtr libpq_get_current_wal_insert_lsn(rewind_source *source);
-void
-libpqConnect(const char *connstr)
+/*
+ * Create a new libpq source.
+ *
+ * The caller has already established the connection, but should not try
+ * to use it while the source is active.
+ */
+rewind_source *
+init_libpq_source(PGconn *conn)
{
- char *str;
- PGresult *res;
+ libpq_source *src;
+
+ init_libpq_conn(conn);
- conn = PQconnectdb(connstr);
- if (PQstatus(conn) == CONNECTION_BAD)
- pg_fatal("could not connect to server: %s",
- PQerrorMessage(conn));
+ src = pg_malloc0(sizeof(libpq_source));
- if (showprogress)
- pg_log_info("connected to server");
+ src->common.traverse_files = libpq_traverse_files;
+ src->common.fetch_file = libpq_fetch_file;
+ src->common.queue_fetch_range = libpq_queue_fetch_range;
+ src->common.finish_fetch = libpq_finish_fetch;
+ src->common.get_current_wal_insert_lsn = libpq_get_current_wal_insert_lsn;
+ src->common.destroy = libpq_destroy;
+
+ src->conn = conn;
+
+ return &src->common;
+}
+
+/*
+ * Initialize a libpq connection for use.
+ */
+static void
+init_libpq_conn(PGconn *conn)
+{
+ PGresult *res;
+ char *str;
/* disable all types of timeouts */
- run_simple_command("SET statement_timeout = 0");
- run_simple_command("SET lock_timeout = 0");
- run_simple_command("SET idle_in_transaction_session_timeout = 0");
+ run_simple_command(conn, "SET statement_timeout = 0");
+ run_simple_command(conn, "SET lock_timeout = 0");
+ run_simple_command(conn, "SET idle_in_transaction_session_timeout = 0");
+ /* we don't intend do any updates. Put the connection in read-only mode to keep us honest */
+ run_simple_command(conn, "SET default_transaction_read_only = off");
+
+ /* secure search_path */
res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);
if (PQresultStatus(res) != PGRES_TUPLES_OK)
pg_fatal("could not clear search_path: %s",
@@ -70,7 +106,7 @@ libpqConnect(const char *connstr)
* currently because we use a temporary table. Better to check for it
* explicitly than error out, for a better error message.
*/
- str = run_simple_query("SELECT pg_is_in_recovery()");
+ str = run_simple_query(conn, "SELECT pg_is_in_recovery()");
if (strcmp(str, "f") != 0)
pg_fatal("source server must not be in recovery mode");
pg_free(str);
@@ -80,27 +116,31 @@ libpqConnect(const char *connstr)
* a page is modified while we read it with pg_read_binary_file(), and we
* rely on full page images to fix them.
*/
- str = run_simple_query("SHOW full_page_writes");
+ str = run_simple_query(conn, "SHOW full_page_writes");
if (strcmp(str, "on") != 0)
pg_fatal("full_page_writes must be enabled in the source server");
pg_free(str);
/*
- * Although we don't do any "real" updates, we do work with a temporary
- * table. We don't care about synchronous commit for that. It doesn't
- * otherwise matter much, but if the server is using synchronous
- * replication, and replication isn't working for some reason, we don't
- * want to get stuck, waiting for it to start working again.
+ * First create a temporary table, and COPY to load it with the list of
+ * blocks that we need to fetch.
*/
- run_simple_command("SET synchronous_commit = off");
+ run_simple_command(conn, "CREATE TEMPORARY TABLE fetchchunks(path text, begin int8, len int4)");
+
+ res = PQexec(conn, "COPY fetchchunks FROM STDIN");
+ if (PQresultStatus(res) != PGRES_COPY_IN)
+ pg_fatal("could not send file list: %s",
+ PQresultErrorMessage(res));
+ PQclear(res);
}
/*
- * Runs a query that returns a single value.
+ * Run a query that returns a single value.
+ *
* The result should be pg_free'd after use.
*/
static char *
-run_simple_query(const char *sql)
+run_simple_query(PGconn *conn, const char *sql)
{
PGresult *res;
char *result;
@@ -123,11 +163,12 @@ run_simple_query(const char *sql)
}
/*
- * Runs a command.
+ * Run a command.
+ *
* In the event of a failure, exit immediately.
*/
static void
-run_simple_command(const char *sql)
+run_simple_command(PGconn *conn, const char *sql)
{
PGresult *res;
@@ -141,17 +182,18 @@ run_simple_command(const char *sql)
}
/*
- * Calls pg_current_wal_insert_lsn() function
+ * Call the pg_current_wal_insert_lsn() function in the remote system.
*/
-XLogRecPtr
-libpqGetCurrentXlogInsertLocation(void)
+static XLogRecPtr
+libpq_get_current_wal_insert_lsn(rewind_source *source)
{
+ PGconn *conn = ((libpq_source *) source)->conn;
XLogRecPtr result;
uint32 hi;
uint32 lo;
char *val;
- val = run_simple_query("SELECT pg_current_wal_insert_lsn()");
+ val = run_simple_query(conn, "SELECT pg_current_wal_insert_lsn()");
if (sscanf(val, "%X/%X", &hi, &lo) != 2)
pg_fatal("unrecognized result \"%s\" for current WAL insert location", val);
@@ -166,9 +208,10 @@ libpqGetCurrentXlogInsertLocation(void)
/*
* Get a list of all files in the data directory.
*/
-void
-libpqProcessFileList(void)
+static void
+libpq_traverse_files(rewind_source *source, process_file_callback_t callback)
{
+ PGconn *conn = ((libpq_source *) source)->conn;
PGresult *res;
const char *sql;
int i;
@@ -246,6 +289,48 @@ libpqProcessFileList(void)
PQclear(res);
}
+/*
+ * Queue up a request to fetch a piece of a file from remote system.
+ */
+static void
+libpq_queue_fetch_range(rewind_source *source, const char *path, uint64 off,
+ size_t len)
+{
+ libpq_source *src = (libpq_source *) source;
+ uint64 begin = off;
+ uint64 end = off + len;
+
+ /*
+ * Write the file range to a temporary table in the server.
+ *
+ * The range is sent to the server as a COPY formatted line, to be inserted
+ * into the 'fetchchunks' temporary table. The libpq_finish_fetch() uses
+ * the temporary table to actually fetch the data.
+ */
+
+ /* Split the range into CHUNKSIZE chunks */
+ while (end - begin > 0)
+ {
+ char linebuf[MAXPGPATH + 23];
+ unsigned int len;
+
+ /* Fine as long as CHUNKSIZE is not bigger than UINT32_MAX */
+ if (end - begin > CHUNKSIZE)
+ len = CHUNKSIZE;
+ else
+ len = (unsigned int) (end - begin);
+
+ begin += len;
+
+ snprintf(linebuf, sizeof(linebuf), "%s\t" UINT64_FORMAT "\t%u\n", path, begin, len);
+
+ if (PQputCopyData(src->conn, linebuf, strlen(linebuf)) != 1)
+ pg_fatal("could not send COPY data: %s",
+ PQerrorMessage(src->conn));
+ }
+}
+
+
/*----
* Runs a query, which returns pieces of files from the remote source data
* directory, and overwrites the corresponding parts of target files with
@@ -256,20 +341,46 @@ libpqProcessFileList(void)
* chunk bytea -- file content
*----
*/
+/*
+ * Receive all the queued chunks and write them to the target data directory.
+ */
static void
-receiveFileChunks(const char *sql)
+libpq_finish_fetch(rewind_source *source)
{
+ libpq_source *src = (libpq_source *) source;
PGresult *res;
+ const char *sql;
- if (PQsendQueryParams(conn, sql, 0, NULL, NULL, NULL, NULL, 1) != 1)
- pg_fatal("could not send query: %s", PQerrorMessage(conn));
+ if (PQputCopyEnd(src->conn, NULL) != 1)
+ pg_fatal("could not send end-of-COPY: %s",
+ PQerrorMessage(src->conn));
+
+ while ((res = PQgetResult(src->conn)) != NULL)
+ {
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ pg_fatal("unexpected result while sending file list: %s",
+ PQresultErrorMessage(res));
+ PQclear(res);
+ }
+
+ /*
+ * We've now copied the list of file ranges that we need to fetch to the
+ * temporary table. Now, actually fetch all of those ranges.
+ */
+ sql =
+ "SELECT path, begin,\n"
+ " pg_read_binary_file(path, begin, len, true) AS chunk\n"
+ "FROM fetchchunks\n";
+
+ if (PQsendQueryParams(src->conn, sql, 0, NULL, NULL, NULL, NULL, 1) != 1)
+ pg_fatal("could not send query: %s", PQerrorMessage(src->conn));
pg_log_debug("getting file chunks");
- if (PQsetSingleRowMode(conn) != 1)
+ if (PQsetSingleRowMode(src->conn) != 1)
pg_fatal("could not set libpq connection to single row mode");
- while ((res = PQgetResult(conn)) != NULL)
+ while ((res = PQgetResult(src->conn)) != NULL)
{
char *filename;
int filenamelen;
@@ -363,28 +474,29 @@ receiveFileChunks(const char *sql)
}
/*
- * Receive a single file as a malloc'd buffer.
+ * Fetch a single file as a malloc'd buffer.
*/
-char *
-libpqGetFile(const char *filename, size_t *filesize)
+static char *
+libpq_fetch_file(rewind_source *source, const char *path, size_t *filesize)
{
+ PGconn *conn = ((libpq_source *) source)->conn;
PGresult *res;
char *result;
int len;
const char *paramValues[1];
- paramValues[0] = filename;
+ paramValues[0] = path;
res = PQexecParams(conn, "SELECT pg_read_binary_file($1)",
1, NULL, paramValues, NULL, NULL, 1);
if (PQresultStatus(res) != PGRES_TUPLES_OK)
pg_fatal("could not fetch remote file \"%s\": %s",
- filename, PQresultErrorMessage(res));
+ path, PQresultErrorMessage(res));
/* sanity check the result set */
if (PQntuples(res) != 1 || PQgetisnull(res, 0, 0))
pg_fatal("unexpected result set while fetching remote file \"%s\"",
- filename);
+ path);
/* Read result to local variables */
len = PQgetlength(res, 0, 0);
@@ -394,7 +506,7 @@ libpqGetFile(const char *filename, size_t *filesize)
PQclear(res);
- pg_log_debug("fetched file \"%s\", length %d", filename, len);
+ pg_log_debug("fetched file \"%s\", length %d", path, len);
if (filesize)
*filesize = len;
@@ -402,142 +514,11 @@ libpqGetFile(const char *filename, size_t *filesize)
}
/*
- * Write a file range to a temporary table in the server.
- *
- * The range is sent to the server as a COPY formatted line, to be inserted
- * into the 'fetchchunks' temporary table. It is used in receiveFileChunks()
- * function to actually fetch the data.
+ * Close a libpq source.
*/
static void
-fetch_file_range(const char *path, uint64 begin, uint64 end)
+libpq_destroy(rewind_source *source)
{
- char linebuf[MAXPGPATH + 23];
-
- /* Split the range into CHUNKSIZE chunks */
- while (end - begin > 0)
- {
- unsigned int len;
-
- /* Fine as long as CHUNKSIZE is not bigger than UINT32_MAX */
- if (end - begin > CHUNKSIZE)
- len = CHUNKSIZE;
- else
- len = (unsigned int) (end - begin);
-
- snprintf(linebuf, sizeof(linebuf), "%s\t" UINT64_FORMAT "\t%u\n", path, begin, len);
-
- if (PQputCopyData(conn, linebuf, strlen(linebuf)) != 1)
- pg_fatal("could not send COPY data: %s",
- PQerrorMessage(conn));
-
- begin += len;
- }
-}
-
-/*
- * Fetch all changed blocks from remote source data directory.
- */
-void
-libpq_executeFileMap(filemap_t *map)
-{
- file_entry_t *entry;
- const char *sql;
- PGresult *res;
- int i;
-
- /*
- * First create a temporary table, and load it with the blocks that we
- * need to fetch.
- */
- sql = "CREATE TEMPORARY TABLE fetchchunks(path text, begin int8, len int4);";
- run_simple_command(sql);
-
- sql = "COPY fetchchunks FROM STDIN";
- res = PQexec(conn, sql);
-
- if (PQresultStatus(res) != PGRES_COPY_IN)
- pg_fatal("could not send file list: %s",
- PQresultErrorMessage(res));
- PQclear(res);
-
- for (i = 0; i < map->nactions; i++)
- {
- entry = map->actions[i];
-
- /* If this is a relation file, copy the modified blocks */
- execute_pagemap(&entry->target_modified_pages, entry->path);
-
- switch (entry->action)
- {
- case FILE_ACTION_NONE:
- /* nothing else to do */
- break;
-
- case FILE_ACTION_COPY:
- /* Truncate the old file out of the way, if any */
- open_target_file(entry->path, true);
- fetch_file_range(entry->path, 0, entry->source_size);
- break;
-
- case FILE_ACTION_TRUNCATE:
- truncate_target_file(entry->path, entry->source_size);
- break;
-
- case FILE_ACTION_COPY_TAIL:
- fetch_file_range(entry->path, entry->target_size, entry->source_size);
- break;
-
- case FILE_ACTION_REMOVE:
- remove_target(entry);
- break;
-
- case FILE_ACTION_CREATE:
- create_target(entry);
- break;
-
- case FILE_ACTION_UNDECIDED:
- pg_fatal("no action decided for \"%s\"", entry->path);
- break;
- }
- }
-
- if (PQputCopyEnd(conn, NULL) != 1)
- pg_fatal("could not send end-of-COPY: %s",
- PQerrorMessage(conn));
-
- while ((res = PQgetResult(conn)) != NULL)
- {
- if (PQresultStatus(res) != PGRES_COMMAND_OK)
- pg_fatal("unexpected result while sending file list: %s",
- PQresultErrorMessage(res));
- PQclear(res);
- }
-
- /*
- * We've now copied the list of file ranges that we need to fetch to the
- * temporary table. Now, actually fetch all of those ranges.
- */
- sql =
- "SELECT path, begin,\n"
- " pg_read_binary_file(path, begin, len, true) AS chunk\n"
- "FROM fetchchunks\n";
-
- receiveFileChunks(sql);
-}
-
-static void
-execute_pagemap(datapagemap_t *pagemap, const char *path)
-{
- datapagemap_iterator_t *iter;
- BlockNumber blkno;
- off_t offset;
-
- iter = datapagemap_iterate(pagemap);
- while (datapagemap_next(iter, &blkno))
- {
- offset = blkno * BLCKSZ;
-
- fetch_file_range(path, offset, offset + BLCKSZ);
- }
- pg_free(iter);
+ pfree(source);
+ /* NOTE: we don't close the connection here, as it was not opened by us. */
}
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 2bdeed26c53..9e04a085226 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -35,8 +35,8 @@ static void usage(const char *progname);
static void createBackupLabel(XLogRecPtr startpoint, TimeLineID starttli,
XLogRecPtr checkpointloc);
-static void digestControlFile(ControlFileData *ControlFile, char *source,
- size_t size);
+static void digestControlFile(ControlFileData *ControlFile,
+ const char *content, size_t size);
static void getRestoreCommand(const char *argv0);
static void sanityChecks(void);
static void findCommonAncestorTimeline(XLogRecPtr *recptr, int *tliIndex);
@@ -69,6 +69,8 @@ int targetNentries;
uint64 fetch_size;
uint64 fetch_done;
+static PGconn *conn;
+static rewind_source *source;
static void
usage(const char *progname)
@@ -269,19 +271,29 @@ main(int argc, char **argv)
atexit(disconnect_atexit);
- /* Connect to remote server */
- if (connstr_source)
- libpqConnect(connstr_source);
-
/*
- * Ok, we have all the options and we're ready to start. Read in all the
- * information we need from both clusters.
+ * Ok, we have all the options and we're ready to start. First, connect
+ * to remote server.
*/
- buffer = slurpFile(datadir_target, "global/pg_control", &size);
- digestControlFile(&ControlFile_target, buffer, size);
- pg_free(buffer);
+ if (connstr_source)
+ {
+ conn = PQconnectdb(connstr_source);
+
+ if (PQstatus(conn) == CONNECTION_BAD)
+ pg_fatal("could not connect to server: %s",
+ PQerrorMessage(conn));
+
+ if (showprogress)
+ pg_log_info("connected to server");
+
+ source = init_libpq_source(conn);
+ }
+ else
+ source = init_local_source(datadir_source);
/*
+ * Check the status of the target instance.
+ *
* If the target instance was not cleanly shut down, start and stop the
* target cluster once in single-user mode to enforce recovery to finish,
* ensuring that the cluster can be used by pg_rewind. Note that if
@@ -289,6 +301,10 @@ main(int argc, char **argv)
* need to make sure by themselves that the target cluster is in a clean
* state.
*/
+ buffer = slurpFile(datadir_target, "global/pg_control", &size);
+ digestControlFile(&ControlFile_target, buffer, size);
+ pg_free(buffer);
+
if (!no_ensure_shutdown &&
ControlFile_target.state != DB_SHUTDOWNED &&
ControlFile_target.state != DB_SHUTDOWNED_IN_RECOVERY)
@@ -300,17 +316,20 @@ main(int argc, char **argv)
pg_free(buffer);
}
- buffer = fetchFile("global/pg_control", &size);
+ buffer = source->fetch_file(source, "global/pg_control", &size);
digestControlFile(&ControlFile_source, buffer, size);
pg_free(buffer);
sanityChecks();
/*
+ * Find the common ancestor timeline between the clusters.
+ *
* If both clusters are already on the same timeline, there's nothing to
* do.
*/
- if (ControlFile_target.checkPointCopy.ThisTimeLineID == ControlFile_source.checkPointCopy.ThisTimeLineID)
+ if (ControlFile_target.checkPointCopy.ThisTimeLineID ==
+ ControlFile_source.checkPointCopy.ThisTimeLineID)
{
pg_log_info("source and target cluster are on the same timeline");
rewind_needed = false;
@@ -370,12 +389,12 @@ main(int argc, char **argv)
chkpttli);
/*
- * Collect information about all files in the target and source systems.
+ * Collect information about all files in the both data directories.
*/
if (showprogress)
pg_log_info("reading source file list");
filemap_init();
- fetchSourceFileList();
+ source->traverse_files(source, &process_source_file);
if (showprogress)
pg_log_info("reading target file list");
@@ -423,7 +442,7 @@ main(int argc, char **argv)
* modified the target directory and there is no turning back!
*/
- execute_file_actions(filemap);
+ execute_file_actions(filemap, source);
progress_report(true);
@@ -443,7 +462,7 @@ main(int argc, char **argv)
if (connstr_source)
{
- endrec = libpqGetCurrentXlogInsertLocation();
+ endrec = source->get_current_wal_insert_lsn(source);
endtli = ControlFile_source.checkPointCopy.ThisTimeLineID;
}
else
@@ -465,6 +484,14 @@ main(int argc, char **argv)
WriteRecoveryConfig(conn, datadir_target,
GenerateRecoveryConfig(conn, NULL));
+ /* don't need the source connection anymore */
+ source->destroy(source);
+ if (conn)
+ {
+ PQfinish(conn);
+ conn = NULL;
+ }
+
pg_log_info("Done!");
return 0;
@@ -627,7 +654,7 @@ getTimelineHistory(ControlFileData *controlFile, int *nentries)
/* Get history file from appropriate source */
if (controlFile == &ControlFile_source)
- histfile = fetchFile(path, NULL);
+ histfile = source->fetch_file(source, path, NULL);
else if (controlFile == &ControlFile_target)
histfile = slurpFile(datadir_target, path, NULL);
else
@@ -783,16 +810,17 @@ checkControlFile(ControlFileData *ControlFile)
}
/*
- * Verify control file contents in the buffer src, and copy it to *ControlFile.
+ * Verify control file contents in the buffer 'content', and copy it to *ControlFile.
*/
static void
-digestControlFile(ControlFileData *ControlFile, char *src, size_t size)
+digestControlFile(ControlFileData *ControlFile,
+ const char *content, size_t size)
{
if (size != PG_CONTROL_FILE_SIZE)
pg_fatal("unexpected control file size %d, expected %d",
(int) size, PG_CONTROL_FILE_SIZE);
- memcpy(ControlFile, src, sizeof(ControlFileData));
+ memcpy(ControlFile, content, sizeof(ControlFileData));
/* set and validate WalSegSz */
WalSegSz = ControlFile->xlog_seg_size;
diff --git a/src/bin/pg_rewind/pg_rewind.h b/src/bin/pg_rewind/pg_rewind.h
index 67f90c2a38c..0dc3dbd5255 100644
--- a/src/bin/pg_rewind/pg_rewind.h
+++ b/src/bin/pg_rewind/pg_rewind.h
@@ -20,8 +20,6 @@
/* Configuration options */
extern char *datadir_target;
-extern char *datadir_source;
-extern char *connstr_source;
extern bool showprogress;
extern bool dry_run;
extern bool do_sync;
@@ -31,9 +29,6 @@ extern int WalSegSz;
extern TimeLineHistoryEntry *targetHistory;
extern int targetNentries;
-/* general state */
-extern PGconn *conn;
-
/* Progress counters */
extern uint64 fetch_size;
extern uint64 fetch_done;
--
2.20.1
--------------BF34D0120055DC3839060F92
Content-Type: text/x-patch; charset=UTF-8;
name="v2-0005-Allow-pg_rewind-to-use-a-standby-server-as-the-so.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="v2-0005-Allow-pg_rewind-to-use-a-standby-server-as-the-so.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: suspicious valgrind reports about radixtree/tidstore on arm64
@ 2024-06-19 15:48 Tomas Vondra <[email protected]>
2024-06-19 21:11 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Tomas Vondra @ 2024-06-19 15:48 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; John Naylor <[email protected]>; Masahiko Sawada <[email protected]>
On 6/19/24 17:11, Tom Lane wrote:
> Tomas Vondra <[email protected]> writes:
>> I tried running master under valgrind on 64-bit ARM (rpi5 running debian
>> 12.5), and I got some suspicous reports, all related to the radixtree
>> code used by tidstore.
>
> What's the test scenario that triggers this?
>
I haven't investigated that yet, I just ran "make check".
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: suspicious valgrind reports about radixtree/tidstore on arm64
2024-06-19 15:48 Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tomas Vondra <[email protected]>
@ 2024-06-19 21:11 ` Tom Lane <[email protected]>
2024-06-19 22:54 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2024-06-19 21:11 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Masahiko Sawada <[email protected]>; John Naylor <[email protected]>
Tomas Vondra <[email protected]> writes:
> On 6/19/24 17:11, Tom Lane wrote:
>> What's the test scenario that triggers this?
> I haven't investigated that yet, I just ran "make check".
I've reproduced what looks like about the same thing on
my Pi 4 using Fedora 38: just run "make installcheck-parallel"
under valgrind, and kaboom. Definitely needs investigation.
regards, tom lane
==00:00:04:08.988 16548== Memcheck, a memory error detector
==00:00:04:08.988 16548== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==00:00:04:08.988 16548== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==00:00:04:08.988 16548== Command: postgres -F
==00:00:04:08.988 16548== Parent PID: 16278
==00:00:04:08.988 16548==
==00:00:04:13.280 16548== Conditional jump or move depends on uninitialised value(s)
==00:00:04:13.280 16548== at 0x4AD2CC: local_ts_node_16_search_eq (radixtree.h:1025)
==00:00:04:13.280 16548== by 0x4AD2CC: local_ts_node_search (radixtree.h:1057)
==00:00:04:13.280 16548== by 0x4AF173: local_ts_get_slot_recursive (radixtree.h:1667)
==00:00:04:13.280 16548== by 0x4AF173: local_ts_set (radixtree.h:1744)
==00:00:04:13.280 16548== by 0x4AF173: TidStoreSetBlockOffsets (tidstore.c:427)
==00:00:04:13.280 16548== by 0x4FCEEF: dead_items_add (vacuumlazy.c:2892)
==00:00:04:13.280 16548== by 0x4FE5EF: lazy_scan_prune (vacuumlazy.c:1500)
==00:00:04:13.280 16548== by 0x4FE5EF: lazy_scan_heap (vacuumlazy.c:975)
==00:00:04:13.280 16548== by 0x4FE5EF: heap_vacuum_rel (vacuumlazy.c:497)
==00:00:04:13.280 16548== by 0x681527: table_relation_vacuum (tableam.h:1720)
==00:00:04:13.280 16548== by 0x681527: vacuum_rel (vacuum.c:2210)
==00:00:04:13.281 16548== by 0x682957: vacuum (vacuum.c:622)
==00:00:04:13.281 16548== by 0x7B7E77: autovacuum_do_vac_analyze (autovacuum.c:3100)
==00:00:04:13.281 16548== by 0x7B7E77: do_autovacuum (autovacuum.c:2417)
==00:00:04:13.281 16548== by 0x7B830B: AutoVacWorkerMain (autovacuum.c:1569)
==00:00:04:13.281 16548== by 0x7BB937: postmaster_child_launch (launch_backend.c:265)
==00:00:04:13.281 16548== by 0x7BD64F: StartChildProcess (postmaster.c:3928)
==00:00:04:13.281 16548== by 0x7BF9FB: StartAutovacuumWorker (postmaster.c:3997)
==00:00:04:13.281 16548== by 0x7BF9FB: process_pm_pmsignal (postmaster.c:3809)
==00:00:04:13.281 16548== by 0x7BF9FB: ServerLoop.isra.0 (postmaster.c:1667)
==00:00:04:13.281 16548== by 0x7C0EBF: PostmasterMain (postmaster.c:1372)
==00:00:04:13.281 16548==
==00:00:04:13.673 16548== Conditional jump or move depends on uninitialised value(s)
==00:00:04:13.673 16548== at 0x4AD2CC: local_ts_node_16_search_eq (radixtree.h:1025)
==00:00:04:13.673 16548== by 0x4AD2CC: local_ts_node_search (radixtree.h:1057)
==00:00:04:13.673 16548== by 0x4AFD4F: local_ts_find (radixtree.h:1111)
==00:00:04:13.673 16548== by 0x4AFD4F: TidStoreIsMember (tidstore.c:443)
==00:00:04:13.673 16548== by 0x5110D7: btvacuumpage (nbtree.c:1235)
==00:00:04:13.673 16548== by 0x51171B: btvacuumscan (nbtree.c:1023)
==00:00:04:13.673 16548== by 0x51185B: btbulkdelete (nbtree.c:824)
==00:00:04:13.673 16548== by 0x683D3B: vac_bulkdel_one_index (vacuum.c:2498)
==00:00:04:13.673 16548== by 0x4FDA9B: lazy_vacuum_one_index (vacuumlazy.c:2443)
==00:00:04:13.673 16548== by 0x4FDA9B: lazy_vacuum_all_indexes (vacuumlazy.c:2026)
==00:00:04:13.673 16548== by 0x4FDA9B: lazy_vacuum (vacuumlazy.c:1944)
==00:00:04:13.673 16548== by 0x4FE7F3: lazy_scan_heap (vacuumlazy.c:1050)
==00:00:04:13.674 16548== by 0x4FE7F3: heap_vacuum_rel (vacuumlazy.c:497)
==00:00:04:13.674 16548== by 0x681527: table_relation_vacuum (tableam.h:1720)
==00:00:04:13.674 16548== by 0x681527: vacuum_rel (vacuum.c:2210)
==00:00:04:13.674 16548== by 0x682957: vacuum (vacuum.c:622)
==00:00:04:13.674 16548== by 0x7B7E77: autovacuum_do_vac_analyze (autovacuum.c:3100)
==00:00:04:13.674 16548== by 0x7B7E77: do_autovacuum (autovacuum.c:2417)
==00:00:04:13.674 16548== by 0x7B830B: AutoVacWorkerMain (autovacuum.c:1569)
==00:00:04:13.674 16548==
==00:00:04:13.681 16548== Conditional jump or move depends on uninitialised value(s)
==00:00:04:13.681 16548== at 0x4AD2CC: local_ts_node_16_search_eq (radixtree.h:1025)
==00:00:04:13.681 16548== by 0x4AD2CC: local_ts_node_search (radixtree.h:1057)
==00:00:04:13.681 16548== by 0x4AFD4F: local_ts_find (radixtree.h:1111)
==00:00:04:13.681 16548== by 0x4AFD4F: TidStoreIsMember (tidstore.c:443)
==00:00:04:13.681 16548== by 0x51126B: btreevacuumposting (nbtree.c:1405)
==00:00:04:13.681 16548== by 0x51126B: btvacuumpage (nbtree.c:1249)
==00:00:04:13.681 16548== by 0x51171B: btvacuumscan (nbtree.c:1023)
==00:00:04:13.681 16548== by 0x51185B: btbulkdelete (nbtree.c:824)
==00:00:04:13.681 16548== by 0x683D3B: vac_bulkdel_one_index (vacuum.c:2498)
==00:00:04:13.681 16548== by 0x4FDA9B: lazy_vacuum_one_index (vacuumlazy.c:2443)
==00:00:04:13.681 16548== by 0x4FDA9B: lazy_vacuum_all_indexes (vacuumlazy.c:2026)
==00:00:04:13.681 16548== by 0x4FDA9B: lazy_vacuum (vacuumlazy.c:1944)
==00:00:04:13.681 16548== by 0x4FE7F3: lazy_scan_heap (vacuumlazy.c:1050)
==00:00:04:13.681 16548== by 0x4FE7F3: heap_vacuum_rel (vacuumlazy.c:497)
==00:00:04:13.681 16548== by 0x681527: table_relation_vacuum (tableam.h:1720)
==00:00:04:13.681 16548== by 0x681527: vacuum_rel (vacuum.c:2210)
==00:00:04:13.681 16548== by 0x682957: vacuum (vacuum.c:622)
==00:00:04:13.681 16548== by 0x7B7E77: autovacuum_do_vac_analyze (autovacuum.c:3100)
==00:00:04:13.681 16548== by 0x7B7E77: do_autovacuum (autovacuum.c:2417)
==00:00:04:13.681 16548== by 0x7B830B: AutoVacWorkerMain (autovacuum.c:1569)
==00:00:04:13.681 16548==
==00:00:04:35.444 16548==
==00:00:04:35.445 16548== HEAP SUMMARY:
==00:00:04:35.445 16548== in use at exit: 1,936,233 bytes in 285 blocks
==00:00:04:35.445 16548== total heap usage: 59,846 allocs, 1,316 frees, 84,811,772 bytes allocated
==00:00:04:35.445 16548==
==00:00:04:37.660 16548== LEAK SUMMARY:
==00:00:04:37.660 16548== definitely lost: 31,979 bytes in 234 blocks
==00:00:04:37.660 16548== indirectly lost: 14,436 bytes in 48 blocks
==00:00:04:37.660 16548== possibly lost: 371,045 bytes in 1,075 blocks
==00:00:04:37.660 16548== still reachable: 586,591 bytes in 1,410 blocks
==00:00:04:37.660 16548== suppressed: 0 bytes in 0 blocks
==00:00:04:37.660 16548== Rerun with --leak-check=full to see details of leaked memory
==00:00:04:37.660 16548==
==00:00:04:37.660 16548== Use --track-origins=yes to see where uninitialised values come from
==00:00:04:37.660 16548== For lists of detected and suppressed errors, rerun with: -s
==00:00:04:37.660 16548== ERROR SUMMARY: 7781 errors from 3 contexts (suppressed: 0 from 0)
Attachments:
[text/plain] 16548.log (6.3K, ../../[email protected]/2-16548.log)
download | inline:
==00:00:04:08.988 16548== Memcheck, a memory error detector
==00:00:04:08.988 16548== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==00:00:04:08.988 16548== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==00:00:04:08.988 16548== Command: postgres -F
==00:00:04:08.988 16548== Parent PID: 16278
==00:00:04:08.988 16548==
==00:00:04:13.280 16548== Conditional jump or move depends on uninitialised value(s)
==00:00:04:13.280 16548== at 0x4AD2CC: local_ts_node_16_search_eq (radixtree.h:1025)
==00:00:04:13.280 16548== by 0x4AD2CC: local_ts_node_search (radixtree.h:1057)
==00:00:04:13.280 16548== by 0x4AF173: local_ts_get_slot_recursive (radixtree.h:1667)
==00:00:04:13.280 16548== by 0x4AF173: local_ts_set (radixtree.h:1744)
==00:00:04:13.280 16548== by 0x4AF173: TidStoreSetBlockOffsets (tidstore.c:427)
==00:00:04:13.280 16548== by 0x4FCEEF: dead_items_add (vacuumlazy.c:2892)
==00:00:04:13.280 16548== by 0x4FE5EF: lazy_scan_prune (vacuumlazy.c:1500)
==00:00:04:13.280 16548== by 0x4FE5EF: lazy_scan_heap (vacuumlazy.c:975)
==00:00:04:13.280 16548== by 0x4FE5EF: heap_vacuum_rel (vacuumlazy.c:497)
==00:00:04:13.280 16548== by 0x681527: table_relation_vacuum (tableam.h:1720)
==00:00:04:13.280 16548== by 0x681527: vacuum_rel (vacuum.c:2210)
==00:00:04:13.281 16548== by 0x682957: vacuum (vacuum.c:622)
==00:00:04:13.281 16548== by 0x7B7E77: autovacuum_do_vac_analyze (autovacuum.c:3100)
==00:00:04:13.281 16548== by 0x7B7E77: do_autovacuum (autovacuum.c:2417)
==00:00:04:13.281 16548== by 0x7B830B: AutoVacWorkerMain (autovacuum.c:1569)
==00:00:04:13.281 16548== by 0x7BB937: postmaster_child_launch (launch_backend.c:265)
==00:00:04:13.281 16548== by 0x7BD64F: StartChildProcess (postmaster.c:3928)
==00:00:04:13.281 16548== by 0x7BF9FB: StartAutovacuumWorker (postmaster.c:3997)
==00:00:04:13.281 16548== by 0x7BF9FB: process_pm_pmsignal (postmaster.c:3809)
==00:00:04:13.281 16548== by 0x7BF9FB: ServerLoop.isra.0 (postmaster.c:1667)
==00:00:04:13.281 16548== by 0x7C0EBF: PostmasterMain (postmaster.c:1372)
==00:00:04:13.281 16548==
==00:00:04:13.673 16548== Conditional jump or move depends on uninitialised value(s)
==00:00:04:13.673 16548== at 0x4AD2CC: local_ts_node_16_search_eq (radixtree.h:1025)
==00:00:04:13.673 16548== by 0x4AD2CC: local_ts_node_search (radixtree.h:1057)
==00:00:04:13.673 16548== by 0x4AFD4F: local_ts_find (radixtree.h:1111)
==00:00:04:13.673 16548== by 0x4AFD4F: TidStoreIsMember (tidstore.c:443)
==00:00:04:13.673 16548== by 0x5110D7: btvacuumpage (nbtree.c:1235)
==00:00:04:13.673 16548== by 0x51171B: btvacuumscan (nbtree.c:1023)
==00:00:04:13.673 16548== by 0x51185B: btbulkdelete (nbtree.c:824)
==00:00:04:13.673 16548== by 0x683D3B: vac_bulkdel_one_index (vacuum.c:2498)
==00:00:04:13.673 16548== by 0x4FDA9B: lazy_vacuum_one_index (vacuumlazy.c:2443)
==00:00:04:13.673 16548== by 0x4FDA9B: lazy_vacuum_all_indexes (vacuumlazy.c:2026)
==00:00:04:13.673 16548== by 0x4FDA9B: lazy_vacuum (vacuumlazy.c:1944)
==00:00:04:13.673 16548== by 0x4FE7F3: lazy_scan_heap (vacuumlazy.c:1050)
==00:00:04:13.674 16548== by 0x4FE7F3: heap_vacuum_rel (vacuumlazy.c:497)
==00:00:04:13.674 16548== by 0x681527: table_relation_vacuum (tableam.h:1720)
==00:00:04:13.674 16548== by 0x681527: vacuum_rel (vacuum.c:2210)
==00:00:04:13.674 16548== by 0x682957: vacuum (vacuum.c:622)
==00:00:04:13.674 16548== by 0x7B7E77: autovacuum_do_vac_analyze (autovacuum.c:3100)
==00:00:04:13.674 16548== by 0x7B7E77: do_autovacuum (autovacuum.c:2417)
==00:00:04:13.674 16548== by 0x7B830B: AutoVacWorkerMain (autovacuum.c:1569)
==00:00:04:13.674 16548==
==00:00:04:13.681 16548== Conditional jump or move depends on uninitialised value(s)
==00:00:04:13.681 16548== at 0x4AD2CC: local_ts_node_16_search_eq (radixtree.h:1025)
==00:00:04:13.681 16548== by 0x4AD2CC: local_ts_node_search (radixtree.h:1057)
==00:00:04:13.681 16548== by 0x4AFD4F: local_ts_find (radixtree.h:1111)
==00:00:04:13.681 16548== by 0x4AFD4F: TidStoreIsMember (tidstore.c:443)
==00:00:04:13.681 16548== by 0x51126B: btreevacuumposting (nbtree.c:1405)
==00:00:04:13.681 16548== by 0x51126B: btvacuumpage (nbtree.c:1249)
==00:00:04:13.681 16548== by 0x51171B: btvacuumscan (nbtree.c:1023)
==00:00:04:13.681 16548== by 0x51185B: btbulkdelete (nbtree.c:824)
==00:00:04:13.681 16548== by 0x683D3B: vac_bulkdel_one_index (vacuum.c:2498)
==00:00:04:13.681 16548== by 0x4FDA9B: lazy_vacuum_one_index (vacuumlazy.c:2443)
==00:00:04:13.681 16548== by 0x4FDA9B: lazy_vacuum_all_indexes (vacuumlazy.c:2026)
==00:00:04:13.681 16548== by 0x4FDA9B: lazy_vacuum (vacuumlazy.c:1944)
==00:00:04:13.681 16548== by 0x4FE7F3: lazy_scan_heap (vacuumlazy.c:1050)
==00:00:04:13.681 16548== by 0x4FE7F3: heap_vacuum_rel (vacuumlazy.c:497)
==00:00:04:13.681 16548== by 0x681527: table_relation_vacuum (tableam.h:1720)
==00:00:04:13.681 16548== by 0x681527: vacuum_rel (vacuum.c:2210)
==00:00:04:13.681 16548== by 0x682957: vacuum (vacuum.c:622)
==00:00:04:13.681 16548== by 0x7B7E77: autovacuum_do_vac_analyze (autovacuum.c:3100)
==00:00:04:13.681 16548== by 0x7B7E77: do_autovacuum (autovacuum.c:2417)
==00:00:04:13.681 16548== by 0x7B830B: AutoVacWorkerMain (autovacuum.c:1569)
==00:00:04:13.681 16548==
==00:00:04:35.444 16548==
==00:00:04:35.445 16548== HEAP SUMMARY:
==00:00:04:35.445 16548== in use at exit: 1,936,233 bytes in 285 blocks
==00:00:04:35.445 16548== total heap usage: 59,846 allocs, 1,316 frees, 84,811,772 bytes allocated
==00:00:04:35.445 16548==
==00:00:04:37.660 16548== LEAK SUMMARY:
==00:00:04:37.660 16548== definitely lost: 31,979 bytes in 234 blocks
==00:00:04:37.660 16548== indirectly lost: 14,436 bytes in 48 blocks
==00:00:04:37.660 16548== possibly lost: 371,045 bytes in 1,075 blocks
==00:00:04:37.660 16548== still reachable: 586,591 bytes in 1,410 blocks
==00:00:04:37.660 16548== suppressed: 0 bytes in 0 blocks
==00:00:04:37.660 16548== Rerun with --leak-check=full to see details of leaked memory
==00:00:04:37.660 16548==
==00:00:04:37.660 16548== Use --track-origins=yes to see where uninitialised values come from
==00:00:04:37.660 16548== For lists of detected and suppressed errors, rerun with: -s
==00:00:04:37.660 16548== ERROR SUMMARY: 7781 errors from 3 contexts (suppressed: 0 from 0)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: suspicious valgrind reports about radixtree/tidstore on arm64
2024-06-19 15:48 Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tomas Vondra <[email protected]>
2024-06-19 21:11 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
@ 2024-06-19 22:54 ` Tom Lane <[email protected]>
2024-06-19 23:51 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2024-06-19 22:54 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Masahiko Sawada <[email protected]>; John Naylor <[email protected]>
I wrote:
> I've reproduced what looks like about the same thing on
> my Pi 4 using Fedora 38: just run "make installcheck-parallel"
> under valgrind, and kaboom. Definitely needs investigation.
The problem appears to be that RT_ALLOC_NODE doesn't bother to
initialize the chunks[] array when making a RT_NODE_16 node.
If we fill fewer than RT_FANOUT_16_MAX of the chunks[] entries,
then when RT_NODE_16_SEARCH_EQ applies vector operations that
read the entire array, it's operating partially on uninitialized
data. Now, that's actually OK because of the "mask off invalid
entries" step, but aarch64 valgrind complains anyway.
I hypothesize that the reason we're not seeing equivalent failures
on x86_64 is one of
1. x86_64 valgrind is stupider than aarch64, and fails to track that
the contents of the SIMD registers are only partially defined.
2. x86_64 valgrind is smarter than aarch64, and is able to see
that the "mask off invalid entries" step removes all the
potentially-uninitialized bits.
The first attached patch, "radixtree-fix-minimal.patch", is enough
to stop the aarch64 valgrind failure for me. However, I think
that the coding here is pretty penny-wise and pound-foolish,
and that what we really ought to do is the second patch,
"radixtree-fix-proposed.patch". I do not believe that asking
memset to zero the three-byte RT_NODE structure produces code
that's either shorter or faster than having it zero 8 bytes
(as for RT_NODE_4) or having it do that and then separately
zero some more stuff (as for the larger node types). Leaving
RT_NODE_4's chunks[] array uninitialized is going to bite us
someday, too, even if it doesn't right now. So I think we
ought to just zero the whole fixed-size part of the nodes,
which is what radixtree-fix-proposed.patch does.
(The RT_NODE_48 case could be optimized a bit if we cared
to swap the order of its slot_idxs[] and isset[] arrays;
then the initial zeroing could just go up to slot_idxs[].
I don't know if there's any reason why the current order
is preferable.)
regards, tom lane
Attachments:
[text/x-diff] radixtree-fix-minimal.patch (577B, ../../[email protected]/2-radixtree-fix-minimal.patch)
download | inline diff:
diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index 338e1d741d..e21f7be3f9 100644
--- a/src/include/lib/radixtree.h
+++ b/src/include/lib/radixtree.h
@@ -849,8 +849,14 @@ RT_ALLOC_NODE(RT_RADIX_TREE * tree, const uint8 kind, const RT_SIZE_CLASS size_c
switch (kind)
{
case RT_NODE_KIND_4:
- case RT_NODE_KIND_16:
break;
+ case RT_NODE_KIND_16:
+ {
+ RT_NODE_16 *n16 = (RT_NODE_16 *) node;
+
+ memset(n16->chunks, 0, sizeof(n16->chunks));
+ break;
+ }
case RT_NODE_KIND_48:
{
RT_NODE_48 *n48 = (RT_NODE_48 *) node;
[text/x-diff] radixtree-fix-proposed.patch (1.0K, ../../[email protected]/3-radixtree-fix-proposed.patch)
download | inline diff:
diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index 338e1d741d..4510f7c4cd 100644
--- a/src/include/lib/radixtree.h
+++ b/src/include/lib/radixtree.h
@@ -845,27 +845,25 @@ RT_ALLOC_NODE(RT_RADIX_TREE * tree, const uint8 kind, const RT_SIZE_CLASS size_c
/* initialize contents */
- memset(node, 0, sizeof(RT_NODE));
switch (kind)
{
case RT_NODE_KIND_4:
+ memset(node, 0, offsetof(RT_NODE_4, children));
+ break;
case RT_NODE_KIND_16:
+ memset(node, 0, offsetof(RT_NODE_16, children));
break;
case RT_NODE_KIND_48:
{
RT_NODE_48 *n48 = (RT_NODE_48 *) node;
- memset(n48->isset, 0, sizeof(n48->isset));
+ memset(n48, 0, offsetof(RT_NODE_48, children));
memset(n48->slot_idxs, RT_INVALID_SLOT_IDX, sizeof(n48->slot_idxs));
break;
}
case RT_NODE_KIND_256:
- {
- RT_NODE_256 *n256 = (RT_NODE_256 *) node;
-
- memset(n256->isset, 0, sizeof(n256->isset));
- break;
- }
+ memset(node, 0, offsetof(RT_NODE_256, children));
+ break;
default:
pg_unreachable();
}
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: suspicious valgrind reports about radixtree/tidstore on arm64
2024-06-19 15:48 Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tomas Vondra <[email protected]>
2024-06-19 21:11 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
2024-06-19 22:54 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
@ 2024-06-19 23:51 ` Tom Lane <[email protected]>
2024-06-20 11:50 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Ranier Vilela <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2024-06-19 23:51 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Masahiko Sawada <[email protected]>; John Naylor <[email protected]>
I wrote:
> I hypothesize that the reason we're not seeing equivalent failures
> on x86_64 is one of
> 1. x86_64 valgrind is stupider than aarch64, and fails to track that
> the contents of the SIMD registers are only partially defined.
> 2. x86_64 valgrind is smarter than aarch64, and is able to see
> that the "mask off invalid entries" step removes all the
> potentially-uninitialized bits.
Hah: it's the second case. If I patch radixtree.h as attached,
then x86_64 valgrind complains about
==00:00:00:32.759 247596== Conditional jump or move depends on uninitialised value(s)
==00:00:00:32.759 247596== at 0x52F668: local_ts_node_16_search_eq (radixtree.h:1018)
showing that it knows that the result of vector8_highbit_mask is
only partly defined. Kind of odd though that aarch64 valgrind
is getting the hard part right and not the easy(?) part.
regards, tom lane
Attachments:
[text/x-diff] make-radixtree-failure-visible-on-x86_64.patch (900B, ../../[email protected]/2-make-radixtree-failure-visible-on-x86_64.patch)
download | inline diff:
diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index 338e1d741d..267ec6de03 100644
--- a/src/include/lib/radixtree.h
+++ b/src/include/lib/radixtree.h
@@ -1015,12 +1015,15 @@ RT_NODE_16_SEARCH_EQ(RT_NODE_16 * node, uint8 chunk)
/* convert comparison to a bitfield */
bitfield = vector8_highbit_mask(cmp1) | (vector8_highbit_mask(cmp2) << sizeof(Vector8));
- /* mask off invalid entries */
- bitfield &= ((UINT64CONST(1) << count) - 1);
-
- /* convert bitfield to index by counting trailing zeros */
if (bitfield)
- slot_simd = &node->children[pg_rightmost_one_pos32(bitfield)];
+ {
+ /* mask off invalid entries */
+ bitfield &= ((UINT64CONST(1) << count) - 1);
+
+ /* convert bitfield to index by counting trailing zeros */
+ if (bitfield)
+ slot_simd = &node->children[pg_rightmost_one_pos32(bitfield)];
+ }
Assert(slot_simd == slot);
return slot_simd;
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: suspicious valgrind reports about radixtree/tidstore on arm64
2024-06-19 15:48 Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tomas Vondra <[email protected]>
2024-06-19 21:11 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
2024-06-19 22:54 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
2024-06-19 23:51 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
@ 2024-06-20 11:50 ` Ranier Vilela <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Ranier Vilela @ 2024-06-20 11:50 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Tomas Vondra <[email protected]>; PostgreSQL Hackers <[email protected]>; Masahiko Sawada <[email protected]>; John Naylor <[email protected]>
Em qua., 19 de jun. de 2024 às 20:52, Tom Lane <[email protected]> escreveu:
> I wrote:
> > I hypothesize that the reason we're not seeing equivalent failures
> > on x86_64 is one of
>
> > 1. x86_64 valgrind is stupider than aarch64, and fails to track that
> > the contents of the SIMD registers are only partially defined.
>
> > 2. x86_64 valgrind is smarter than aarch64, and is able to see
> > that the "mask off invalid entries" step removes all the
> > potentially-uninitialized bits.
>
> Hah: it's the second case. If I patch radixtree.h as attached,
> then x86_64 valgrind complains about
>
> ==00:00:00:32.759 247596== Conditional jump or move depends on
> uninitialised value(s)
> ==00:00:00:32.759 247596== at 0x52F668: local_ts_node_16_search_eq
> (radixtree.h:1018)
>
> showing that it knows that the result of vector8_highbit_mask is
> only partly defined.
I wouldn't be surprised if *RT_NODE_16_GET_INSERTPOS*
(src/include/lib/radixtree.h),
does not suffer from the same problem?
Even with Assert trying to protect.
Does the fix not apply here too?
best regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-06-20 11:50 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-19 12:34 [PATCH v2 4/5] pg_rewind: Refactor the abstraction to fetch from local/libpq source. Heikki Linnakangas <[email protected]>
2024-06-19 15:48 Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tomas Vondra <[email protected]>
2024-06-19 21:11 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
2024-06-19 22:54 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
2024-06-19 23:51 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Tom Lane <[email protected]>
2024-06-20 11:50 ` Re: suspicious valgrind reports about radixtree/tidstore on arm64 Ranier Vilela <[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