On 2017/03/07 10:49, Michael Paquier wrote:
> On Tue, Mar 7, 2017 at 10:37 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> On 2017/03/07 7:28, Tom Lane wrote:
>>> Kevin Grittner <kgri...@gmail.com> writes:
>>>> With e434ad39ae7316bcf35fd578dd34ad7e1ff3c25f I did a `make world`,
>>>> `make install-world`, a fresh default initdb, a start with default
>>>> config, `make installcheck`, connected to the regression database
>>>> with psql as the initial superuser, and ran:
>>>
>>>> regression=# vacuum freeze analyze;
>>>> WARNING: relcache reference leak: relation "p1" not closed
>>>> VACUUM
>>>
>>> p1 is a partitioned table. (BTW, could I lobby for people not to use such
>>> generic, collision-prone names for tables that will be left behind after
>>> the regression tests?) Also, I find that "vacuum analyze" is sufficient,
>>> or even just "analyze", or "analyze p1". I think it's highly likely this
>>> was introduced by 3c3bb99330aa9b4c2f6258bfa0265d806bf365c3. Certainly
>>> that failed to add appropriate regression test cases, or we would have
>>> noticed this already.
>>
>> That's right, sorry about that. Attached patch fixes the relcache leak
>> and adds tests in vacuum.sql and truncate.sql.
>
> (I was just poking at that)
> if (childrel != onerel)
> heap_close(childrel, AccessShareLock);
> + else
> + heap_close(childrel, NoLock);
> continue;
> Shouldn't that be conditional on the relkind of childrel?
I think we could simply Assert that childrel is partitioned table in this
whole block. A child table could be a regular table, a materialized view
(?), a foreign table and a partitioned table, the first three of which are
handled by the first two blocks.
Updated patch attached.
Also, I found out that alter_table.sql mistakenly forgot to drop
partitioned table "p1". Patch 0002 takes care of that.
Thanks,
Amit
>From 0fe3bcea9424133b8711bdcc23b1432cde4fc394 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 7 Mar 2017 10:26:16 +0900
Subject: [PATCH 1/2] Fix relcache ref leak in acquire_inherited_sample_rows
Add tests for vacuum, analyze, truncate on partitioned table, as
3c3bb9933 should have.
---
src/backend/commands/analyze.c | 7 +++++--
src/test/regress/expected/truncate.out | 6 ++++++
src/test/regress/expected/vacuum.out | 9 +++++++++
src/test/regress/sql/truncate.sql | 7 +++++++
src/test/regress/sql/vacuum.sql | 10 ++++++++++
5 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index a70c760341..b91df986c5 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1360,11 +1360,14 @@ acquire_inherited_sample_rows(Relation onerel, int elevel,
else
{
/*
- * ignore, but release the lock on it. could be a partitioned
- * table.
+ * ignore, but release the lock on it. don't try to unlock the
+ * passed-in relation
*/
+ Assert(childrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
if (childrel != onerel)
heap_close(childrel, AccessShareLock);
+ else
+ heap_close(childrel, NoLock);
continue;
}
diff --git a/src/test/regress/expected/truncate.out b/src/test/regress/expected/truncate.out
index 5c5277e0f1..81612d8c88 100644
--- a/src/test/regress/expected/truncate.out
+++ b/src/test/regress/expected/truncate.out
@@ -420,3 +420,9 @@ SELECT nextval('truncate_a_id1'); -- fail, seq should have been dropped
ERROR: relation "truncate_a_id1" does not exist
LINE 1: SELECT nextval('truncate_a_id1');
^
+-- partitioned table
+CREATE TABLE truncparted (a int, b char) PARTITION BY LIST (a);
+CREATE TABLE truncparted1 PARTITION OF truncparted FOR VALUES IN (1);
+INSERT INTO truncparted VALUES (1, 'a');
+TRUNCATE truncparted;
+DROP TABLE truncparted;
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 9b604be4b6..6f68663087 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -82,3 +82,12 @@ VACUUM FULL vactst;
VACUUM (DISABLE_PAGE_SKIPPING) vaccluster;
DROP TABLE vaccluster;
DROP TABLE vactst;
+-- partitioned table
+CREATE TABLE vacparted (a int, b char) PARTITION BY LIST (a);
+CREATE TABLE vacparted1 PARTITION OF vacparted FOR VALUES IN (1);
+INSERT INTO vacparted VALUES (1, 'a');
+UPDATE vacparted SET b = 'b';
+VACUUM (ANALYZE) vacparted;
+VACUUM (FULL) vacparted;
+VACUUM (FREEZE) vacparted;
+DROP TABLE vacparted;
diff --git a/src/test/regress/sql/truncate.sql b/src/test/regress/sql/truncate.sql
index a3d6f5368f..d61eea1a42 100644
--- a/src/test/regress/sql/truncate.sql
+++ b/src/test/regress/sql/truncate.sql
@@ -215,3 +215,10 @@ SELECT * FROM truncate_a;
DROP TABLE truncate_a;
SELECT nextval('truncate_a_id1'); -- fail, seq should have been dropped
+
+-- partitioned table
+CREATE TABLE truncparted (a int, b char) PARTITION BY LIST (a);
+CREATE TABLE truncparted1 PARTITION OF truncparted FOR VALUES IN (1);
+INSERT INTO truncparted VALUES (1, 'a');
+TRUNCATE truncparted;
+DROP TABLE truncparted;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 7b819f654d..7c5fb04917 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -64,3 +64,13 @@ VACUUM (DISABLE_PAGE_SKIPPING) vaccluster;
DROP TABLE vaccluster;
DROP TABLE vactst;
+
+-- partitioned table
+CREATE TABLE vacparted (a int, b char) PARTITION BY LIST (a);
+CREATE TABLE vacparted1 PARTITION OF vacparted FOR VALUES IN (1);
+INSERT INTO vacparted VALUES (1, 'a');
+UPDATE vacparted SET b = 'b';
+VACUUM (ANALYZE) vacparted;
+VACUUM (FULL) vacparted;
+VACUUM (FREEZE) vacparted;
+DROP TABLE vacparted;
--
2.11.0
>From 19a2e5d76a048bf7f761376471947366bffbd6c4 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 7 Mar 2017 13:45:10 +0900
Subject: [PATCH 2/2] Drop a table mistakenly left behind by alter_table.sql
---
src/test/regress/expected/alter_table.out | 1 +
src/test/regress/sql/alter_table.sql | 1 +
2 files changed, 2 insertions(+)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index c15bbdcbd1..2227f2d977 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3371,3 +3371,4 @@ alter table p attach partition p1 for values from (1, 2) to (1, 10);
ERROR: partition constraint is violated by some row
-- cleanup
drop table p;
+drop table p1;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 37f327bf6d..8cd6786a90 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2227,3 +2227,4 @@ alter table p attach partition p1 for values from (1, 2) to (1, 10);
-- cleanup
drop table p;
+drop table p1;
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
