From: Mark Dilger <mark.dilger@enterprisedb.com>
To: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Subject: Re: BUG #17212: pg_amcheck fails on checking temporary relations
Date: Sun, 3 Oct 2021 15:20:20 -0700
Message-ID: <E283B45F-5745-49C9-BBC7-99876E2FABD1@enterprisedb.com> (raw)
In-Reply-To: <E85855BD-C063-4B24-9EAA-02DECC8F42B9@enterprisedb.com>
References: <17212-34dd4a1d6bba98bf@postgresql.org>
<CAH2-Wz=bw-Uk2yoOq7yevC9hyMGgZ2Ud34nd9YJJPXJ3pkVcrw@mail.gmail.com>
<E85855BD-C063-4B24-9EAA-02DECC8F42B9@enterprisedb.com>
> On Oct 3, 2021, at 10:04 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
>> On Oct 2, 2021, at 10:32 AM, Peter Geoghegan <pg@bowt.ie> wrote:
>>
>> On Sat, Oct 2, 2021 at 4:49 AM PG Bug reporting form
>> <noreply@postgresql.org> wrote:
>>> Although you can add --exclude-relation=*.pg_temp*.*, this behaviour differs
>>> from the behaviour of pg_dump and friends, which skip such relations
>>> silently.
>>
>> I agree -- this behavior is a bug.
>>
>> Can you propose a fix, Mark?
>
> The attached patch includes a test case for this, which shows the problems against the current pg_amcheck.c, and a new version of pg_amcheck.c which fixes the bug. Could you review it?
>
> Thanks for bringing this to my attention.
Reposting to pgsql-hackers in preparation for making a commitfest entry.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[application/octet-stream] v1-0001-Bug-fix-do-not-check-temp-tables-from-pg_amcheck.patch (4.9K, ../E283B45F-5745-49C9-BBC7-99876E2FABD1@enterprisedb.com/2-v1-0001-Bug-fix-do-not-check-temp-tables-from-pg_amcheck.patch)
download | inline diff:
From fc2e4a14113f7b3d556707f900f1c6cf2a61c601 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Sun, 3 Oct 2021 09:52:22 -0700
Subject: [PATCH v1] Bug fix: do not check temp tables from pg_amcheck
Fix BUG #17212: pg_amcheck fails on checking temporary relations
Since pg_amcheck does not create temporary tables of its own, all
temporary tables it encounters must necessarily belong to other
sessions. Check neither temporary tables nor indexes over such
tables.
---
src/bin/pg_amcheck/pg_amcheck.c | 15 +++++++-------
src/bin/pg_amcheck/t/003_check.pl | 33 +++++++++++++++++++++++++++++++
2 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.cindex ec04b977de..33a9965a60 100644--- a/src/bin/pg_amcheck/pg_amcheck.c+++ b/src/bin/pg_amcheck/pg_amcheck.c@@ -1888,11 +1888,9 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"\nAND (c.relam = %u OR NOT ep.heap_only OR ep.rel_regex IS NULL)"
"\nAND (c.relam = %u OR NOT ep.btree_only OR ep.rel_regex IS NULL)",
HEAP_TABLE_AM_OID, BTREE_AM_OID);
-+ appendPQExpBufferStr(&sql, "\nWHERE c.relpersistence != 't'");
if (opts.excludetbl || opts.excludeidx || opts.excludensp)
- appendPQExpBufferStr(&sql, "\nWHERE ep.pattern_id IS NULL");- else- appendPQExpBufferStr(&sql, "\nWHERE true");+ appendPQExpBufferStr(&sql, "\nAND ep.pattern_id IS NULL");
/*
* We need to be careful not to break the --no-dependent-toast and
@@ -1944,7 +1942,8 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"\nON ('pg_toast' ~ ep.nsp_regex OR ep.nsp_regex IS NULL)"
"\nAND (t.relname ~ ep.rel_regex OR ep.rel_regex IS NULL)"
"\nAND ep.heap_only"
- "\nWHERE ep.pattern_id IS NULL");+ "\nWHERE ep.pattern_id IS NULL"+ "\nAND t.relpersistence != 't'");
appendPQExpBufferStr(&sql,
"\n)");
}
@@ -1962,7 +1961,8 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"\nINNER JOIN pg_catalog.pg_index i "
"ON r.oid = i.indrelid "
"INNER JOIN pg_catalog.pg_class c "
- "ON i.indexrelid = c.oid");+ "ON i.indexrelid = c.oid "+ "AND c.relpersistence != 't'");
if (opts.excludeidx || opts.excludensp)
appendPQExpBufferStr(&sql,
"\nINNER JOIN pg_catalog.pg_namespace n "
@@ -2000,7 +2000,8 @@ compile_relation_list_one_db(PGconn *conn, SimplePtrList *relations,
"INNER JOIN pg_catalog.pg_index i "
"ON t.oid = i.indrelid"
"\nINNER JOIN pg_catalog.pg_class c "
- "ON i.indexrelid = c.oid");+ "ON i.indexrelid = c.oid "+ "AND c.relpersistence != 't'");
if (opts.excludeidx)
appendPQExpBufferStr(&sql,
"\nLEFT OUTER JOIN exclude_pat ep "
diff --git a/src/bin/pg_amcheck/t/003_check.pl b/src/bin/pg_amcheck/t/003_check.plindex c26e5eda67..de7814089e 100644--- a/src/bin/pg_amcheck/t/003_check.pl+++ b/src/bin/pg_amcheck/t/003_check.pl@@ -337,6 +337,32 @@ $node->command_checks_all([ @cmd, '-d', 'db1', '-d', 'db2', '-d', 'db3' ],
#
perform_all_corruptions();
+# Hold open an unrelated session with temporary tables defined, and indexes+# over them defined, to verify that pg_amcheck does not fail attempting to+# check them. Check both temporary objects whose creation has been committed+# and not.+#+# BUG #17212: pg_amcheck fails on checking temporary relations+#+my ($sql_1, $sql_2) = map { qq(+CREATE TEMPORARY TABLE empty_tmp$_ (i INTEGER) ON COMMIT PRESERVE ROWS;+CREATE TEMPORARY TABLE data_tmp$_ (i INTEGER[], b BOX) ON COMMIT PRESERVE ROWS;+INSERT INTO data_tmp$_ (i, b) VALUES (ARRAY[1,2,3]::INTEGER[], '((1,2),(3,4))'::BOX);+CREATE INDEX brin_tmp$_ ON data_tmp$_ USING BRIN (b);+CREATE INDEX btree_tmp$_ ON data_tmp$_ USING BTREE (i);+CREATE INDEX gin_tmp$_ ON data_tmp$_ USING GIN (i);+CREATE INDEX gist_tmp$_ ON data_tmp$_ USING GIST (b);+CREATE INDEX hash_tmp$_ ON data_tmp$_ USING HASH (i);+) } (1, 2);+my $in = '';+my $out = '';+my $timer = IPC::Run::timeout(180);+my $h = $node->background_psql('db1', \$in, \$out, $timer);+$in .= qq{+BEGIN; $sql_1; COMMIT; -- other sessions see these in the catalogs+BEGIN; $sql_2; -- no commit, so these objects invisible to other sessions+};+$h->pump_nb;
# Checking databases with amcheck installed and corrupt relations, pg_amcheck
# command itself should return exit status = 2, because tables and indexes are
@@ -517,3 +543,10 @@ $node->command_checks_all(
[ @cmd, '-d', 'db1', '-d', 'db2', '-d', 'db3', '-S', 's*' ],
0, [$no_output_re], [$no_output_re],
'pg_amcheck excluding all corrupt schemas');
++# Finish the unrelated session. We could let the test cleanup do this for us,+# but we'd like to verify that nothing pg_amcheck did above breaks this other+# session.+$in = 'COMMIT;';+$h->pump_nb;+$h->finish;--
2.21.1 (Apple Git-122.3)
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: mark.dilger@enterprisedb.com, pgsql-hackers@postgresql.org, exclusion@gmail.com, robertmhaas@gmail.com, pg@bowt.ie
Subject: Re: BUG #17212: pg_amcheck fails on checking temporary relations
In-Reply-To: <E283B45F-5745-49C9-BBC7-99876E2FABD1@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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