Thread: 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
Attachment
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