Horton

Thread

Commits

  1. Give partitioned table "p" in regression tests a less generic name.

  2. Fix relcache reference leak.

  3. Combine several DROP variants into generic DropStmt

  4. Reorder the asynchronous libpq calls for replication connection

  5. Don't uselessly rewrite, truncate, VACUUM, or ANALYZE partitioned tables.

  1. WARNING: relcache reference leak: relation "p1" not closed

    Kevin Grittner <kgrittn@gmail.com> — 2017-03-06T21:11:42Z

    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

  2. Re: WARNING: relcache reference leak: relation "p1" not closed

    Kevin Grittner <kgrittn@gmail.com> — 2017-03-06T21:24:43Z

    [original message held up for review -- should be along eventualy...]

On Mon, Mar 6, 2017 at 3:11 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
> 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

Still present in e6477a8134ace06ef3a45a7ce15813cd398e72d8

-- 
Kevin Grittner

  3. Re: WARNING: relcache reference leak: relation "p1" not closed

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-06T22:28:57Z

    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

  4. Re: WARNING: relcache reference leak: relation "p1" not closed

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-03-07T01:37:46Z

    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

  5. Re: WARNING: relcache reference leak: relation "p1" not closed

    Michael Paquier <michael.paquier@gmail.com> — 2017-03-07T01:49:48Z

    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

  6. Re: WARNING: relcache reference leak: relation "p1" not closed

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-03-07T04:55:00Z

    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

  7. Re: WARNING: relcache reference leak: relation "p1" not closed

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-07T05:04:42Z

    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

  8. Re: WARNING: relcache reference leak: relation "p1" not closed

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-03-07T06:43:05Z

    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

  9. Re: WARNING: relcache reference leak: relation "p1" not closed

    Robert Haas <robertmhaas@gmail.com> — 2017-03-07T16:34:52Z

    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

  10. Re: WARNING: relcache reference leak: relation "p1" not closed

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-03-08T01:55:57Z

    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