In pageinspect, perform clean-up after testing gin-related functions
Hello all,
In pageinspect/sql/gin.sql, we don't drop the table test1 at the end
of the test. IMHO, we should clean-up at the end of a test. I've
attached the patch to perform the same.
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
Attachments:
0001-In-pageinspect-drop-table-after-testing-gin-related-.patchtext/x-patch; charset=US-ASCII; name=0001-In-pageinspect-drop-table-after-testing-gin-related-.patchDownload
From 1e4be3749eed9ff9d59f775d2bd4ad2b409a6d3c Mon Sep 17 00:00:00 2001
From: Kuntal Ghosh <kuntal.ghosh@enterprisedb.com>
Date: Wed, 11 Jul 2018 12:21:13 +0530
Subject: [PATCH] In pageinspect, drop table after testing gin-related
functions
---
contrib/pageinspect/expected/gin.out | 1 +
contrib/pageinspect/sql/gin.sql | 2 ++
2 files changed, 3 insertions(+)
diff --git a/contrib/pageinspect/expected/gin.out b/contrib/pageinspect/expected/gin.out
index 82f63b2..ef7570b 100644
--- a/contrib/pageinspect/expected/gin.out
+++ b/contrib/pageinspect/expected/gin.out
@@ -35,3 +35,4 @@ FROM gin_leafpage_items(get_raw_page('test1_y_idx',
-[ RECORD 1 ]
?column? | t
+DROP TABLE test1;
diff --git a/contrib/pageinspect/sql/gin.sql b/contrib/pageinspect/sql/gin.sql
index d516ed3..423f5c5 100644
--- a/contrib/pageinspect/sql/gin.sql
+++ b/contrib/pageinspect/sql/gin.sql
@@ -17,3 +17,5 @@ SELECT COUNT(*) > 0
FROM gin_leafpage_items(get_raw_page('test1_y_idx',
(pg_relation_size('test1_y_idx') /
current_setting('block_size')::bigint)::int - 1));
+
+DROP TABLE test1;
--
1.8.3.1
On Wed, Jul 11, 2018 at 12:37 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
Hello all,
In pageinspect/sql/gin.sql, we don't drop the table test1 at the end
of the test. IMHO, we should clean-up at the end of a test.
Yeah, it is good practice to drop the objects at the end. It is
strange that original commit adfb81d9e1 has this at the end of the
test, but a later commit 367b99bbb1 by Tom has removed the Drop
statement. AFAICS, this is just a silly mistake, but I might be
missing something. Tom, do you remember any reason for doing so? If
not, then I think we can revert back that change (aka commit Kuntal's
patch).
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On 2018-07-11 12:56:49 +0530, Amit Kapila wrote:
On Wed, Jul 11, 2018 at 12:37 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:Hello all,
In pageinspect/sql/gin.sql, we don't drop the table test1 at the end
of the test. IMHO, we should clean-up at the end of a test.Yeah, it is good practice to drop the objects at the end. It is
strange that original commit adfb81d9e1 has this at the end of the
test, but a later commit 367b99bbb1 by Tom has removed the Drop
statement. AFAICS, this is just a silly mistake, but I might be
missing something. Tom, do you remember any reason for doing so? If
not, then I think we can revert back that change (aka commit Kuntal's
patch).
We actually sometimes intentionally want to persist objects past the end
of the test. Allows to test pg_dump / pg_upgrade. Don't know whether
that's the case here, but it's worthwhile to note.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2018-07-11 12:56:49 +0530, Amit Kapila wrote:
Yeah, it is good practice to drop the objects at the end. It is
strange that original commit adfb81d9e1 has this at the end of the
test, but a later commit 367b99bbb1 by Tom has removed the Drop
statement. AFAICS, this is just a silly mistake, but I might be
missing something. Tom, do you remember any reason for doing so? If
not, then I think we can revert back that change (aka commit Kuntal's
patch).
We actually sometimes intentionally want to persist objects past the end
of the test. Allows to test pg_dump / pg_upgrade. Don't know whether
that's the case here, but it's worthwhile to note.
I don't think our pg_dump testbed makes any use of contrib regression
tests, so that's not the reason here. I believe I took out the DROP
because it made it impossible to do additional manual tests after the end
of an installcheck run without laboriously re-creating the test table.
In other words, I disagree with Amit's opinion that it's good practice
to drop everything at the end of a test script. There are often good
reasons to leave the objects available for later use.
regards, tom lane
On Wed, Jul 11, 2018 at 11:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
On 2018-07-11 12:56:49 +0530, Amit Kapila wrote:
Yeah, it is good practice to drop the objects at the end. It is
strange that original commit adfb81d9e1 has this at the end of the
test, but a later commit 367b99bbb1 by Tom has removed the Drop
statement. AFAICS, this is just a silly mistake, but I might be
missing something. Tom, do you remember any reason for doing so? If
not, then I think we can revert back that change (aka commit Kuntal's
patch).We actually sometimes intentionally want to persist objects past the end
of the test. Allows to test pg_dump / pg_upgrade. Don't know whether
that's the case here, but it's worthwhile to note.I don't think our pg_dump testbed makes any use of contrib regression
tests, so that's not the reason here. I believe I took out the DROP
because it made it impossible to do additional manual tests after the end
of an installcheck run without laboriously re-creating the test table.
Fair point, but using a generic name like 'test1' and leaving it can
sometimes cause confusion. In this case, it was not clear by looking
at the test and all the nearby tests (brin, btree and page) uses the
same table name and drops the table at end of the test. The name
conflict doesn't arise because the test for 'gin' was at the end of
those.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com