Thread: Wrong value in metapage of GIN INDEX.

Dear Hackers.

Kuroda-san and I are interested in the GIN index and have been testing
various things.
While testing, we are found a little bug.
Some cases, the value of nEntries in the metapage was set to the wrong value.

This is a reproduce of bug situation.
=# SET maintenance_work_mem TO '1MB';
=# CREATE TABLE foo(i jsonb);
=# INSERT INTO foo(i) select jsonb_build_object('foobar001', i) FROM
generate_series(1, 10000) AS i;

# Input the same value again.
=# INSERT INTO foo(i) select jsonb_build_object('foobar001', i) FROM
generate_series(1, 10000) AS i;
# Creates GIN Index.
=# CREATE INDEX foo_idx ON foo USING gin (i jsonb_ops);


=# SELECT * FROM gin_metapage_info(get_raw_page('foo_idx', 0)) WITH
(fastupdate=off);
-[ RECORD 1 ]----+-----------
pending_head     | 4294967295
pending_tail     | 4294967295
tail_free_size   | 0
n_pending_pages  | 0
n_pending_tuples | 0
n_total_pages    | 74
n_entry_pages    | 69
n_data_pages     | 4
n_entries        | 20004 <--★
version          | 2

In this example, the nentries value should be 10001 because the gin
index stores duplicate values in one leaf(posting tree or posting
list).
But, if look at the nentries value of metapage using pageinspect, it
is stored as 20004.
So, Let's run the vacuum.


=# VACUUM foo;
=# SELECT * FROM gin_metapage_info(get_raw_page('foo_idx', 0));
-[ RECORD 1 ]----+-----------
pending_head     | 4294967295
pending_tail     | 4294967295
tail_free_size   | 0
n_pending_pages  | 0
n_pending_tuples | 0
n_total_pages    | 74
n_entry_pages    | 69
n_data_pages     | 4
n_entries        | 10001 <--★
version          | 2

Ah. Run to the vacuum, nEntries is changing the normal value.

There is a problem with the ginEntryInsert function. That calls the
table scan when creating the gin index, ginBuildCallback function
stores the new heap value inside buildstate struct.
And next step, If GinBuildState struct is the size of the memory to be
using is equal to or larger than the maintenance_work_mem value, run
to input value into the GIN index.
This process is a function called ginEnctryInsert.
The ginEntryInsert function called at this time determines that a new
entry is added and increase　the value of nEntries.
However currently, ginEntryInsert is first to increase in　the value of
nEntries, and to determine if there are the same entries in the
current GIN index.
That causes the bug.

The patch is very simple.
Fix to increase the value of nEntries only when a non-duplicate GIN
index leaf added.

This bug detection and code fix worked with Kuroda-san.

Best Regards.
Moon.

Moon-san, kuroda.keisuke-san

On Thu, Aug 29, 2019 at 8:20 AM, Moon, Insung wrote:
> =# CREATE INDEX foo_idx ON foo USING gin (i jsonb_ops);
> =# SELECT * FROM gin_metapage_info(get_raw_page('foo_idx', 0)) WITH (fastupdate=off);

This is not important thing but some mistakes are here.

=# CREATE INDEX foo_idx ON foo USING gin (i jsonb_ops) WITH (fastupdate=off);
=# SELECT * FROM gin_metapage_info(get_raw_page('foo_idx', 0));


> In this example, the nentries value should be 10001 because the gin index stores duplicate values in one
leaf(posting
> tree or posting list).
...
> The patch is very simple.
> Fix to increase the value of nEntries only when a non-duplicate GIN index leaf added.

Does nentries show the number of entries in the leaf pages?
If so, the fix seems to be correct.

--
Yoshikazu Imai

"imai.yoshikazu@fujitsu.com" <imai.yoshikazu@fujitsu.com> writes:
> Moon-san, kuroda.keisuke-san
> On Thu, Aug 29, 2019 at 8:20 AM, Moon, Insung wrote:
>> The patch is very simple.
>> Fix to increase the value of nEntries only when a non-duplicate GIN index leaf added.

> Does nentries show the number of entries in the leaf pages?
> If so, the fix seems to be correct.

I looked at this issue.  The code in ginEntryInsert is not obviously wrong
by itself; it depends on what you think nEntries is supposed to count.
However, ginvacuum.c updates nEntries to the sum of PageGetMaxOffsetNumber()
across all the index's leaf pages, ie the number of surviving leaf items.

It's hard to see how ginvacuum could reverse-engineer a value that would
match what ginEntryInsert is doing, so probably we ought to define
nEntries as the number of leaf items, which seems to make the proposed
patch correct.  (It could use a bit more commentary though.)

I'm inclined to apply this to HEAD only; it doesn't seem significant
enough to justify back-patching.

            regards, tom lane

Dear Tom Lane.

On Tue, Nov 5, 2019 at 3:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> "imai.yoshikazu@fujitsu.com" <imai.yoshikazu@fujitsu.com> writes:
> > Moon-san, kuroda.keisuke-san
> > On Thu, Aug 29, 2019 at 8:20 AM, Moon, Insung wrote:
> >> The patch is very simple.
> >> Fix to increase the value of nEntries only when a non-duplicate GIN index leaf added.
>
> > Does nentries show the number of entries in the leaf pages?
> > If so, the fix seems to be correct.
>
> I looked at this issue.  The code in ginEntryInsert is not obviously wrong
> by itself; it depends on what you think nEntries is supposed to count.
> However, ginvacuum.c updates nEntries to the sum of PageGetMaxOffsetNumber()
> across all the index's leaf pages, ie the number of surviving leaf items.
>
> It's hard to see how ginvacuum could reverse-engineer a value that would
> match what ginEntryInsert is doing, so probably we ought to define
> nEntries as the number of leaf items, which seems to make the proposed
> patch correct.  (It could use a bit more commentary though.)
>
> I'm inclined to apply this to HEAD only; it doesn't seem significant
> enough to justify back-patching.

Thank you for review and push to patch.
Yes. I don't think it's a bug that has a big impact like your opinion.

Best regards.
Moon.

>
>                         regards, tom lane