Re: Speedup of relation deletes during recovery

On Wed, Jun 27, 2018 at 1:46 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2018-06-27 13:44:03 +1200, Thomas Munro wrote:
>> On further reflection, on the basis that it's the most conservative
>> change, +1 for Fujii-san's close-in-reverse-order idea.  We should
>> reconsider that data structure for 12; there doesn't seems to be a
>> good reason to carry all those comments warning about performance when
>> the O(1) version is shorter than the comments.
>
> Agreed on this.  I like the dlist version more than the earlier one, so
> let's fix it up, for v12+.  But regardless I'd argue that we consider
> disabling that infrastructure while in recovery - it's just unnecessary.

I tested this patch using the functions that Michael provided + an
extra variant truncate_tables(), using the -v2 patch + the adjustment
to close in reverse order.  I set shared_buffers to 4GB and
max_locks_per_transaction to 30000.  I tested three CREATE/DROP code
paths, and also two different TRUNCATE paths:

SELECT create_tables(10000);
SELECT drop_tables(10000);

BEGIN;
SELECT create_tables(10000);
ROLLBACK;

SELECT create_tables(10000);
BEGIN;
SELECT drop_tables(10000);
PREPARE TRANSACTION 'x';
COMMIT PREPARED 'x';

SELECT create_tables(10000);
SELECT truncate_tables(10000);

In all these cases my startup process would eat 100% CPU for ~20
seconds, and with the patch this dropped to ~1-2 second (I didn't
measure precisely, I just watched htop) with the patch.  I also tried
back-patching to 9.3 and the results were the same.

Based on this and positive feedback from other reviewers, I will mark
this "Ready for Committer" (meaning v2 + close-in-reverse-order).  I
did have one cosmetic remark a few messages up (redundant variable
initialisation).

By the way, as noted by others already, there is still a way to reach
a horrible case with or without the patch:

BEGIN;
SELECT create_tables(10000);
SELECT truncate_tables(10000);

That's a different code path that eats a lot of CPU on the *primary*, because:

                /*
                 * Normally, we need a transaction-safe truncation
here.  However, if
                 * the table was either created in the current
(sub)transaction or has
                 * a new relfilenode in the current (sub)transaction,
then we can just
                 * truncate it in-place, because a rollback would
cause the whole
                 * table or the current physical file to be thrown away anyway.
                 */
                if (rel->rd_createSubid == mySubid ||
                        rel->rd_newRelfilenodeSubid == mySubid)
                {
                        /* Immediate, non-rollbackable truncation is OK */
                        heap_truncate_one_rel(rel);
                }
                else

Without range-scannable buffer mapping (Andres's radix tree thing),
that bet doesn't work out too well when you do it more than once.
Hmm... we could just... not do that?

(Has anyone ever looked into a lazier approach to dropping buffers?)

-- 
Thomas Munro
http://www.enterprisedb.com