Subject: Re: Collecting statistics about contents of JSONB columns

From: Mahendra Singh Thalor <[email-masked]>
Date: Fri, 11 Mar 2022 22:40:43 +0530

On Fri, 4 Feb 2022 at 08:30, Tomas Vondra <[email-masked]>
wrote:
>
>
>
> On 2/4/22 03:47, Tomas Vondra wrote:
> > ./json-generate.py 30000 2 8 1000 6 1000
>
> Sorry, this should be (different order of parameters):
>
> ./json-generate.py 30000 2 1000 8 6 1000
>

Thanks, Tomas for this test case.

Hi Hackers,

For the last few days, I was doing testing on the top of these JSON
optimizers patches and was taking help fro Tomas Vondra to understand
patches and testing results.
Thanks, Tomas for your feedback and suggestions.

Below is the summary:
*Point 1)* analyse is taking very much time for large documents:
For large JSON documents, analyze took very large time as compared to the
current head. For reference, I am attaching test file (./json-generate.py
30000 2 1000 8 6 1000)

Head: analyze test ; Time: 120.864 ms
With patch: analyze test ; Time: more than 2 hours

analyze is taking a very large time because with these patches, firstly we
iterate over all sample rows (in above case 30000), and we store all the
paths (here around 850k paths).
In another pass, we took 1 path at a time and collects stats for the
particular path by analyzing all the sample rows and we continue this
process for all 850k paths or we can say that we do 850k loops, and in each
loop we extract values for a single path.

*Point 2)* memory consummation increases rapidly for large documents:
In the above test case, there are total 851k paths and to keep stats for
one path, we allocate 1120 bytes.

Total paths : 852689 ~ 852k

Memory for 1 path to keep stats: 1120 ~ 1 KB

(sizeof(JsonValueStats) = 1120 from “Analyze Column”)

Total memory for all paths: 852689 * 1120 = 955011680 ~ 955 MB

Extra memory for each path will be more. I mean, while analyzing each path,
we allocate some more memory based on frequency and others

To keep all entries(851k paths) in the hash, we use around 1GB memory for
hash so this is also very large.

*Point 3*) Review comment noticed by Tomas Vondra:

+       oldcxt = MemoryContextSwitchTo(ctx->stats->anl_context);
+       pstats->stats = jsonAnalyzeBuildPathStats(pstats);
+       MemoryContextSwitchTo(oldcxt);

Above should be:
+       oldcxt = MemoryContextSwitchTo(ctx->mcxt);
+       pstats->stats = jsonAnalyzeBuildPathStats(pstats);
+       MemoryContextSwitchTo(oldcxt);

*Response from Tomas Vondra:*
The problem is "anl_context" is actually "Analyze", i.e. the context for
the whole ANALYZE command, for all the columns. But we only want to keep
those path stats while processing a particular column. At the end, after
processing all paths from a column, we need to "build" the final stats in
the column, and this result needs to go into "Analyze" context. But all the
partial results need to go into "Analyze Column" context.

*Point 4)*

+/*

+ * jsonAnalyzeCollectPath

+ *             Extract a single path from JSON documents and collect its
values.

+ */

+static void

+jsonAnalyzeCollectPath(JsonAnalyzeContext *ctx, Jsonb *jb, void *param)

+{

+       JsonPathAnlStats *pstats = (JsonPathAnlStats *) param;

+       JsonbValue      jbvtmp;

+       JsonbValue *jbv = JsonValueInitBinary(&jbvtmp, jb);

+       JsonPathEntry *path;

+       JsonPathEntry **entries;

+       int                     i;

+

+       entries = palloc(sizeof(*entries) * pstats->depth);

+

+       /* Build entry array in direct order */

+       for (path = &pstats->path, i = pstats->depth - 1;

+                path->parent && i >= 0;

+                path = path->parent, i--)

+               entries[i] = path;

+

+       jsonAnalyzeCollectSubpath(ctx, pstats, jbv, entries, 0);

+

+       pfree(entries);

----many times, we are trying to palloc with zero size and entries is
pointing to invalid memory (because pstats->depth=0) so I think, we should
not try to palloc with 0??

*Fix:*

+       If (pstats->depth)

  +            entries = palloc(sizeof(*entries) * pstats->depth);

>From these points, we can say that we should rethink our design to collect
stats for all paths.

We can set limits(like MCV) for paths or we can give an explicit path to
collect stats for a particular path only or we can pass a subset of the
JSON values.

In the above case, there are total 851k paths, but we can collect stats for
only 1000 paths that are most common so this way we can minimize time and
memory also and we might even keep at
least frequencies for the non-analyzed paths.

Next, I will take the latest patches from Nikita's last email and I will do
more tests.

Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

---

From: Greg Stark <[email-masked]>
Date: Fri, 1 Apr 2022 10:51:01 -0400

This patch has bitrotted, presumably after the other JSON patchset was
applied. It looks like it's failing in the json header file so it may
be as simple as additional functions added on nearby lines.

Please rebase. Reminder, it's the last week of the commitfest so time
is of the essence....

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Tue, 10 May 2022 16:49:04 +0530

> Please rebase. Reminder, it's the last week of the commitfest so time

Thanks, Greg for the report.

Here, I am attaching re-based patches of the v05 series. These patches
are re-based on the commit 7dd3ee508432730d15c5.

> I noticed some typos.

> diff --git a/src/backend/utils/adt/jsonb_selfuncs.c b/src/backend/utils/adt/jsonb_selfuncs.c
> index f5520f88a1d..d98cd7020a1 100644

Thanks, Justin for the review. We will fix these comments in the next version.

> Next, I am going to try to disable all-paths collection and implement
> collection of most common paths (and/or hashed paths maybe).

Thanks, Nikita for the v04 series of patches. I tested on the top of
your patches and verified that time taken by analyse is reduced for
large complex json docs.

In v03 patches, it was more than 2 hours, and in v04 patches, it is 39
sec only (time for Tomas's test case).

I am waiting for your patches (disable all-paths collection and
implement collection of most common paths)

Just for testing purposes, I am posting re-based patches here.

-- 
Thanks and Regards
Mahendra Singh Thalor