pg_rewind: Skip log directory for file type check like pg_wal
Hello hackers,
I think we should extend the "log" directory the same courtesy as was
done for pg_wal (pg_xlog) in 0e42397f42b.
Today, even if BOTH source and target servers have symlinked "log"
directories, pg_rewind fails with:
file "log" is of different type in source and target.
Attached is a repro patch using the 004_pg_xlog_symlink.pl test to
demonstrate the failure.
Running make check PROVE_TESTS='t/004_pg_xlog_symlink.pl'
in src/bin/pg_rewind should suffice after applying.
This is because when we use the libpq query to fetch the filemap from
the source server, we consider the log directory as a directory, even if
it is a symlink. This is because pg_stat_file() is used in that query in
libpq_traverse_files() and pg_stat_file() returns isdir=t for symlinks
to directories.
This shortcoming is somewhat called out:
* XXX: There is no backend function to get a symbolic link's target in
* general, so if the admin has put any custom symbolic links in the data
* directory, they won't be copied correctly.
We could fix the query and/or pg_stat_file(). However, we would also
like to support deployments where only one of the primaries and/or
standbys have the symlink. That is not hard to conceive, given primaries
and standbys can have drastically disparate log volume and/or log
collection requirements.
Attached is a patch that treats "log" like we treat "pg_wal".
Regards,
Soumyadeep (VMware)
Attachments:
v1-0001-Fix-pg_rewind-when-log-is-a-symlink.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-pg_rewind-when-log-is-a-symlink.patchDownload
From 697414d2b630efdad0a9137ea9cc93f8576a9792 Mon Sep 17 00:00:00 2001
From: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>
Date: Sun, 5 Mar 2023 17:57:55 -0800
Subject: [PATCH v1 1/1] Fix pg_rewind when log is a symlink
The log directory can often be symlinked in the same way the pg_wal
directory is. Today, even if BOTH the source and target servers have
their log directories symlinked, pg_rewind will run into the error:
"log" is of different type in source and target
This is because when we use the libpq query to fetch the filemap from
the source server, we consider the log directory as a directory, even if
it is a symlink. This is because pg_stat_file() is used in that query in
libpq_traverse_files() and pg_stat_file() returns isdir=t for symlinks
to directories.
This shortcoming is somewhat called out:
* XXX: There is no backend function to get a symbolic link's target in
* general, so if the admin has put any custom symbolic links in the data
* directory, they won't be copied correctly.
We could fix the query and/or pg_stat_file(). However, we would also
like to support deployments where only one of the primaries and/or
standbys have the symlink. That is not hard to conceive, given primaries
and standbys can have drastically disparate log volume and/or log
collection requirements.
So, we decide to extend the log directory the same courtesy as was done
for pg_wal (pg_xlog) in 0e42397f42b.
---
src/bin/pg_rewind/filemap.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index bd5c598e200..a076bb33996 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -221,11 +221,11 @@ process_source_file(const char *path, file_type_t type, size_t size,
file_entry_t *entry;
/*
- * Pretend that pg_wal is a directory, even if it's really a symlink. We
+ * Pretend that pg_wal/log is a directory, even if it's really a symlink. We
* don't want to mess with the symlink itself, nor complain if it's a
* symlink in source but not in target or vice versa.
*/
- if (strcmp(path, "pg_wal") == 0 && type == FILE_TYPE_SYMLINK)
+ if (((strcmp(path, "pg_wal") == 0 || strcmp(path, "log") == 0)) && type == FILE_TYPE_SYMLINK)
type = FILE_TYPE_DIRECTORY;
/*
@@ -263,9 +263,9 @@ process_target_file(const char *path, file_type_t type, size_t size,
*/
/*
- * Like in process_source_file, pretend that pg_wal is always a directory.
+ * Like in process_source_file, pretend that pg_wal/log is always a directory.
*/
- if (strcmp(path, "pg_wal") == 0 && type == FILE_TYPE_SYMLINK)
+ if (((strcmp(path, "pg_wal") == 0 || strcmp(path, "log") == 0)) && type == FILE_TYPE_SYMLINK)
type = FILE_TYPE_DIRECTORY;
/* Remember this target file */
--
2.34.1
repro.patchtext/x-patch; charset=US-ASCII; name=repro.patchDownload
diff --git a/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl b/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl
index 5fb7fa9077c..f95ba1a1486 100644
--- a/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl
+++ b/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl
@@ -2,7 +2,7 @@
# Copyright (c) 2021-2023, PostgreSQL Global Development Group
#
-# Test pg_rewind when the target's pg_wal directory is a symlink.
+# Test pg_rewind when the target's log directory is a symlink.
#
use strict;
use warnings;
@@ -27,11 +27,12 @@ sub run_test
RewindTest::setup_cluster($test_mode);
my $test_primary_datadir = $node_primary->data_dir;
+ mkdir("$test_primary_datadir/log") or die;
# turn pg_wal into a symlink
- print("moving $test_primary_datadir/pg_wal to $primary_xlogdir\n");
- move("$test_primary_datadir/pg_wal", $primary_xlogdir) or die;
- dir_symlink($primary_xlogdir, "$test_primary_datadir/pg_wal") or die;
+ print("moving $test_primary_datadir/log to $primary_xlogdir\n");
+ move("$test_primary_datadir/log", $primary_xlogdir) or die;
+ dir_symlink($primary_xlogdir, "$test_primary_datadir/log") or die;
RewindTest::start_primary();
@@ -43,6 +44,16 @@ sub run_test
RewindTest::create_standby($test_mode);
+ my $test_standby_datadir = $node_standby->data_dir;
+ mkdir("$test_standby_datadir/log") or die;
+
+ my $standby_xlogdir =
+ "${PostgreSQL::Test::Utils::tmp_check}/xlog_standby";
+ print("moving $test_standby_datadir/log to $standby_xlogdir\n");
+ move("$test_standby_datadir/log", $standby_xlogdir) or die;
+ dir_symlink($standby_xlogdir, "$test_standby_datadir/log") or die;
+
+
# Insert additional data on primary that will be replicated to standby
primary_psql("INSERT INTO tbl1 values ('in primary, before promotion')");
Hello Soumyadeep,
The problem indeed exists, but IMO the "log" directory case must be handled
differently:
1. We don't need or I would even say we don't want to sync log files from
the new primary, because it destroys the actual logs, which could be very
important to figure out what has happened with the old primary
2. Unlike "pg_wal", the "log" directory is not necessarily located inside
PGDATA. The actual value is configured using "log_directory" GUC, which
just happened to be "log" by default. And in fact actual values on source
and target could be different.
Regards,
--
Alexander Kukushkin
On Mon, Mar 6, 2023 at 12:28 AM Alexander Kukushkin <cyberdemn@gmail.com> wrote:
Hello Soumyadeep,
The problem indeed exists, but IMO the "log" directory case must be handled differently:
1. We don't need or I would even say we don't want to sync log files from the new primary, because it destroys the actual logs, which could be very important to figure out what has happened with the old primary
Yes, this can be solved by adding "log" to excludeDirContents. We did
this for GPDB.
2. Unlike "pg_wal", the "log" directory is not necessarily located inside PGDATA. The actual value is configured using "log_directory" GUC, which just happened to be "log" by default. And in fact actual values on source and target could be different.
I think we only care about files/dirs inside the datadir. Anything
outside is out of scope for
pg_rewind AFAIU. We can only address the common case here. As mentioned in this
comment:
* XXX: There is no backend function to get a symbolic link's target in
* general, so if the admin has put any custom symbolic links in the data
* directory, they won't be copied correctly.
There is not much we can do about custom configurations.
Regards,
Soumyadeep (VMware)
On Mon, 6 Mar 2023 at 19:37, Soumyadeep Chakraborty <
soumyadeep2007@gmail.com> wrote:
2. Unlike "pg_wal", the "log" directory is not necessarily located
inside PGDATA. The actual value is configured using "log_directory" GUC,
which just happened to be "log" by default. And in fact actual values on
source and target could be different.I think we only care about files/dirs inside the datadir. Anything
outside is out of scope for
pg_rewind AFAIU. We can only address the common case here. As mentioned in
this
comment:* XXX: There is no backend function to get a symbolic link's target in
* general, so if the admin has put any custom symbolic links in the data
* directory, they won't be copied correctly.
That's exactly my point. Users are very creative.
On one node they could set log_directory to for example "pg_log" and on
another one "my_log".
And they would be writing logs to $PGDATA/pg_log and $PGDATA/my_log
respectively and they are both located inside datadir.
Lets assume that on the source we have "pg_log" and on the target we have
"my_log" (they are configured using "log_directory" GUC).
When doing rewind in this case we want neither to remove the content of
"my_log" on the target nor to copy content of "pg_log" from the source.
It couldn't be achieved just by introducing a static string "log". The
"log_directory" GUC must be examined on both, source and target.
Regards,
--
Alexander Kukushkin
On Mon, Mar 6, 2023 at 11:33 PM Alexander Kukushkin <cyberdemn@gmail.com> wrote:
Lets assume that on the source we have "pg_log" and on the target we have "my_log" (they are configured using "log_directory" GUC).
When doing rewind in this case we want neither to remove the content of "my_log" on the target nor to copy content of "pg_log" from the source.
It couldn't be achieved just by introducing a static string "log". The "log_directory" GUC must be examined on both, source and target.
Trouble with doing that is if pg_rewind is run in non-libpq (offline)
mode. Then we would have to parse it out of the conf file(s)?
Is there a standard way of doing that?
Regards,
Soumyadeep (VMware)
On Tue, 7 Mar 2023 at 08:52, Soumyadeep Chakraborty <
soumyadeep2007@gmail.com> wrote:
It couldn't be achieved just by introducing a static string "log". The
"log_directory" GUC must be examined on both, source and target.
Trouble with doing that is if pg_rewind is run in non-libpq (offline)
mode. Then we would have to parse it out of the conf file(s)?
Is there a standard way of doing that?
pg_rewind is already doing something similar for "restore_command":
/*
* Get value of GUC parameter restore_command from the target cluster.
*
* This uses a logic based on "postgres -C" to get the value from the
* cluster.
*/
static void
getRestoreCommand(const char *argv0)
For the running source cluster one could just use "SHOW log_directory"
Regards,
--
Alexander Kukushkin
On 7 Mar 2023, at 08:33, Alexander Kukushkin <cyberdemn@gmail.com> wrote:
The "log_directory" GUC must be examined on both, source and target.
Agreed, log_directory must be resolved to the configured values. Teaching
pg_rewind about those in case they are stored in $PGDATA sounds like a good
idea though.
--
Daniel Gustafsson