Re: refactoring relation extension and BufferAlloc(), faster COPY

Hi,

On 2023-02-21 18:18:02 +0200, Heikki Linnakangas wrote:
> > v2-0007-bufmgr-Move-relation-extension-handling-into-Bulk.patch
>
> > +static BlockNumber
> > +BulkExtendSharedRelationBuffered(Relation rel,
> > + SMgrRelation smgr,
> > + bool skip_extension_lock,
> > + char relpersistence,
> > + ForkNumber fork, ReadBufferMode mode,
> > + BufferAccessStrategy strategy,
> > + uint32 *num_pages,
> > + uint32 num_locked_pages,
> > + Buffer *buffers)
>
> Ugh, that's a lot of arguments, some are inputs and some are outputs. I
> don't have any concrete suggestions, but could we simplify this somehow?
> Needs a comment at least.

Yea. I think this is the part of the patchset I like the least.

The ugliest bit is accepting both rel and smgr. The background to that is that
we need the relation oid to acquire the extension lock. But during crash
recovery we don't have that - which is fine, because we don't need the
extension lock.

We could have two different of functions, but that ends up a mess as well, as
we've seen in other cases.

> > v2-0008-Convert-a-few-places-to-ExtendRelationBuffered.patch
>
> > diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
> > index de1427a1e0e..1810f7ebfef 100644
> > --- a/src/backend/access/brin/brin.c
> > +++ b/src/backend/access/brin/brin.c
> > @@ -829,9 +829,11 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo)
> > * whole relation will be rolled back.
> > */
> > - meta = ReadBuffer(index, P_NEW);
> > + meta = ExtendRelationBuffered(index, NULL, true,
> > + index->rd_rel->relpersistence,
> > + MAIN_FORKNUM, RBM_ZERO_AND_LOCK,
> > + NULL);
> > Assert(BufferGetBlockNumber(meta) == BRIN_METAPAGE_BLKNO);
> > - LockBuffer(meta, BUFFER_LOCK_EXCLUSIVE);
> > brin_metapage_init(BufferGetPage(meta), BrinGetPagesPerRange(index),
> > BRIN_CURRENT_VERSION);
>
> Since we're changing the API anyway, how about introducing a new function
> for this case where we extend the relation but we what block number we're
> going to get? This pattern of using P_NEW and asserting the result has
> always felt awkward to me.

To me it always felt like a code smell that some code insists on specific
getting specific block numbers with P_NEW. I guess it's ok for things like
building a new index, but outside of that it feels wrong.

The first case I found just now is revmap_physical_extend(). Which seems to
extend the relation while holding an lwlock. Ugh.

Maybe ExtendRelationBufferedTo() or something like that? With a big comment
saying that users of it are likely bad ;)

> > - buf = ReadBuffer(irel, P_NEW);
> > + buf = ExtendRelationBuffered(irel, NULL, false,
> > + irel->rd_rel->relpersistence,
> > + MAIN_FORKNUM, RBM_ZERO_AND_LOCK,
> > + NULL);
>
> These new calls are pretty verbose, compared to ReadBuffer(rel, P_NEW). I'd
> suggest something like:

I guess. Not sure if it's worth optimizing for brevity all that much here -
there's not that many places extending relations. Several places end up with
less code, actually , because they don't need to care about the extension lock
themselves anymore. I think an ExtendBuffer() that doesn't mention the fork,
etc, ends up being more confusing than helpful.

> buf = ExtendBuffer(rel);

Without the relation in the name it just seems confusing to me - the extension
isn't "restricted" to shared_buffers. ReadBuffer() isn't great as a name
either, but it makes a bit more sense at least, it reads into a buffer. And
it's a vastly more frequent operation, so optimizing for density is worth it.

> Do other ReadBufferModes than RBM_ZERO_AND_LOCK make sense with
> ExtendRelationBuffered?

Hm. That's a a good point. Probably not. Perhaps it could be useful to support
RBM_NORMAL as well? But even if, it'd just be a lock release away if we always
used RBM_ZERO_AND_LOCK.

> Is it ever possible to call this without a relcache entry? WAL redo
> functions do that with ReadBuffer, but they only extend a relation
> implicitly, by replay a record for a particular block.

I think we should use it for crash recovery as well, but the patch doesn't
yet. We have some gnarly code there, see the loop using P_NEW in
XLogReadBufferExtended(). Extending the file one-by-one is a lot more
expensive than doing it in bulk.

> All of the above comments are around the BulkExtendRelationBuffered()
> function's API. That needs a closer look and a more thought-out design to
> make it nice. Aside from that, this approach seems valid.

Thanks for looking! I agree that it can stand a fair bit of polishing...

Greetings,

Andres Freund