Re: PATCH: optimized DROP of multiple tables within a transaction

On 2012-12-06 23:38:59 +0100, Tomas Vondra wrote:
> On 6.12.2012 05:47, Shigeru Hanada wrote:
> >> I've done a simple benchmark on my laptop with 2GB shared buffers, it's
> >> attached in the drop-test.py (it's a bit messy, but it works).
> > [snip]
> >> With those parameters, I got these numbers on the laptop:
> >>
> >>   creating 10000 tables
> >>     all tables created in 3.298694 seconds
> >>   dropping 10000 tables one by one ...
> >>     all tables dropped in 32.692478 seconds
> >>   creating 10000 tables
> >>     all tables created in 3.458178 seconds
> >>   dropping 10000 tables in batches by 100...
> >>     all tables dropped in 3.28268 seconds
> >>
> >> So it's 33 seconds vs. 3.3 seconds, i.e. 10x speedup. On AWS we usually
> >> get larger differences, as we use larger shared buffers and the memory
> >> is significantly slower there.
> >
> > Do you have performance numbers of same test on not-patched PG?
> > This patch aims to improve performance of bulk DROP, so 4th number in
> > the result above should be compared to 4th number of not-patched PG?
>
> I've re-run the tests with the current patch on my home workstation, and
> the results are these (again 10k tables, dropped either one-by-one or in
> batches of 100).
>
> 1) unpatched
>
> dropping one-by-one:        15.5 seconds
> dropping in batches of 100: 12.3 sec
>
> 2) patched (v3.1)
>
> dropping one-by-one:        32.8 seconds
> dropping in batches of 100:  3.0 sec
>
> The problem here is that when dropping the tables one-by-one, the
> bsearch overhead is tremendous and significantly increases the runtime.
> I've done a simple check (if dropping a single table, use the original
> simple comparison) and I got this:
>
> 3) patched (v3.2)
>
> dropping one-by-one:        16.0 seconds
> dropping in batches of 100:  3.3 sec
>
> i.e. the best of both. But it seems like an unnecessary complexity to me
> - if you need to drop a lot of tables you'll probably do that in a
> transaction anyway.
>

Imo that's still a pretty bad performance difference. And your
single-table optimization will probably fall short as soon as the table
has indexes and/or a toast table...

> +
> +/*
> + * Used to sort relfilenode array (ordered by [relnode, dbnode, spcnode]), so
> + * that it's suitable for bsearch.
> + */
> +static int
> +rnode_comparator(const void * p1, const void * p2)
> +{
> +    RelFileNodeBackend n1 = * (RelFileNodeBackend *) p1;
> +    RelFileNodeBackend n2 = * (RelFileNodeBackend *) p2;
> +
> +    if (n1.node.relNode < n2.node.relNode)
> +        return -1;
> +    else if (n1.node.relNode > n2.node.relNode)
> +        return 1;
> +
> +    if (n1.node.dbNode < n2.node.dbNode)
> +        return -1;
> +    else if (n1.node.dbNode > n2.node.dbNode)
> +        return 1;
> +
> +    if (n1.node.spcNode < n2.node.spcNode)
> +        return -1;
> +    else if (n1.node.spcNode > n2.node.spcNode)
> +        return 1;
> +    else
> +        return 0;
> +}

ISTM that this whole comparator could be replaced by memcmp(). That
could quite possibly lower the overhead of the bsearch() in simple
cases. We already rely on the fact that RelFileNode's have no padding
atm (c.f. buf_internal.h), so a memcmp() seems fine to me.

Greetings,

Andres Freund

--Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services