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

On Sun, Dec 30, 2018 at 10:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sun, Dec 30, 2018 at 3:49 AM John Naylor <jcnaylor(at)gmail(dot)com> wrote:
> > > I also tried to insert more
> > > records till 8 pages and same regression is observed! So I guess even
> > > HEAP_FSM_CREATION_THRESHOLD = 4 is not perfect!
> >
> > That's curious, because once the table exceeds the threshold, it would
> > be allowed to update the FSM, and in the process write 3 pages that it
> > didn't have to in the 4 page test. The master branch has the FSM
> > already, so I would expect the 8 page case to regress more.
> >
>
> It is not clear to me why you think there should be regression at 8
> pages when HEAP_FSM_CREATION_THRESHOLD is 4. Basically, once FSM
> starts getting updated, we should be same as HEAD as it won't take any
> special path?

In this particular test, the FSM is already created ahead of time for
the master branch, so we can compare accessing FSM versus checking
every page. My reasoning is that passing the threshold would take some
time to create 3 FSM pages with the patch, leading to a larger
regression. It seems we don't observe this, however.

On Sun, Dec 30, 2018 at 10:59 PM Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
> I have some minor comments for pg_upgrade patch
> 1. Now we call stat main fork file in transfer_relfile()
> + sret = stat(old_file, &statbuf);
>
> + /* Save the size of the first segment of the main fork. */
> + if (type_suffix[0] == '\0' && segno == 0)
> + first_seg_size = statbuf.st_size;
>
> But we do not handle the case if stat has returned any error!

How about this:

/* Did file open fail? */
if (stat(old_file, &statbuf) != 0)
{
/* Extent, fsm, or vm does not exist? That's OK, just return */
if (errno == ENOENT &&
(type_suffix[0] != '\0' || segno != 0))
return first_seg_size;
else
pg_fatal("error while checking for file existence \"%s.%s\"
(\"%s\" to \"%s\"): %s\n",
map->nspname, map->relname, old_file, new_file,
strerror(errno));
}

/* Save the size of the first segment of the main fork. */
else if (type_suffix[0] == '\0' && segno == 0)
first_seg_size = statbuf.st_size;

/* If extent, fsm, or vm is empty, just return */
else if (statbuf.st_size == 0)
return first_seg_size;

> 2. src/bin/pg_upgrade/pg_upgrade.h
>
> char *relname;
> +
> + char relkind; /* relation relkind -- see pg_class.h */
>
> I think we can remove the added empty line.

In the full context:

- /* the rest are used only for logging and error reporting */
+
+ /* These are used only for logging and error reporting. */
char *nspname; /* namespaces */
char *relname;
+
+ char relkind; /* relation relkind -- see pg_class.h */

Relkind is not used for logging or error reporting, so the space sets
it apart from the previous members. I could instead put relkind before
those other two...

-John Naylor