pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feed BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
18+ messages / 7 participants
[nested] [flat]
* BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-05 09:59 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 3 replies; 18+ messages in thread
From: PG Bug reporting form @ 2019-07-05 09:59 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: myon@debian.org
The following bug has been logged on the website:
Bug reference: 15896
Logged by: Christoph Berg
Email address: myon@debian.org
PostgreSQL version: 12beta2
Operating system: Debian
Description:
When pg_upgrading from 10-or-earlier to 12beta2 or 13devel, an assertion is
raised. (Spotted by Debian's postgresql-common upgrade tests. Previously
missed because we were only testing version+1 upgrades.)
TRAP: FailedAssertion(»!(metad->btm_version >= 3)«, Datei:
»/build/postgresql-12-3URvLF/postgresql-12-12~beta2/build/../src/backend/access/nbtree/nbtpage.c«,
Zeile: 665)
Reproducer:
10.9:
CREATE TABLE phone (name varchar(255) PRIMARY KEY, tel int NOT NULL);
INSERT INTO phone VALUES ('Alice', 2);
CREATE USER foo;
GRANT INSERT ON phone TO foo;
INSERT INTO phone VALUES ('Bob', 1);
pg_upgradecluster -m upgrade 10 foo
(runs /usr/lib/postgresql/12/bin/initdb -D /var/lib/postgresql/12/foo
--auth-local peer --auth-host md5 --encoding UTF8 --lc-collate de_DE.utf8
--lc-ctype de_DE.utf8
/usr/lib/postgresql/12/bin/pg_upgrade -b /usr/lib/postgresql/10/bin -B
/usr/lib/postgresql/12/bin -p 5434 -P 5435 -d /etc/postgresql/10/foo -D
/etc/postgresql/12/foo)
12beta2:
SELECT * FROM phone ORDER BY name;
TRAP: FailedAssertion(»!(metad->btm_version >= 3)«, Datei:
»/build/postgresql-12-3URvLF/postgresql-12-12~beta2/build/../src/backend/access/nbtree/nbtpage.c«,
Zeile: 665)
2019-07-05 11:34:35.040 CEST [14325] LOG: Serverprozess (PID 14376) wurde
von Signal 6 beendet: Abgebrochen
2019-07-05 11:34:35.040 CEST [14325] DETAIL: Der fehlgeschlagene Prozess
führte aus: SELECT * FROM phone ORDER BY name;
Combinations tested:
bad: 10->12 9.6->12 9.6->13
good: 10->11 11->12 12->13
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-05 12:51 Bernd Helmle <mailings@oopsware.de>
parent: PG Bug reporting form <noreply@postgresql.org>
2 siblings, 0 replies; 18+ messages in thread
From: Bernd Helmle @ 2019-07-05 12:51 UTC (permalink / raw)
To: myon@debian.org; pgsql-bugs@lists.postgresql.org
Am Freitag, den 05.07.2019, 09:59 +0000 schrieb PG Bug reporting form:
> 12beta2:
> SELECT * FROM phone ORDER BY name;
> TRAP: FailedAssertion(»!(metad->btm_version >= 3)«, Datei:
> »/build/postgresql-12-3URvLF/postgresql-12-
> 12~beta2/build/../src/backend/access/nbtree/nbtpage.c«,
> Zeile: 665)
> 2019-07-05 11:34:35.040 CEST [14325] LOG: Serverprozess (PID 14376)
> wurde
> von Signal 6 beendet: Abgebrochen
> 2019-07-05 11:34:35.040 CEST [14325] DETAIL: Der fehlgeschlagene
> Prozess
> führte aus: SELECT * FROM phone ORDER BY name;
Additional note from me after a short test:
It works if you upgrade to PG11 first _and_ have touched the index in a
way it has rewritten the metapage to version 3, e.g. adding new pages
to the index.
Thanks
Bernd
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-05 15:49 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: PG Bug reporting form <noreply@postgresql.org>
2 siblings, 1 reply; 18+ messages in thread
From: Alvaro Herrera @ 2019-07-05 15:49 UTC (permalink / raw)
To: myon@debian.org; pgsql-bugs@lists.postgresql.org; +Cc: Peter Geoghegan <pg@bowt.ie>
Adding Peter G. to CC.
On 2019-Jul-05, PG Bug reporting form wrote:
> When pg_upgrading from 10-or-earlier to 12beta2 or 13devel, an assertion is
> raised. (Spotted by Debian's postgresql-common upgrade tests. Previously
> missed because we were only testing version+1 upgrades.)
>
> TRAP: FailedAssertion(»!(metad->btm_version >= 3)«, Datei:
> »/build/postgresql-12-3URvLF/postgresql-12-12~beta2/build/../src/backend/access/nbtree/nbtpage.c«,
> Zeile: 665)
Seems that _bt_getrootheight is too optimistic about the metapage
version it'll find. I suppose this could be handled by just not caching
the metapage if it is of the old version ... or maybe by calling
_bt_upgrademetapage().
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-05 22:14 Peter Geoghegan <pg@bowt.ie>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 2 replies; 18+ messages in thread
From: Peter Geoghegan @ 2019-07-05 22:14 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Christoph Berg <myon@debian.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>
On Fri, Jul 5, 2019 at 8:49 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > TRAP: FailedAssertion(»!(metad->btm_version >= 3)«, Datei:
> > »/build/postgresql-12-3URvLF/postgresql-12-12~beta2/build/../src/backend/access/nbtree/nbtpage.c«,
> > Zeile: 665)
>
> Seems that _bt_getrootheight is too optimistic about the metapage
> version it'll find. I suppose this could be handled by just not caching
> the metapage if it is of the old version ... or maybe by calling
> _bt_upgrademetapage().
The problem here predates v12 -- the call to _bt_cachemetadata() was
added to _bt_getrootheight() by commit 0a64b45152b, which went into
v11. My commit dd299df8189 added a new assertion that fails, but
that's just a symptom -- I changed the code in _bt_getrootheight() to
use a BTMetaPageData pointer to shared memory (i.e. a pointer to the
authoritative version), rather than using the newly-out-of-sync cached
version. It shouldn't be out-of-sync at all.
_bt_getrootheight() is mostly just something that exists for the
planner, so it has no business calling _bt_cachemetadata(), which will
"upgrade" the cached metadata image from version 2 to version 3 if it
happens to be on version 2. How can it be okay to upgrade the cached
version without also upgrading the on-disk/shared_buffers version?
This bug was hiding in plain sight.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-05 22:32 Peter Geoghegan <pg@bowt.ie>
parent: PG Bug reporting form <noreply@postgresql.org>
2 siblings, 1 reply; 18+ messages in thread
From: Peter Geoghegan @ 2019-07-05 22:32 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Fri, Jul 5, 2019 at 3:00 AM PG Bug reporting form
<noreply@postgresql.org> wrote:
> When pg_upgrading from 10-or-earlier to 12beta2 or 13devel, an assertion is
> raised. (Spotted by Debian's postgresql-common upgrade tests. Previously
> missed because we were only testing version+1 upgrades.)
Obviously you were right to adopt pg_upgrade tests that cross more
than one version at a time. Thanks!
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-06 13:11 Christoph Berg <myon@debian.org>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Berg @ 2019-07-06 13:11 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Re: Peter Geoghegan 2019-07-06 <CAH2-WznYXDG6aANoag5ixoMrV=0bD-W5i9cVq4cRkb-fA4xRmw@mail.gmail.com>
> > When pg_upgrading from 10-or-earlier to 12beta2 or 13devel, an assertion is
> > raised. (Spotted by Debian's postgresql-common upgrade tests. Previously
> > missed because we were only testing version+1 upgrades.)
>
> Obviously you were right to adopt pg_upgrade tests that cross more
> than one version at a time. Thanks!
Indeed, and running 9.6->10->11 tests (that were present before as
well, but never run) revealed another bug in pg_upgradecluster
itself... Testsuites are just awesome.
Christoph
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-11 07:18 Peter Geoghegan <pg@bowt.ie>
parent: Peter Geoghegan <pg@bowt.ie>
1 sibling, 1 reply; 18+ messages in thread
From: Peter Geoghegan @ 2019-07-11 07:18 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Christoph Berg <myon@debian.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>; Alexander Korotkov <a.korotkov@postgrespro.ru>
On Fri, Jul 5, 2019 at 3:14 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Fri, Jul 5, 2019 at 8:49 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > > TRAP: FailedAssertion(»!(metad->btm_version >= 3)«, Datei:
> > > »/build/postgresql-12-3URvLF/postgresql-12-12~beta2/build/../src/backend/access/nbtree/nbtpage.c«,
> > > Zeile: 665)
> >
> > Seems that _bt_getrootheight is too optimistic about the metapage
> > version it'll find. I suppose this could be handled by just not caching
> > the metapage if it is of the old version ... or maybe by calling
> > _bt_upgrademetapage().
>
> The problem here predates v12 -- the call to _bt_cachemetadata() was
> added to _bt_getrootheight() by commit 0a64b45152b, which went into
> v11. My commit dd299df8189 added a new assertion that fails, but
> that's just a symptom -- I changed the code in _bt_getrootheight() to
> use a BTMetaPageData pointer to shared memory (i.e. a pointer to the
> authoritative version), rather than using the newly-out-of-sync cached
> version. It shouldn't be out-of-sync at all.
>
> _bt_getrootheight() is mostly just something that exists for the
> planner, so it has no business calling _bt_cachemetadata(), which will
> "upgrade" the cached metadata image from version 2 to version 3 if it
> happens to be on version 2. How can it be okay to upgrade the cached
> version without also upgrading the on-disk/shared_buffers version?
> This bug was hiding in plain sight.
CC'ing Alexander, who was also involved in commit 0a64b45152b...
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-11 08:15 Michael Paquier <michael@paquier.xyz>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 18+ messages in thread
From: Michael Paquier @ 2019-07-11 08:15 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Christoph Berg <myon@debian.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>; Alexander Korotkov <a.korotkov@postgrespro.ru>
On Thu, Jul 11, 2019 at 12:18:46AM -0700, Peter Geoghegan wrote:
> On Fri, Jul 5, 2019 at 3:14 PM Peter Geoghegan <pg@bowt.ie> wrote:
>> _bt_getrootheight() is mostly just something that exists for the
>> planner, so it has no business calling _bt_cachemetadata(), which will
>> "upgrade" the cached metadata image from version 2 to version 3 if it
>> happens to be on version 2. How can it be okay to upgrade the cached
>> version without also upgrading the on-disk/shared_buffers version?
>> This bug was hiding in plain sight.
>
> CC'ing Alexander, who was also involved in commit 0a64b45152b...
So the problem has been introduced in v11 per this commit, still we
only see the issue since v12 because your code relied on a wrong
assumption. Per what I am reading, it seems to me that we should fix
v11, but that's a live problem for v12 because of the page format
upgrade so we need to track it as an open item.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190711081553.GI4500@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-12 03:31 Peter Geoghegan <pg@bowt.ie>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 18+ messages in thread
From: Peter Geoghegan @ 2019-07-12 03:31 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Christoph Berg <myon@debian.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>; Alexander Korotkov <a.korotkov@postgrespro.ru>
On Thu, Jul 11, 2019 at 1:16 AM Michael Paquier <michael@paquier.xyz> wrote:
> So the problem has been introduced in v11 per this commit, still we
> only see the issue since v12 because your code relied on a wrong
> assumption.
We see the issue on v12 because my code added an assertion that caught
the issue. The behavior on v12 is otherwise substantively the same as
on v11. Even if it was true that my commits made nbtree rely on the
same "wrong assumption" more often than on v11, why would it matter?
The bug is still a v11 bug.
> Per what I am reading, it seems to me that we should fix
> v11, but that's a live problem for v12 because of the page format
> upgrade so we need to track it as an open item.
The highest priority ought to be fixing the problem in the stable v11
branch. That said, there is hardly any difference between v11 and v12.
It isn't possible to upgrade a B-Tree index from v2/v3 to v4 (the
latest version) on-the-fly. But if in your judgement it makes sense to
add a v12 open item, I have no objection.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-16 02:01 Peter Geoghegan <pg@bowt.ie>
parent: Peter Geoghegan <pg@bowt.ie>
1 sibling, 1 reply; 18+ messages in thread
From: Peter Geoghegan @ 2019-07-16 02:01 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Christoph Berg <myon@debian.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>
On Fri, Jul 5, 2019 at 3:14 PM Peter Geoghegan <pg@bowt.ie> wrote:
> The problem here predates v12 -- the call to _bt_cachemetadata() was
> added to _bt_getrootheight() by commit 0a64b45152b, which went into
> v11. My commit dd299df8189 added a new assertion that fails, but
> that's just a symptom -- I changed the code in _bt_getrootheight() to
> use a BTMetaPageData pointer to shared memory (i.e. a pointer to the
> authoritative version), rather than using the newly-out-of-sync cached
> version. It shouldn't be out-of-sync at all.
The attached HEAD-only patch fixes the issue by not actually upgrading
the page within _bt_cachemetadata(), while making some existing
assertions less aggressive. I haven't tested it properly just yet --
ran out of time to do that today.
Maybe you can try this out with your original test case, Christoph?
Thanks
--
Peter Geoghegan
Attachments:
[application/octet-stream] 0001-Fix-nbtree-metapage-cache-upgrade-bug.patch (4.3K, ../../CAH2-Wz=jvYQmPgSxZCdHe1NwK+S3j1jF6ydc1G0eHs7BNdpn2w@mail.gmail.com/2-0001-Fix-nbtree-metapage-cache-upgrade-bug.patch)
download | inline diff:
From 423a21396f1571b38424b9ef3280cdf529384f50 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Mon, 15 Jul 2019 17:51:03 -0700
Subject: [PATCH] Fix nbtree metapage cache upgrade bug.
It's not okay to update a cached copy of an indexes metapage metadata at
a point where it isn't also being updated on disk.
Discussion: https://postgr.es/m/CAH2-Wzmj6pz98qZ6+Ro-=tHvyBJ6q0yxHV8QLOr6O0mE20Nw9Q@mail.gmail.com
---
src/backend/access/nbtree/nbtinsert.c | 3 ++-
src/backend/access/nbtree/nbtpage.c | 20 ++++++++++++--------
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 602f8849d4..46a1167d65 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -1135,6 +1135,7 @@ _bt_insertonpg(Relation rel,
if (BufferIsValid(metabuf))
{
+ /* _bt_upgrademetapage() should have been called */
Assert(metad->btm_version >= BTREE_NOVAC_VERSION);
xlmeta.version = metad->btm_version;
xlmeta.root = metad->btm_root;
@@ -2160,7 +2161,7 @@ _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf)
XLogRegisterBuffer(0, rootbuf, REGBUF_WILL_INIT);
XLogRegisterBuffer(1, lbuf, REGBUF_STANDARD);
XLogRegisterBuffer(2, metabuf, REGBUF_WILL_INIT | REGBUF_STANDARD);
-
+ /* _bt_upgrademetapage() should have been called */
Assert(metad->btm_version >= BTREE_NOVAC_VERSION);
md.version = metad->btm_version;
md.root = rootblknum;
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 50455db9af..1da710ba90 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -135,14 +135,12 @@ _bt_cachemetadata(Relation rel, BTMetaPageData *input)
else
{
/*
- * Upgrade meta-data: copy available information from meta-page and
- * fill new fields with default values.
- *
- * Note that we cannot upgrade to version 4+ without a REINDEX, since
- * extensive on-disk changes are required.
+ * Copy available information from meta-page and fill new fields from
+ * later versions with default values defensively. The cached version
+ * number should not change, though.
*/
memcpy(cached_metad, input, offsetof(BTMetaPageData, btm_oldest_btpo_xact));
- cached_metad->btm_version = BTREE_NOVAC_VERSION;
+ cached_metad->btm_version = input->btm_version;
cached_metad->btm_oldest_btpo_xact = InvalidTransactionId;
cached_metad->btm_last_cleanup_num_heap_tuples = -1.0;
}
@@ -241,6 +239,7 @@ _bt_update_meta_cleanup_info(Relation rel, TransactionId oldestBtpoXact,
XLogBeginInsert();
XLogRegisterBuffer(0, metabuf, REGBUF_WILL_INIT | REGBUF_STANDARD);
+ /* _bt_upgrademetapage() should have been called */
Assert(metad->btm_version >= BTREE_NOVAC_VERSION);
md.version = metad->btm_version;
md.root = metad->btm_root;
@@ -439,6 +438,7 @@ _bt_getroot(Relation rel, int access)
XLogRegisterBuffer(0, rootbuf, REGBUF_WILL_INIT);
XLogRegisterBuffer(2, metabuf, REGBUF_WILL_INIT | REGBUF_STANDARD);
+ /* _bt_upgrademetapage() should have been called */
Assert(metad->btm_version >= BTREE_NOVAC_VERSION);
md.version = metad->btm_version;
md.root = rootblkno;
@@ -660,7 +660,7 @@ _bt_getrootheight(Relation rel)
_bt_cachemetadata(rel, metad);
/* We shouldn't have cached it if any of these fail */
Assert(metad->btm_magic == BTREE_MAGIC);
- Assert(metad->btm_version >= BTREE_NOVAC_VERSION);
+ Assert(metad->btm_version >= BTREE_MIN_VERSION);
Assert(metad->btm_fastroot != P_NONE);
_bt_relbuf(rel, metabuf);
}
@@ -711,7 +711,7 @@ _bt_heapkeyspace(Relation rel)
_bt_cachemetadata(rel, metad);
/* We shouldn't have cached it if any of these fail */
Assert(metad->btm_magic == BTREE_MAGIC);
- Assert(metad->btm_version >= BTREE_NOVAC_VERSION);
+ Assert(metad->btm_version >= BTREE_MIN_VERSION);
Assert(metad->btm_fastroot != P_NONE);
_bt_relbuf(rel, metabuf);
}
@@ -2094,6 +2094,10 @@ _bt_unlink_halfdead_page(Relation rel, Buffer leafbuf, bool *rightsib_empty)
{
XLogRegisterBuffer(4, metabuf, REGBUF_WILL_INIT | REGBUF_STANDARD);
+ /*
+ * _bt_upgrademetapage() should have been called when root needs
+ * to be updated
+ */
Assert(metad->btm_version >= BTREE_NOVAC_VERSION);
xlmeta.version = metad->btm_version;
xlmeta.root = metad->btm_root;
--
2.17.1
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-16 08:13 Christoph Berg <myon@debian.org>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Berg @ 2019-07-16 08:13 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>
Re: Peter Geoghegan 2019-07-16 <CAH2-Wz=jvYQmPgSxZCdHe1NwK+S3j1jF6ydc1G0eHs7BNdpn2w@mail.gmail.com>
> Maybe you can try this out with your original test case, Christoph?
I applied the patch to 12/HEAD and the Debian testsuite now passes the
10->12 upgrade test.
Thanks,
Christoph
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-17 23:10 Peter Geoghegan <pg@bowt.ie>
parent: Christoph Berg <myon@debian.org>
0 siblings, 1 reply; 18+ messages in thread
From: Peter Geoghegan @ 2019-07-17 23:10 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>; Alexander Korotkov <a.korotkov@postgrespro.ru>
On Tue, Jul 16, 2019 at 1:13 AM Christoph Berg <myon@debian.org> wrote:
> I applied the patch to 12/HEAD and the Debian testsuite now passes the
> 10->12 upgrade test.
Thanks for testing!
I am not quite sure if I should push ahead with this, simply because I
don't know what the point of commit 0a64b45152b really was (Alexandar?
Teodor?). Why not just make the assertions a bit more less strict in
one or two places? Is the _bt_cachemetadata() function really
necessary? Can we remove it now?
AFAICT the only purpose that _bt_cachemetadata() serves that isn't
better handled by updating the assertions that were failing back in
April of 2018 (and still sometimes fail) is initializing the new-to-v3
fields defensively (initializing btm_oldest_btpo_xact and
btm_last_cleanup_num_heap_tuples are set to their default values).
Even that seems unnecessary, since every piece of code knows that it
isn't sensible to read those values. Including contrib/pageinspect,
which was actually taught this by commit 0a64b45152b itself.
I'm a bit nervous about pushing a commit that will almost be a
straight revert of 0a64b45152b without first getting confirmation that
_bt_cachemetadata() is actually totally unnecessary.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-17 23:51 Peter Geoghegan <pg@bowt.ie>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 18+ messages in thread
From: Peter Geoghegan @ 2019-07-17 23:51 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>; Alexander Korotkov <a.korotkov@postgrespro.ru>
On Wed, Jul 17, 2019 at 4:10 PM Peter Geoghegan <pg@bowt.ie> wrote:
> I am not quite sure if I should push ahead with this, simply because I
> don't know what the point of commit 0a64b45152b really was (Alexandar?
> Teodor?). Why not just make the assertions a bit more less strict in
> one or two places? Is the _bt_cachemetadata() function really
> necessary? Can we remove it now?
Attached revision of the patch shows what I have in mind -- this is
almost a straight revert of 0a64b45152b. This is really no different
than the first version, though it couldn't hurt to test it once more
if you have time, Christoph.
I wonder why commit 0a64b45152b didn't look like this in the first
place. My approach is the obvious way to fix the problem that
0a64b45152b was designed to fix. Am I missing something?
Prior to Postgres v11, we never supported upgrades -- the nbtree
version was 2 for many many years (since before we even had pg_upgrade
to worry about). Obviously that meant that assertions that looked like
"Assert(metad->btm_version == BTREE_VERSION)" had to be updated by
commit 857f9c36cda (this was a big feature commit that commit
0a64b45152b tried to fix-up). It looks like commit 857f9c36cda simply
missed a few assertions in the upgrade path, including
_bt_getrootheight(). So, again, I don't know why commit 0a64b45152b
had to go any further than updating 2 or 3 assertions that were
overlooked in commit 857f9c36cda.
--
Peter Geoghegan
Attachments:
[application/octet-stream] v2-0001-Fix-nbtree-metapage-cache-upgrade-bug.patch (6.0K, ../../CAH2-Wznb+-L9_HEjSGZD0Wty8Auqmfkna7pNN6nu+oWis7iWYA@mail.gmail.com/2-v2-0001-Fix-nbtree-metapage-cache-upgrade-bug.patch)
download | inline diff:
From d03e6be539c51d98a6ec0bc90a063edc16b1b202 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Mon, 15 Jul 2019 17:51:03 -0700
Subject: [PATCH v2] Fix nbtree metapage cache upgrade bug.
It's not okay to update a cached copy of an nbtree index's metapage
metadata at a point where it isn't also being updated on disk. Commit
0a64b45152b seems to have gotten this wrong.
This is almost a straight revert of commit 0a64b45152b, except the
changes to contrib/pageinspect remain, and the failing assertions are
updated to deal with the possibility that an index might be pg_upgrade'd
from an earlier version.
Discussion: https://postgr.es/m/CAH2-Wzmj6pz98qZ6+Ro-=tHvyBJ6q0yxHV8QLOr6O0mE20Nw9Q@mail.gmail.com
---
src/backend/access/nbtree/nbtpage.c | 70 ++++++++---------------------
src/include/access/nbtree.h | 2 +-
2 files changed, 20 insertions(+), 52 deletions(-)
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 50455db9af..5cf7a3f6d9 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -33,7 +33,6 @@
#include "storage/predicate.h"
#include "utils/snapmgr.h"
-static void _bt_cachemetadata(Relation rel, BTMetaPageData *input);
static BTMetaPageData *_bt_getmeta(Relation rel, Buffer metabuf);
static bool _bt_mark_page_halfdead(Relation rel, Buffer buf, BTStack stack);
static bool _bt_unlink_halfdead_page(Relation rel, Buffer leafbuf,
@@ -109,45 +108,6 @@ _bt_upgrademetapage(Page page)
((char *) metad + sizeof(BTMetaPageData)) - (char *) page;
}
-/*
- * Cache metadata from input meta page to rel->rd_amcache.
- */
-static void
-_bt_cachemetadata(Relation rel, BTMetaPageData *input)
-{
- BTMetaPageData *cached_metad;
-
- /* We assume rel->rd_amcache was already freed by caller */
- Assert(rel->rd_amcache == NULL);
- rel->rd_amcache = MemoryContextAlloc(rel->rd_indexcxt,
- sizeof(BTMetaPageData));
-
- /* Meta page should be of supported version */
- Assert(input->btm_version >= BTREE_MIN_VERSION &&
- input->btm_version <= BTREE_VERSION);
-
- cached_metad = (BTMetaPageData *) rel->rd_amcache;
- if (input->btm_version >= BTREE_NOVAC_VERSION)
- {
- /* Version with compatible meta-data, no need to upgrade */
- memcpy(cached_metad, input, sizeof(BTMetaPageData));
- }
- else
- {
- /*
- * Upgrade meta-data: copy available information from meta-page and
- * fill new fields with default values.
- *
- * Note that we cannot upgrade to version 4+ without a REINDEX, since
- * extensive on-disk changes are required.
- */
- memcpy(cached_metad, input, offsetof(BTMetaPageData, btm_oldest_btpo_xact));
- cached_metad->btm_version = BTREE_NOVAC_VERSION;
- cached_metad->btm_oldest_btpo_xact = InvalidTransactionId;
- cached_metad->btm_last_cleanup_num_heap_tuples = -1.0;
- }
-}
-
/*
* Get metadata from share-locked buffer containing metapage, while performing
* standard sanity checks. Sanity checks here must match _bt_getroot().
@@ -483,7 +443,9 @@ _bt_getroot(Relation rel, int access)
/*
* Cache the metapage data for next time
*/
- _bt_cachemetadata(rel, metad);
+ rel->rd_amcache = MemoryContextAlloc(rel->rd_indexcxt,
+ sizeof(BTMetaPageData));
+ memcpy(rel->rd_amcache, metad, sizeof(BTMetaPageData));
/*
* We are done with the metapage; arrange to release it via first
@@ -650,6 +612,7 @@ _bt_getrootheight(Relation rel)
*/
if (metad->btm_root == P_NONE)
{
+ Assert(metad->btm_version >= BTREE_MIN_VERSION);
_bt_relbuf(rel, metabuf);
return 0;
}
@@ -657,16 +620,18 @@ _bt_getrootheight(Relation rel)
/*
* Cache the metapage data for next time
*/
- _bt_cachemetadata(rel, metad);
- /* We shouldn't have cached it if any of these fail */
- Assert(metad->btm_magic == BTREE_MAGIC);
- Assert(metad->btm_version >= BTREE_NOVAC_VERSION);
- Assert(metad->btm_fastroot != P_NONE);
+ rel->rd_amcache = MemoryContextAlloc(rel->rd_indexcxt,
+ sizeof(BTMetaPageData));
+ memcpy(rel->rd_amcache, metad, sizeof(BTMetaPageData));
_bt_relbuf(rel, metabuf);
}
/* Get cached page */
metad = (BTMetaPageData *) rel->rd_amcache;
+ /* We shouldn't have cached it if any of these fail */
+ Assert(metad->btm_magic == BTREE_MAGIC);
+ Assert(metad->btm_version >= BTREE_MIN_VERSION);
+ Assert(metad->btm_fastroot != P_NONE);
return metad->btm_fastlevel;
}
@@ -701,6 +666,7 @@ _bt_heapkeyspace(Relation rel)
{
uint32 btm_version = metad->btm_version;
+ Assert(metad->btm_version >= BTREE_MIN_VERSION);
_bt_relbuf(rel, metabuf);
return btm_version > BTREE_NOVAC_VERSION;
}
@@ -708,16 +674,18 @@ _bt_heapkeyspace(Relation rel)
/*
* Cache the metapage data for next time
*/
- _bt_cachemetadata(rel, metad);
- /* We shouldn't have cached it if any of these fail */
- Assert(metad->btm_magic == BTREE_MAGIC);
- Assert(metad->btm_version >= BTREE_NOVAC_VERSION);
- Assert(metad->btm_fastroot != P_NONE);
+ rel->rd_amcache = MemoryContextAlloc(rel->rd_indexcxt,
+ sizeof(BTMetaPageData));
+ memcpy(rel->rd_amcache, metad, sizeof(BTMetaPageData));
_bt_relbuf(rel, metabuf);
}
/* Get cached page */
metad = (BTMetaPageData *) rel->rd_amcache;
+ /* We shouldn't have cached it if any of these fail */
+ Assert(metad->btm_magic == BTREE_MAGIC);
+ Assert(metad->btm_version >= BTREE_MIN_VERSION);
+ Assert(metad->btm_fastroot != P_NONE);
return metad->btm_version > BTREE_NOVAC_VERSION;
}
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index a3583f225b..1b312311ea 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -97,7 +97,7 @@ typedef BTPageOpaqueData *BTPageOpaque;
typedef struct BTMetaPageData
{
uint32 btm_magic; /* should contain BTREE_MAGIC */
- uint32 btm_version; /* should contain BTREE_VERSION */
+ uint32 btm_version; /* nbtree version (always <= BTREE_VERSION) */
BlockNumber btm_root; /* current root location */
uint32 btm_level; /* tree level of the root page */
BlockNumber btm_fastroot; /* current "fast" root location */
--
2.17.1
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-18 09:30 Christoph Berg <myon@debian.org>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 18+ messages in thread
From: Christoph Berg @ 2019-07-18 09:30 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>; Alexander Korotkov <a.korotkov@postgrespro.ru>
Re: Peter Geoghegan 2019-07-18 <CAH2-Wznb+-L9_HEjSGZD0Wty8Auqmfkna7pNN6nu+oWis7iWYA@mail.gmail.com>
> Attached revision of the patch shows what I have in mind -- this is
> almost a straight revert of 0a64b45152b. This is really no different
> than the first version, though it couldn't hurt to test it once more
> if you have time, Christoph.
That version works as well on 12 HEAD. It passes "check world" and the
Debian testsuite on top of that running 10->12 upgrade tests.
Christoph
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-18 20:25 Peter Geoghegan <pg@bowt.ie>
parent: Christoph Berg <myon@debian.org>
0 siblings, 1 reply; 18+ messages in thread
From: Peter Geoghegan @ 2019-07-18 20:25 UTC (permalink / raw)
To: Christoph Berg <myon@debian.org>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>; Alexander Korotkov <a.korotkov@postgrespro.ru>
On Thu, Jul 18, 2019 at 2:30 AM Christoph Berg <myon@debian.org> wrote:
> That version works as well on 12 HEAD. It passes "check world" and the
> Debian testsuite on top of that running 10->12 upgrade tests.
Pushed a slightly refined version of the same patch back to the v11 branch.
Thanks for the report, and for testing!
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-18 20:57 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 18+ messages in thread
From: Alvaro Herrera @ 2019-07-18 20:57 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Christoph Berg <myon@debian.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>; Alexander Korotkov <a.korotkov@postgrespro.ru>
On 2019-Jul-18, Peter Geoghegan wrote:
> On Thu, Jul 18, 2019 at 2:30 AM Christoph Berg <myon@debian.org> wrote:
> > That version works as well on 12 HEAD. It passes "check world" and the
> > Debian testsuite on top of that running 10->12 upgrade tests.
>
> Pushed a slightly refined version of the same patch back to the v11 branch.
Thank you!
I wonder what the consequences are in a system that doesn't have
assertions enabled. Do we just end up with an "upgraded" cached
metapage, and no further ill effects? That seems pretty benign ...
I hope we don't end up with an expected v4 metapage or something crazy
like that? (I suppose we would have noticed.)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)«
@ 2019-07-18 21:27 Peter Geoghegan <pg@bowt.ie>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 0 replies; 18+ messages in thread
From: Peter Geoghegan @ 2019-07-18 21:27 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Christoph Berg <myon@debian.org>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>; Teodor Sigaev <teodor@sigaev.ru>; Alexander Korotkov <a.korotkov@postgrespro.ru>
On Thu, Jul 18, 2019 at 1:57 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> I wonder what the consequences are in a system that doesn't have
> assertions enabled. Do we just end up with an "upgraded" cached
> metapage, and no further ill effects? That seems pretty benign ...
AFAICT, it's benign. The new fields added to the metapage by commit
857f9c36cda were put there because that happened to be convenient.
They were only really something that VACUUM is expected to care about.
The usual concerns about having a stale meta page don't apply to those
fields.
The design of commit 0a64b45152b seems to have been "let's just
pretend that it's a v3 page in the cached version, not a v2 page,
since code that might used a cached version won't care about the
difference, and code that does care about the difference definitely
won't use the cache". This was made much more complicated by the fact
that 0a64b45152b didn't really seem to admit that it was doing this --
it's very weird that the cached version could be *ahead of* the
authoritative version.
It's now possible that the cached version number will become stale on
v11, but that should be harmless for the same reason as it was
harmless when it happened in the opposite direction. Besides, there
are well established ways in which a cached nbtree metapage is allowed
to become stale -- comments above _bt_getrootheight() have always said
that that's okay.
The situation here was confusing enough that it seemed worth
backpatching a fix to v11, even though I believe that nobody using v11
will have see any misbehavior related to commit 0a64b45152b.
> I hope we don't end up with an expected v4 metapage or something crazy
> like that? (I suppose we would have noticed.)
I think that we'd have heard about it by now.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 18+ messages in thread
* [PATCH] Search for SLRU page only in its own bank
@ 2024-08-13 18:23 Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 18+ messages in thread
From: Alvaro Herrera @ 2024-08-13 18:23 UTC (permalink / raw)
Author: Sergey Sargsyan <sergey.sargsyan.2001@gmail.com>
Discussion: https://postgr.es/m/18582-5f301dd30ba91a38@postgresql.org
---
src/backend/access/transam/slru.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 248aa1a655..27473009b0 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -1184,7 +1184,7 @@ SlruSelectLRUPage(SlruCtl ctl, int64 pageno)
Assert(LWLockHeldByMe(SimpleLruGetBankLock(ctl, pageno)));
/* See if page already has a buffer assigned */
- for (int slotno = 0; slotno < shared->num_slots; slotno++)
+ for (int slotno = bankstart; slotno < bankend; slotno++)
{
if (shared->page_status[slotno] != SLRU_PAGE_EMPTY &&
shared->page_number[slotno] == pageno)
--
2.39.2
--v2knzwxqy2cmbttn--
^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2024-08-13 18:23 UTC | newest]
Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-07-05 09:59 BUG #15896: pg_upgrade from 10-or-earlier: TRAP: FailedAssertion(»!(metad->btm_version >= 3)« PG Bug reporting form <noreply@postgresql.org>
2019-07-05 12:51 ` Bernd Helmle <mailings@oopsware.de>
2019-07-05 15:49 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-07-05 22:14 ` Peter Geoghegan <pg@bowt.ie>
2019-07-11 07:18 ` Peter Geoghegan <pg@bowt.ie>
2019-07-11 08:15 ` Michael Paquier <michael@paquier.xyz>
2019-07-12 03:31 ` Peter Geoghegan <pg@bowt.ie>
2019-07-16 02:01 ` Peter Geoghegan <pg@bowt.ie>
2019-07-16 08:13 ` Christoph Berg <myon@debian.org>
2019-07-17 23:10 ` Peter Geoghegan <pg@bowt.ie>
2019-07-17 23:51 ` Peter Geoghegan <pg@bowt.ie>
2019-07-18 09:30 ` Christoph Berg <myon@debian.org>
2019-07-18 20:25 ` Peter Geoghegan <pg@bowt.ie>
2019-07-18 20:57 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-07-18 21:27 ` Peter Geoghegan <pg@bowt.ie>
2019-07-05 22:32 ` Peter Geoghegan <pg@bowt.ie>
2019-07-06 13:11 ` Christoph Berg <myon@debian.org>
2024-08-13 18:23 [PATCH] Search for SLRU page only in its own bank Alvaro Herrera <alvherre@alvh.no-ip.org>
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