While hacking on pg_rewind, I noticed that commit and abort WAL records
are never marked with the XLR_SPECIAL_REL_UPDATE flag. But if the record
contains "dropped relfilenodes", surely it should be?
It's harmless as far as the backend and all the programs in PostgreSQL
repository are concerned, but the point of XLR_SPECIAL_REL_UPDATE is to
aid external tools that try to track which files are modified. Attached
is a patch to fix it.
It's always been like that, but I am not going backport, for fear of
breaking existing applications. If a program reads the WAL, and would
actually need to do something with commit records dropping relations,
that seems like such a common scenario that the author should've thought
about it and handled it even without the flag reminding about it. Fixing
it in master ought to be enough.
Thoughts?
- Heikki
>From cafc42a1dfe6afc9d36c0440dd45a5b12d9a6063 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 12 Aug 2020 23:07:50 +0300
Subject: [PATCH 1/2] Make xact.h usable in frontend.
xact.h included utils/datetime.h, which cannot be used in the frontend
(it includes fmgr.h, which needs Datum). But xact.h only needs the
definition of TimestampTz from it, which is available directly in
datatypes/timestamp.h. Change xact.h to include that instead of
utils/datetime.h, so that it can be used in client programs.
---
src/backend/nodes/params.c | 1 +
src/backend/utils/time/snapmgr.c | 2 ++
src/include/access/xact.h | 2 +-
3 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/backend/nodes/params.c b/src/backend/nodes/params.c
index 1719119fc28..bce0c7e72b2 100644
--- a/src/backend/nodes/params.c
+++ b/src/backend/nodes/params.c
@@ -16,6 +16,7 @@
#include "postgres.h"
#include "access/xact.h"
+#include "fmgr.h"
#include "mb/stringinfo_mb.h"
#include "nodes/params.h"
#include "parser/parse_node.h"
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 6b6c8571e23..02b51bc4fe0 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -53,6 +53,7 @@
#include "access/xact.h"
#include "access/xlog.h"
#include "catalog/catalog.h"
+#include "datatype/timestamp.h"
#include "lib/pairingheap.h"
#include "miscadmin.h"
#include "storage/predicate.h"
@@ -67,6 +68,7 @@
#include "utils/resowner_private.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
+#include "utils/timestamp.h"
/*
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index c18554bae2c..9f9dc7d3354 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -16,11 +16,11 @@
#include "access/transam.h"
#include "access/xlogreader.h"
+#include "datatype/timestamp.h"
#include "lib/stringinfo.h"
#include "nodes/pg_list.h"
#include "storage/relfilenode.h"
#include "storage/sinval.h"
-#include "utils/datetime.h"
/*
* Maximum size of Global Transaction ID (including '\0').
--
2.20.1
>From f04db8fee6508000d3fdbbcaf1577186959ac310 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 14 Aug 2020 11:36:18 +0300
Subject: [PATCH 2/2] Mark commit and abort WAL records with
XLR_SPECIAL_REL_UPDATE.
If a commit or abort record includes "dropped relfilenodes", then replaying
the record will remove data files. That is surely a "special rel update",
but the records were not marked as such.
Teach pg_rewind to expect and ignore them, and add a test case to cover it.
It's always been like this, but no backporting for fear of breaking
existing applications. If an application parser the WAL but is not handling
commit/abort records, it would stop working. That might be a good thing if
it would actually need to handle the dropped rels, but it will be caught
when the application is updated to work with PostgreSQL v14 anyway.
---
src/backend/access/transam/xact.c | 2 ++
src/bin/pg_rewind/parsexlog.c | 13 +++++++++++++
src/bin/pg_rewind/t/001_basic.pl | 15 ++++++++++++++-
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 7ccb7d68ed9..af6afcebb13 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5565,6 +5565,7 @@ XactLogCommitRecord(TimestampTz commit_time,
{
xl_xinfo.xinfo |= XACT_XINFO_HAS_RELFILENODES;
xl_relfilenodes.nrels = nrels;
+ info |= XLR_SPECIAL_REL_UPDATE;
}
if (nmsgs > 0)
@@ -5697,6 +5698,7 @@ XactLogAbortRecord(TimestampTz abort_time,
{
xl_xinfo.xinfo |= XACT_XINFO_HAS_RELFILENODES;
xl_relfilenodes.nrels = nrels;
+ info |= XLR_SPECIAL_REL_UPDATE;
}
if (TransactionIdIsValid(twophase_xid))
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 2325fb5d302..2229c86f9af 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -14,6 +14,7 @@
#include <unistd.h>
#include "access/rmgr.h"
+#include "access/xact.h"
#include "access/xlog_internal.h"
#include "access/xlogreader.h"
#include "catalog/pg_control.h"
@@ -397,6 +398,18 @@ extractPageInfo(XLogReaderState *record)
* source system.
*/
}
+ else if (rmid == RM_XACT_ID &&
+ ((rminfo & XLOG_XACT_OPMASK) == XLOG_XACT_COMMIT ||
+ (rminfo & XLOG_XACT_OPMASK) == XLOG_XACT_COMMIT_PREPARED ||
+ (rminfo & XLOG_XACT_OPMASK) == XLOG_XACT_ABORT ||
+ (rminfo & XLOG_XACT_OPMASK) == XLOG_XACT_ABORT_PREPARED))
+ {
+ /*
+ * These records can include "dropped rels". We can safely ignore
+ * them, we will see that they are missing and copy them from the
+ * source.
+ */
+ }
else if (info & XLR_SPECIAL_REL_UPDATE)
{
/*
diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl
index fb4a0acd965..ba528e262f3 100644
--- a/src/bin/pg_rewind/t/001_basic.pl
+++ b/src/bin/pg_rewind/t/001_basic.pl
@@ -1,7 +1,7 @@
use strict;
use warnings;
use TestLib;
-use Test::More tests => 20;
+use Test::More tests => 23;
use FindBin;
use lib $FindBin::RealBin;
@@ -29,6 +29,10 @@ sub run_test
primary_psql("CREATE TABLE tail_tbl (id integer, d text)");
primary_psql("INSERT INTO tail_tbl VALUES (0, 'in primary')");
+ # This test table is dropped in the old primary after promotion.
+ primary_psql("CREATE TABLE drop_tbl (d text)");
+ primary_psql("INSERT INTO drop_tbl VALUES ('in primary')");
+
primary_psql("CHECKPOINT");
RewindTest::create_standby($test_mode);
@@ -66,6 +70,9 @@ sub run_test
primary_psql("DELETE FROM tail_tbl WHERE id > 10");
primary_psql("VACUUM tail_tbl");
+ # Drop drop_tbl. pg_rewind should copy it back.
+ primary_psql("DROP TABLE drop_tbl");
+
# Before running pg_rewind, do a couple of extra tests with several
# option combinations. As the code paths taken by those tests
# do not change for the "local" and "remote" modes, just run them
@@ -154,6 +161,12 @@ in primary, before promotion
),
'tail-copy');
+ check_query(
+ 'SELECT * FROM drop_tbl',
+ qq(in primary
+),
+ 'drop');
+
# Permissions on PGDATA should be default
SKIP:
{
--
2.20.1
