RE: [PATCH] Speedup truncates of relation forks

On Wednesday, June 12, 2019 4:29 PM (GMT+9), Masahiko Sawada wrote:
> On Wed, Jun 12, 2019 at 12:25 PM Tsunakawa, Takayuki
> <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote:
> >
> > From: Tomas Vondra [mailto:tomas(dot)vondra(at)2ndquadrant(dot)com]
> > > Years ago I've implemented an optimization for many DROP TABLE
> > > commands in a single transaction - instead of scanning buffers for
> > > each relation, the code now accumulates a small number of relations
> > > into an array, and then does a bsearch for each buffer.
> > >
> > > Would something like that be applicable/useful here? That is, if we
> > > do multiple TRUNCATE commands in a single transaction, can we
> > > optimize it like this?
> >
> > Unfortunately not. VACUUM and autovacuum handles each table in a different
> transaction.
>
> We do RelationTruncate() also when we truncate heaps that are created in the
> current transactions or has a new relfilenodes in the current transaction.
> So I think there is a room for optimization Thomas suggested, although I'm
> not sure it's a popular use case.

I couldn't think of a use case too.

> I've not look at this patch deeply but in DropRelFileNodeBuffer I think we
> can get the min value of all firstDelBlock and use it as the lower bound of
> block number that we're interested in. That way we can skip checking the array
> during scanning the buffer pool.

I'll take note of this suggestion.
Could you help me expound more on this idea, skipping the internal loop by
comparing the min and buffer descriptor (bufHdr)?

In the current patch, I've implemented the following in DropRelFileNodeBuffers:
for (i = 0; i < NBuffers; i++)
{
...
buf_state = LockBufHdr(bufHdr);
for (k = 0; k < nforks; k++)
{
if (RelFileNodeEquals(bufHdr->tag.rnode, rnode.node) &&
bufHdr->tag.forkNum == forkNum[k] &&
bufHdr->tag.blockNum >= firstDelBlock[k])
{
InvalidateBuffer(bufHdr); /* releases spinlock */
break;
}

> Don't we use each elements of nblocks for each fork? That is, each fork uses
> an element at its fork number in the nblocks array and sets InvalidBlockNumber
> for invalid slots, instead of passing the valid number of elements. That way
> the following code that exist at many places,
>
> blocks[nforks] = visibilitymap_mark_truncate(rel, nblocks);
> if (BlockNumberIsValid(blocks[nforks]))
> {
> forks[nforks] = VISIBILITYMAP_FORKNUM;
> nforks++;
> }
>
> would become
>
> blocks[VISIBILITYMAP_FORKNUM] = visibilitymap_mark_truncate(rel,
> nblocks);

In the patch, we want to truncate all forks' blocks simultaneously, so
we optimize the invalidation of buffers and reduce the number of loops
using those values.
The suggestion above would have to remove the forks array and its
forksize (nforks), is it correct? But I think we’d need the fork array
and nforks to execute the truncation all at once.
If I'm missing something, I'd really appreciate your further comments.

--
Thank you everyone for taking a look at my thread.
I've also already added this patch to the CommitFest app.

Regards,
Kirk Jamison