Re: gincostestimate

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer(at)wulczer(dot)org> writes:
> [ review of gincostestimate-0.19 ]

I went through this patch, re-synced with current HEAD, and did some
minor editorializing; a new version is attached. (Caution: I have not
tested this beyond verifying that it still compiles.)

> Codewise I have one question: the patch changes a loop in
> ginvacuumcleanup from
> for (blkno = GIN_ROOT_BLKNO + 1; blkno < npages; blkno++)
> to
> for (blkno = GIN_ROOT_BLKNO; blkno < npages; blkno++)
> why should it now go through all blocks?

I think this is correct. Before, vacuum had nothing useful to do on the
root page so it just skipped it. Now, it needs to count the root page
in the appropriate way in the stats it's gathering. The previous coding
maybe could have used a comment, but this version is unsurprising.

> The patch has lots of statements like if ( GinPageIsLeaf(page) ), that
> is with extra space between the outer parenthesis and the condition,
> which AFAIK is not the project style. I guess pgindent fixes that, so
> it's no big deal.

> There are lines with elog(NOTICE) that are #if 0, they probably should
> either become elog(DEBUGX) or get removed.

I fixed the latter and cleaned up some of the formatting violations,
though not all. I dunno if anyone feels like running pgindent on the
patch at the moment.

I think there are two big problems left before this patch can be
applied:

1. The use of rd_amcache is very questionable. There's no provision for
updates executed by one session to get reflected into stats already
cached in another session. You could fix that by forcing relcache
flushes whenever you update the stats, as btree does:

/* send out relcache inval for metapage change */
if (!InRecovery)
CacheInvalidateRelcache(rel);

However I think that's probably a Really Bad Idea, because it would
result in extremely frequent relcache flushes, and those are expensive.
(The reason this mechanism is good for btree is it only needs to update
its cache after a root page split, which is infrequent.) My advice is
to drop the use of rd_amcache completely, and just have the code read
the metapage every time gincostestimate runs. If you don't like that
then you're going to need to think hard about how often to update the
cache and what can drive that operation at the right times.

BTW, another problem that would need to be fixed if you keep this code
is that ginUpdateStatInPlace wants to force the new values into
rd_amcache, which (a) is pretty useless and (b) risks a PANIC on
palloc failure, because it's called from a critical section.

2. Some of the calculations in gincostestimate seem completely bogus.
In particular, this bit:

/* calc partial match: we don't need a search but an index scan */
*indexStartupCost += partialEntriesInQuals * numEntryPages / numEntries;

is surely wrong, because the number being added to indexStartupCost is
a pure ratio not scaled by any cost unit. I don't understand what this
number is supposed to be, so it's not clear what cost variable ought to
be included.

This equation doesn't seem amazingly sane either:

/* cost to scan data pages for each matched entry */
pagesFetched = ceil((searchEntriesInQuals + partialEntriesInQuals) *
numDataPages / numEntries);

This has pagesFetched *decreasing* as numEntries increases, which cannot
be right can it?

Also, right after that step, there's a bit of code that re-estimates
pagesFetched from selectivity and uses the larger value. Fine, but why
are you only applying that idea here and not to the entry-pages
calculation done a few lines earlier?

regards, tom lane