Re: refactoring relation extension and BufferAlloc(), faster COPY - Mailing list pgsql-hackers

> 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.

> 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.

> -        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:

buf = ExtendBuffer(rel);

Do other ReadBufferModes than RBM_ZERO_AND_LOCK make sense with 
ExtendRelationBuffered?

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.

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.

- Heikki