pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedCommit/abort WAL records with dropped rels missing XLR_SPECIAL_REL_UPDATE
4+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2020-08-17 a28d731a11 Mark commit and abort WAL records with XLR_SPECIAL_REL_UPDATE.
* Commit/abort WAL records with dropped rels missing XLR_SPECIAL_REL_UPDATE
@ 2020-08-14 08:47 Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 4+ messages in thread
From: Heikki Linnakangas @ 2020-08-14 08:47 UTC (permalink / raw)
To: pgsql-hackers
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
Attachments: download all (2 files, .tar.gz)
0001-Make-xact.h-usable-in-frontend.patch (text/x-patch, 2.1K) download
From cafc42a1dfe6afc9d36c0440dd45a5b12d9a6063 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@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
0002-Mark-commit-and-abort-WAL-records-with-XLR_SPECIAL_R.patch (text/x-patch, 4.0K) download
From f04db8fee6508000d3fdbbcaf1577186959ac310 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@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
* Re: Commit/abort WAL records with dropped rels missing XLR_SPECIAL_REL_UPDATE
@ 2020-08-15 05:35 Amit Kapila <amit.kapila16@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 4+ messages in thread
From: Amit Kapila @ 2020-08-15 05:35 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers
On Fri, Aug 14, 2020 at 2:17 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> 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?
>
Right.
> 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.
>
+1 for doing it in master only. Even if someone comes up with such a
scenario for back-branches, we can revisit our decision to backport
this but like you, I also don't see any pressing need to do it now.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Commit/abort WAL records with dropped rels missing XLR_SPECIAL_REL_UPDATE
@ 2020-08-17 07:00 Michael Paquier <michael@paquier.xyz>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 4+ messages in thread
From: Michael Paquier @ 2020-08-17 07:00 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; pgsql-hackers
On Sat, Aug 15, 2020 at 11:05:43AM +0530, Amit Kapila wrote:
> On Fri, Aug 14, 2020 at 2:17 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> 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.
>
> +1 for doing it in master only. Even if someone comes up with such a
> scenario for back-branches, we can revisit our decision to backport
> this but like you, I also don't see any pressing need to do it now.
+1.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Commit/abort WAL records with dropped rels missing XLR_SPECIAL_REL_UPDATE
@ 2020-08-17 07:55 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 4+ messages in thread
From: Heikki Linnakangas @ 2020-08-17 07:55 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: pgsql-hackers
On 17/08/2020 10:00, Michael Paquier wrote:
> On Sat, Aug 15, 2020 at 11:05:43AM +0530, Amit Kapila wrote:
>> On Fri, Aug 14, 2020 at 2:17 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>> 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.
>>
>> +1 for doing it in master only. Even if someone comes up with such a
>> scenario for back-branches, we can revisit our decision to backport
>> this but like you, I also don't see any pressing need to do it now.
>
> +1.
Pushed, thanks!
- Heikki
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2020-08-17 07:55 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-14 08:47 Commit/abort WAL records with dropped rels missing XLR_SPECIAL_REL_UPDATE Heikki Linnakangas <hlinnaka@iki.fi>
2020-08-15 05:35 ` Amit Kapila <amit.kapila16@gmail.com>
2020-08-17 07:00 ` Michael Paquier <michael@paquier.xyz>
2020-08-17 07:55 ` Heikki Linnakangas <hlinnaka@iki.fi>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox