Re: WIP: Avoid creation of the free space map for small tables

From: John Naylor <jcnaylor(at)gmail(dot)com>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: WIP: Avoid creation of the free space map for small tables
Date: 2018-11-23 06:26:43
Message-ID: CAJVSVGV2AkUm5=Q8A60muy+kX28RsSsCQM6_bHEyUm2W4TMpaw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 11/16/18, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

I've attached v8, which includes the 2-state map and addresses the points below:

> Some assorted comments:
> 1.
> <para>
> -Each heap and index relation, except for hash indexes, has a Free Space
> Map
> +Each heap relation, unless it is very small, and each index relation,
> +except for hash indexes, has a Free Space Map
> (FSM) to keep track of available space in the relation. It's stored
>
> It appears that line has ended abruptly.

Revised.

> 2.
> page = BufferGetPage(buffer);
> + targetBlock = BufferGetBlockNumber(buffer);
>
> if (!PageIsNew(page))
> elog(ERROR, "page %u of relation \"%s\" should be empty but is not",
> - BufferGetBlockNumber(buffer),
> + targetBlock,
> RelationGetRelationName(relation));
>
> PageInit(page, BufferGetPageSize(buffer), 0);
> @@ -623,7 +641,18 @@ loop:
> * current backend to make more insertions or not, which is probably a
> * good bet most of the time. So for now, don't add it to FSM yet.
> */
> - RelationSetTargetBlock(relation, BufferGetBlockNumber(buffer));
> + RelationSetTargetBlock(relation, targetBlock);
>
> Is this related to this patch? If not, I suggest let's do it
> separately if required.
>
> 3.
> static int fsm_set_and_search(Relation rel, FSMAddress addr, uint16 slot,
> - uint8 newValue, uint8 minValue);
> + uint8 newValue, uint8 minValue);
>
> This appears to be a spurious change.

2 and 3 are separated into 0001.

> 4.
> @@ -378,24 +386,15 @@ RelationGetBufferForTuple(Relation relation, Size
> len,
> * target.
> */
> targetBlock = GetPageWithFreeSpace(relation, len + saveFreeSpace);
> +
> + /*
> + * In case we used an in-memory map of available blocks, reset
> + * it for next use.
> + */
> + if (targetBlock < HEAP_FSM_CREATION_THRESHOLD)
> + ClearLocalMap();
>
> How will you clear the local map during error? I think you need to
> clear it in abort path and you can name the function as
> FSMClearLocalMap or something like that.

Done. I've put this call last before abort processing.

> 5.
> +/*#define TRACE_TARGETBLOCK */
>
> Debugging leftover, do you want to retain this and related stuff
> during the development of patch?

I have kept it aside as a separate patch but not attached it for now.

Also, we don't quite have a consensus on the threshold value, but I
have set it to 4 pages for v8. If this is still considered too
expensive (and basic tests show it shouldn't be), I suspect it'd be
better to interleave the available block numbers as described a couple
days ago than lower the threshold further.

I have looked at zhio.c, and it seems trivial to adapt zheap to this patchset.

-John Naylor

Attachment Content-Type Size
v8-0001-Minor-cosmetic-adjustments-for-consistency.patch text/x-patch 1.9 KB
v8-0002-Avoid-creation-of-the-free-space-map-for-small-ta.patch text/x-patch 28.9 KB
v8-0003-During-pg_upgrade-skip-transfer-of-FSMs-if-they.patch text/x-patch 7.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2018-11-23 07:39:13 Re: Accounting of zero-filled buffers in EXPLAIN (BUFFERS)
Previous Message Thomas Munro 2018-11-23 04:45:18 Re: Refactoring the checkpointer's fsync request queue