Thread: [HACKERS] WARNING: relcache reference leak: relation "p1" not closed
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 -- Kevin Grittner
Kevin Grittner <kgrittn@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.
regards, tom lane
On 2017/03/07 7:28, Tom Lane wrote: > Kevin Grittner <kgrittn@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. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Tue, Mar 7, 2017 at 10:37 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > On 2017/03/07 7:28, Tom Lane wrote: >> Kevin Grittner <kgrittn@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? -- Michael
On 2017/03/07 10:49, Michael Paquier wrote: > On Tue, Mar 7, 2017 at 10:37 AM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> On 2017/03/07 7:28, Tom Lane wrote: >>> Kevin Grittner <kgrittn@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> Also, I found out that alter_table.sql mistakenly forgot to drop
> partitioned table "p1". Patch 0002 takes care of that.
While that might or might not have been intentional, I think it's an
astoundingly bad idea to not leave any partitioned tables behind in
the final state of the regression database. Doing so would likely
have meant that this particular bug evaded detection for much longer
than it did. Moreover, it would mean that the pg_upgrade test would
have exactly no coverage of partitioned cases.
Therefore, there should definitely be a partitioned table, hopefully with
a less generic name than "p1", in the final regression DB state. Whether
this particular one from alter_table.sql is a good candidate, I dunno.
But let's not drop it without adding a better-thought-out replacement.
regards, tom lane
On 2017/03/07 14:04, Tom Lane wrote: > Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes: >> Also, I found out that alter_table.sql mistakenly forgot to drop >> partitioned table "p1". Patch 0002 takes care of that. > > While that might or might not have been intentional, I think it's an > astoundingly bad idea to not leave any partitioned tables behind in > the final state of the regression database. Doing so would likely > have meant that this particular bug evaded detection for much longer > than it did. Moreover, it would mean that the pg_upgrade test would > have exactly no coverage of partitioned cases. That's true. Should have been apparent to me. > Therefore, there should definitely be a partitioned table, hopefully with > a less generic name than "p1", in the final regression DB state. Whether > this particular one from alter_table.sql is a good candidate, I dunno. > But let's not drop it without adding a better-thought-out replacement. OK, let's drop p1 in alter_table.sql. I think a partitioned table created in insert.sql is a good candidate to keep around after having it renamed, which patch 0003 does. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Tue, Mar 7, 2017 at 1:43 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > On 2017/03/07 14:04, Tom Lane wrote: >> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes: >>> Also, I found out that alter_table.sql mistakenly forgot to drop >>> partitioned table "p1". Patch 0002 takes care of that. >> >> While that might or might not have been intentional, I think it's an >> astoundingly bad idea to not leave any partitioned tables behind in >> the final state of the regression database. Doing so would likely >> have meant that this particular bug evaded detection for much longer >> than it did. Moreover, it would mean that the pg_upgrade test would >> have exactly no coverage of partitioned cases. > > That's true. Should have been apparent to me. > >> Therefore, there should definitely be a partitioned table, hopefully with >> a less generic name than "p1", in the final regression DB state. Whether >> this particular one from alter_table.sql is a good candidate, I dunno. >> But let's not drop it without adding a better-thought-out replacement. > > OK, let's drop p1 in alter_table.sql. I think a partitioned table created > in insert.sql is a good candidate to keep around after having it renamed, > which patch 0003 does. Committed 0001. Committed 0002 and 0003 together. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2017/03/08 1:34, Robert Haas wrote: > On Tue, Mar 7, 2017 at 1:43 AM, Amit Langote wrote: >> On 2017/03/07 14:04, Tom Lane wrote: >>> Therefore, there should definitely be a partitioned table, hopefully with >>> a less generic name than "p1", in the final regression DB state. Whether >>> this particular one from alter_table.sql is a good candidate, I dunno. >>> But let's not drop it without adding a better-thought-out replacement. >> >> OK, let's drop p1 in alter_table.sql. I think a partitioned table created >> in insert.sql is a good candidate to keep around after having it renamed, >> which patch 0003 does. > > Committed 0001. > > Committed 0002 and 0003 together. Thanks. Regards, Amit