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

From: John Naylor <john(dot)naylor(at)2ndquadrant(dot)com>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: WIP: Avoid creation of the free space map for small tables
Date: 2019-01-25 23:35:30
Message-ID: CACPNZCs9J2vTKtmMPNKts_ONyZyXf18_WtdhUvf2=Na5BfZxRA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Thu, Jan 24, 2019 at 5:19 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 1.
> + if ((maps[mapnum].relkind != RELKIND_RELATION &&
> + maps[mapnum].relkind != RELKIND_TOASTVALUE) ||
> + first_seg_size > HEAP_FSM_CREATION_THRESHOLD * BLCKSZ ||
> + GET_MAJOR_VERSION(new_cluster.major_version) <= 1100)
> + (void) transfer_relfile(&maps[mapnum], "_fsm", vm_must_add_frozenbit);
>
> I think this check will needlessly be performed for future versions as
> well, say when wants to upgrade from PG12 to PG13. That might not
> create any problem, but let's try to be more precise. Can you try to
> rewrite this check? You might want to encapsulate it inside a
> function. I have thought of doing something similar to what we do for
> vm, see checks relate to VISIBILITY_MAP_FROZEN_BIT_CAT_VER, but I
> guess for this patch it is not important to check catalog version as
> even if someone tries to upgrade to the same version.

Agreed, done for v19 (I've only attached the pg_upgrade patch).

> 2.
> transfer_relfile()
> {
> ..
> - /* Is it an extent, fsm, or vm file? */
> - if (type_suffix[0] != '\0' || segno != 0)
> + /* Did file open fail? */
> + if (stat(old_file, &statbuf) != 0)
> ..
> }
>
> So from now onwards, we will call stat for even 0th segment which
> means there is one additional system call for each relation, not sure
> if that matters, but I think there is no harm in once testing with a
> large number of relations say 10K to 50K relations which have FSM.

Performance testing is probably a good idea anyway, but I went ahead
and implemented your next idea:

> The other alternative is we can fetch pg_class.relpages and rely on
> that to take this decision, but again if that is not updated, we might
> take the wrong decision.

We can think of it this way: Which is worse,
1. Transferring a FSM we don't need, or
2. Skipping a FSM we need

I'd say #2 is worse. So, in v19 we check pg_class.relpages and if it's
a heap and less than or equal the threshold we call stat on the 0th
segment to verify. In the common case, the cost of the stat call is
offset by not linking the FSM. Despite needing another pg_class field,
I think this code is actually easier to read than my earlier versions.

> 3.
> -static void
> +static Size
> transfer_relfile(FileNameMap *map, const char *type_suffix, bool
> vm_must_add_frozenbit)
>
> If we decide to go with the approach proposed by you, we should add
> some comments atop this function for return value change?

Done, as well as other comment edits.

--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment Content-Type Size
v19-0003-During-pg_upgrade-conditionally-skip-transfer-of.patch text/x-patch 8.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message John Naylor 2019-01-25 23:38:50 Re: WIP: Avoid creation of the free space map for small tables
Previous Message Bruce Momjian 2019-01-25 23:34:17 Re: GUC parameters accepts values in both octal and hexadecimal formats