Re: bt_index_parent_check and concurrently build indexes
Hello, Andrey! Thanks for the review! > I think usually write only commit year. Something tells me you can safely write 2025 there. Done. > Can't wrap my head why do you need this? Oops, copy-paste, fixed. > I think this comment describes behavior before the fix in present tense. Fixed. > Snapshot business seems incorrect to me here... Hm, it seems like it is correct. `snapshot` variable is deleted, we only use `state->snapshot` now (if it is required at all). Best regards, Mikhail. v2-0001-amcheck-Fix-bt_index_parent_check-behavior-with-C.patch Description: Binary data
Re: bt_index_parent_check and concurrently build indexes
> On 13 Dec 2024, at 04:59, Michail Nikolaev wrote:
>
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
I think usually write only commit year. Something tells me you can safely write
2025 there.
+Test::More->builder->todo_start('filesystem bug')
+ if PostgreSQL::Test::Utils::has_wal_read_bug;
Can't wrap my head why do you need this?
+# it fails, because it is expect to find the deleted row in index
I think this comment describes behavior before the fix in present tense.
- if (snapshot != SnapshotAny)
- UnregisterSnapshot(snapshot);
Snapshot business seems incorrect to me here...
Best regards, Andrey Borodin.
Re: bt_index_parent_check and concurrently build indexes
On Mon, Dec 9, 2024 at 3:51 PM Michail Nikolaev wrote: > After some time, I managed to find a way to reproduce the issue. It turns out > that bt_index_parent_check is not suitable for validating indexes built > concurrently. Good catch. > I’ve attached a patch that reproduces the issue (it incorrectly reports the > index as invalid, even though it is actually valid). > > I’m unsure of the best way to address this issue, but here are some possible > options: > * Simply update the documentation. > * Issue a WARNING if !tupleIsAlive. > * Modify bt_index_parent_check to use an MVCC snapshot for the heap scan Offhand, I think that using an MVCC snapshot would make the most sense. It's not as if there is a big benefit to not doing so with bt_index_parent_check. My recollection is that we did it this way because it was as close as possible to the CREATE INDEX code that heapallindexed verification was loosely based on. -- Peter Geoghegan
Re: bt_index_parent_check and concurrently build indexes
Hello, Andrey! > Interesting bug. It's amazing how long it stand, giving that it would be triggered by almost any check after updating a table. Probably because in real cases, bt_index_check is used much more frequently than bt_index_parent_check. > From my POV correct fix direction is to use approach similar to index building. > E.i. remove "if (!state->readonly)" check. Are there any known downsides of this? Yes, it also looks correct to me. I have attached the patch with such changes. Also, I have registered a commit fest entry for the issue: https://commitfest.postgresql.org/51/5438/ v3-0001-amcheck-Fix-bt_index_parent_check-behavior-with-C.patch Description: Binary data
Re: bt_index_parent_check and concurrently build indexes
> On 9 Dec 2024, at 23:51, Michail Nikolaev wrote: > > * Modify bt_index_parent_check to use an MVCC snapshot for the heap scan Hi! Interesting bug. It's amazing how long it stand, giving that it would be triggered by almost any check after updating a table. From my POV correct fix direction is to use approach similar to index building. E.i. remove "if (!state->readonly)" check. Are there any known downsides of this? Best regards, Andrey Borodin.
Re: bt_index_parent_check and concurrently build indexes
Hello, everyone and Peter.
I simplified the patch to reproduce the issue more effectively. Now,
bt_index_parent_check fails on a valid index containing just two tuples.
Peter, I included you in this message because you are the author of
bt_index_parent_check, so I thought you might find this relevant.
Best regards,
Mikhail.
>
From 6de9549360b7d92b0184c50016aec1c41534e127 Mon Sep 17 00:00:00 2001
From: nkey
Date: Sun, 8 Dec 2024 17:32:15 +0100
Subject: [PATCH v2] test to reproduce issue with bt_index_parent_check and
CREATE INDEX CONCURRENTLY
---
contrib/amcheck/meson.build | 1 +
.../t/006_cic_bt_index_parent_check.pl| 51 +++
2 files changed, 52 insertions(+)
create mode 100644 contrib/amcheck/t/006_cic_bt_index_parent_check.pl
diff --git a/contrib/amcheck/meson.build b/contrib/amcheck/meson.build
index fc08e32539..2eb7ff11bd 100644
--- a/contrib/amcheck/meson.build
+++ b/contrib/amcheck/meson.build
@@ -45,6 +45,7 @@ tests += {
't/003_cic_2pc.pl',
't/004_verify_nbtree_unique.pl',
't/005_pitr.pl',
+ 't/006_cic_bt_index_parent_check.pl',
],
},
}
diff --git a/contrib/amcheck/t/006_cic_bt_index_parent_check.pl b/contrib/amcheck/t/006_cic_bt_index_parent_check.pl
new file mode 100644
index 00..1d8bb84f22
--- /dev/null
+++ b/contrib/amcheck/t/006_cic_bt_index_parent_check.pl
@@ -0,0 +1,51 @@
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Test bt_index_parent_check with index created with CREATE INDEX CONCURRENTLY
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+
+use Test::More;
+
+Test::More->builder->todo_start('filesystem bug')
+ if PostgreSQL::Test::Utils::has_wal_read_bug;
+
+my ($node, $result);
+
+#
+# Test set-up
+#
+$node = PostgreSQL::Test::Cluster->new('CIC_bt_index_parent_check_test');
+$node->init;
+$node->append_conf('postgresql.conf', 'fsync = off');
+$node->append_conf('postgresql.conf', 'autovacuum = off');
+$node->append_conf('postgresql.conf',
+ 'lock_timeout = ' . (1000 * $PostgreSQL::Test::Utils::timeout_default));
+$node->start;
+$node->safe_psql('postgres', q(CREATE EXTENSION amcheck));
+$node->safe_psql('postgres', q(CREATE TABLE tbl(i int primary key)));
+# Insert two rows into index
+$node->safe_psql('postgres', q(INSERT INTO tbl SELECT i FROM generate_series(1, 2) s(i);));
+
+# start background transaction
+my $in_progress_h = $node->background_psql('postgres');
+$in_progress_h->query_safe(
+ q(
+BEGIN;
+SELECT pg_current_xact_id();
+));
+
+# delete one row from table, while background transaction is in progress
+$node->safe_psql('postgres', q(DELETE FROM tbl WHERE i = 1;));
+# create index concurrently, which will skip the deleted row
+$node->safe_psql('postgres', q(CREATE INDEX CONCURRENTLY idx ON tbl(i);));
+
+# check index using bt_index_parent_check
+$result = $node->psql('postgres', q(SELECT bt_index_parent_check('idx', heapallindexed => true)));
+# it fails, because it is expect to find the deleted row in index
+is($result, '0', 'bt_index_parent_check after removed rows');
+
+$in_progress_h->quit;
+done_testing();
--
2.43.0
