Horton

Thread

Commits

  1. Fix ginEntryInsert's counting of GIN leaf tuples.

  1. Wrong value in metapage of GIN INDEX.

    Moon, Insung <tsukiwamoon.pgsql@gmail.com> — 2019-08-29T08:19:51Z

    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.

  2. Re: Wrong value in metapage of GIN INDEX.

    Keisuke Kuroda <keisuke.kuroda.3862@gmail.com> — 2019-08-30T02:48:47Z

    Hi Moon-san.

Thank you for posting.

We are testing the GIN index onJSONB type.
The default maintenance_work_mem (64MB) was fine in usually cases.
However, this problem occurs when indexing very large JSONB data.

best regards,
Keisuke Kuroda

2019年8月29日(木) 17:20 Moon, Insung <tsukiwamoon.pgsql@gmail.com>:

> 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.
>

  3. RE: Wrong value in metapage of GIN INDEX.

    imai.yoshikazu@fujitsu.com <imai.yoshikazu@fujitsu.com> — 2019-10-08T06:46:10Z

    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

  4. Re: Wrong value in metapage of GIN INDEX.

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-04T18:55:02Z

    "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

  5. Re: Wrong value in metapage of GIN INDEX.

    Moon, Insung <tsukiwamoon.pgsql@gmail.com> — 2019-11-05T08:21:14Z

    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