public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4 2/7] Row pattern recognition patch (parse/analysis).
6+ messages / 3 participants
[nested] [flat]
* [PATCH v4 2/7] Row pattern recognition patch (parse/analysis).
@ 2023-08-09 07:56 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Tatsuo Ishii @ 2023-08-09 07:56 UTC (permalink / raw)
---
src/backend/parser/parse_agg.c | 7 +
src/backend/parser/parse_clause.c | 292 +++++++++++++++++++++++++++++-
src/backend/parser/parse_expr.c | 4 +
src/backend/parser/parse_func.c | 3 +
4 files changed, 305 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 85cd47b7ae..aa7a1cee80 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -564,6 +564,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
+
/*
* There is intentionally no default: case here, so that the
* compiler will warn if we add a new ParseExprKind without
@@ -953,6 +957,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 334b9b42bd..60020a7025 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -100,7 +100,10 @@ static WindowClause *findWindowClause(List *wclist, const char *name);
static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc,
Node *clause);
-
+static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist);
+static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist);
+static void transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
+static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
/*
* transformFromClause -
@@ -2950,6 +2953,10 @@ transformWindowDefinitions(ParseState *pstate,
rangeopfamily, rangeopcintype,
&wc->endInRangeFunc,
windef->endOffset);
+
+ /* Process Row Pattern Recognition related clauses */
+ transformRPR(pstate, wc, windef, targetlist);
+
wc->runCondition = NIL;
wc->winref = winref;
@@ -3815,3 +3822,286 @@ transformFrameOffset(ParseState *pstate, int frameOptions,
return node;
}
+
+/*
+ * transformRPR
+ * Process Row Pattern Recognition related clauses
+ */
+static void
+transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist)
+{
+ /*
+ * Window definition exists?
+ */
+ if (windef == NULL)
+ return;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ /* Check Frame option. Frame must start at current row */
+ if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("FRAME must start at current row when row patttern recognition is used")));
+
+ /* Transform AFTER MACH SKIP TO clause */
+ wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo;
+
+ /* Transform SEEK or INITIAL clause */
+ wc->initial = windef->rpCommonSyntax->initial;
+
+ /* Transform DEFINE clause into list of TargetEntry's */
+ wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist);
+
+ /* Check PATTERN clause and copy to patternClause */
+ transformPatternClause(pstate, wc, windef);
+
+ /* Transform MEASURE clause */
+ transformMeasureClause(pstate, wc, windef);
+}
+
+/*
+ * transformDefineClause Process DEFINE clause and transform ResTarget into
+ * list of TargetEntry.
+ *
+ * XXX we only support column reference in row pattern definition search
+ * condition, e.g. "price". <row pattern definition variable name>.<column
+ * reference> is not supported, e.g. "A.price".
+ */
+static List *
+transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist)
+{
+ /* DEFINE variable name initials */
+ static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz";
+
+ ListCell *lc, *l;
+ ResTarget *restarget, *r;
+ List *restargets;
+ char *name;
+ int initialLen;
+ int i;
+
+ /*
+ * If Row Definition Common Syntax exists, DEFINE clause must exist.
+ * (the raw parser should have already checked it.)
+ */
+ Assert(windef->rpCommonSyntax->rpDefs != NULL);
+
+ /*
+ * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE
+ * per the SQL standard.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ bool found = false;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *)lfirst(lc);
+ name = strVal(a->lexpr);
+
+ foreach(l, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *)lfirst(l);
+
+ if (!strcmp(restarget->name, name))
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ /*
+ * "name" is missing. So create "name AS name IS TRUE" ResTarget
+ * node and add it to the temporary list.
+ */
+ A_Const *n;
+
+ restarget = makeNode(ResTarget);
+ n = makeNode(A_Const);
+ n->val.boolval.type = T_Boolean;
+ n->val.boolval.boolval = true;
+ n->location = -1;
+ restarget->name = pstrdup(name);
+ restarget->indirection = NIL;
+ restarget->val = (Node *)n;
+ restarget->location = -1;
+ restargets = lappend((List *)restargets, restarget);
+ }
+ }
+
+ if (list_length(restargets) >= 1)
+ {
+ /* add missing DEFINEs */
+ windef->rpCommonSyntax->rpDefs = list_concat(windef->rpCommonSyntax->rpDefs,
+ restargets);
+ list_free(restargets);
+ }
+
+ /*
+ * Check for duplicate row pattern definition variables. The standard
+ * requires that no two row pattern definition variable names shall be
+ * equivalent.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *)lfirst(lc);
+ name = restarget->name;
+
+ /*
+ * Add DEFINE expression (Restarget->val) to the targetlist as a
+ * TargetEntry if it does not exist yet. Planner will add the column
+ * ref var node to the outer plan's target list later on. This makes
+ * DEFINE expression could access the outer tuple while evaluating
+ * PATTERN.
+ *
+ * XXX: adding whole expressions of DEFINE to the plan.targetlist is
+ * not so good, because it's not necessary to evalute the expression
+ * in the target list while running the plan. We should extract the
+ * var nodes only then add them to the plan.targetlist.
+ */
+ findTargetlistEntrySQL99(pstate, (Node *)restarget->val, targetlist, EXPR_KIND_RPR_DEFINE);
+
+ /*
+ * Make sure that the row pattern definition search condition is a
+ * boolean expression.
+ */
+ transformWhereClause(pstate, restarget->val,
+ EXPR_KIND_RPR_DEFINE, "DEFINE");
+
+ foreach(l, restargets)
+ {
+ char *n;
+
+ r = (ResTarget *) lfirst(l);
+ n = r->name;
+
+ if (!strcmp(n, name))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause",
+ name),
+ parser_errposition(pstate, exprLocation((Node *)r))));
+ }
+ restargets = lappend(restargets, restarget);
+ }
+ list_free(restargets);
+
+ /*
+ * Create list of row pattern DEFINE variable name's initial.
+ * We assign [a-z] to them (up to 26 variable names are allowed).
+ */
+ restargets = NIL;
+ i = 0;
+ initialLen = strlen(defineVariableInitials);
+
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ char initial[2];
+
+ restarget = (ResTarget *)lfirst(lc);
+ name = restarget->name;
+
+ if (i >= initialLen)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("number of row pattern definition variable names exceeds %d", initialLen),
+ parser_errposition(pstate, exprLocation((Node *)restarget))));
+ }
+ initial[0] = defineVariableInitials[i++];
+ initial[1] = '\0';
+ wc->defineInitial = lappend(wc->defineInitial, makeString(pstrdup(initial)));
+ }
+
+ return transformTargetList(pstate, windef->rpCommonSyntax->rpDefs,
+ EXPR_KIND_RPR_DEFINE);
+}
+
+/*
+ * transformPatternClause
+ * Process PATTERN clause and return PATTERN clause in the raw parse tree
+ */
+static void
+transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+ ListCell *lc, *l;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ /*
+ * Primary row pattern variable names in PATTERN clause must appear in
+ * DEFINE clause as row pattern definition variable names.
+ */
+ wc->patternVariable = NIL;
+ wc->patternRegexp = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ char *name;
+ char *regexp;
+ bool found = false;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *)lfirst(lc);
+ name = strVal(a->lexpr);
+
+ foreach(l, windef->rpCommonSyntax->rpDefs)
+ {
+ ResTarget *restarget = (ResTarget *)lfirst(l);
+
+ if (!strcmp(restarget->name, name))
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("primary row pattern variable name \"%s\" does not appear in DEFINE clause",
+ name),
+ parser_errposition(pstate, exprLocation((Node *)a))));
+ }
+ wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name)));
+ regexp = strVal(lfirst(list_head(a->name)));
+ wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp)));
+ }
+}
+
+/*
+ * transformMeasureClause
+ * Process MEASURE clause
+ * XXX MEASURE clause is not supported yet
+ */
+static List *
+transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+ if (windef->rowPatternMeasures == NIL)
+ return NIL;
+
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s","MEASURE clause is not supported yet"),
+ parser_errposition(pstate, exprLocation((Node *)windef->rowPatternMeasures))));
+}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index fed8e4d089..8921b7ae01 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -557,6 +557,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
case EXPR_KIND_COPY_WHERE:
case EXPR_KIND_GENERATED_COLUMN:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
@@ -1770,6 +1771,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_VALUES:
case EXPR_KIND_VALUES_SINGLE:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
case EXPR_KIND_CHECK_CONSTRAINT:
@@ -3149,6 +3151,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "GENERATED AS";
case EXPR_KIND_CYCLE_MARK:
return "CYCLE";
+ case EXPR_KIND_RPR_DEFINE:
+ return "DEFINE";
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index b3f0b6a137..2ff3699538 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2656,6 +2656,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
--
2.25.1
----Next_Part(Wed_Aug__9_17_41_12_2023_134)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v4-0003-Row-pattern-recognition-patch-planner.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: WIP Incremental JSON Parser
@ 2024-01-03 11:57 Andrew Dunstan <[email protected]>
2024-01-03 13:45 ` Re: WIP Incremental JSON Parser Robert Haas <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Dunstan @ 2024-01-03 11:57 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 2024-01-02 Tu 10:14, Robert Haas wrote:
> On Tue, Dec 26, 2023 at 11:49 AM Andrew Dunstan <[email protected]> wrote:
>> Quite a long time ago Robert asked me about the possibility of an
>> incremental JSON parser. I wrote one, and I've tweaked it a bit, but the
>> performance is significantly worse that that of the current Recursive
>> Descent parser. Nevertheless, I'm attaching my current WIP state for it,
>> and I'll add it to the next CF to keep the conversation going.
> Thanks for doing this. I think it's useful even if it's slower than
> the current parser, although that probably necessitates keeping both,
> which isn't great, but I don't see a better alternative.
>
>> One possible use would be in parsing large manifest files for
>> incremental backup. However, it struck me a few days ago that this might
>> not work all that well. The current parser and the new parser both
>> palloc() space for each field name and scalar token in the JSON (unless
>> they aren't used, which is normally not the case), and they don't free
>> it, so that particularly if done in frontend code this amounts to a
>> possible memory leak, unless the semantic routines do the freeing
>> themselves. So while we can save some memory by not having to slurp in
>> the whole JSON in one hit, we aren't saving any of that other allocation
>> of memory, which amounts to almost as much space as the raw JSON.
> It seems like a pretty significant savings no matter what. Suppose the
> backup_manifest file is 2GB, and instead of creating a 2GB buffer, you
> create an 1MB buffer and feed the data to the parser in 1MB chunks.
> Well, that saves 2GB less 1MB, full stop. Now if we address the issue
> you raise here in some way, we can potentially save even more memory,
> which is great, but even if we don't, we still saved a bunch of memory
> that could not have been saved in any other way.
>
> As far as addressing that other issue, we could address the issue
> either by having the semantic routines free the memory if they don't
> need it, or alternatively by having the parser itself free the memory
> after invoking any callbacks to which it might be passed. The latter
> approach feels more conceptually pure, but the former might be the
> more practical approach. I think what really matters here is that we
> document who must or may do which things. When a callback gets passed
> a pointer, we can document either that (1) it's a palloc'd chunk that
> the calllback can free if they want or (2) that it's a palloc'd chunk
> that the caller must not free or (3) that it's not a palloc'd chunk.
> We can further document the memory context in which the chunk will be
> allocated, if applicable, and when/if the parser will free it.
Yeah. One idea I had yesterday was to stash the field names, which in
large JSON docs tent to be pretty repetitive, in a hash table instead of
pstrduping each instance. The name would be valid until the end of the
parse, and would only need to be duplicated by the callback function if
it were needed beyond that. That's not the case currently with the
parse_manifest code. I'll work on using a hash table.
The parse_manifest code does seem to pfree the scalar values it no
longer needs fairly well, so maybe we don't need to to anything there.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: WIP Incremental JSON Parser
2024-01-03 11:57 Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
@ 2024-01-03 13:45 ` Robert Haas <[email protected]>
2024-01-03 14:59 ` Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Robert Haas @ 2024-01-03 13:45 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Wed, Jan 3, 2024 at 6:57 AM Andrew Dunstan <[email protected]> wrote:
> Yeah. One idea I had yesterday was to stash the field names, which in
> large JSON docs tent to be pretty repetitive, in a hash table instead of
> pstrduping each instance. The name would be valid until the end of the
> parse, and would only need to be duplicated by the callback function if
> it were needed beyond that. That's not the case currently with the
> parse_manifest code. I'll work on using a hash table.
IMHO, this is not a good direction. Anybody who is parsing JSON
probably wants to discard the duplicated labels and convert other
heavily duplicated strings to enum values or something. (e.g. if every
record has {"color":"red"} or {"color":"green"}). So the hash table
lookups will cost but won't really save anything more than just
freeing the memory not needed, but will probably be more expensive.
> The parse_manifest code does seem to pfree the scalar values it no
> longer needs fairly well, so maybe we don't need to to anything there.
Hmm. This makes me wonder if you've measured how much actual leakage there is?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: WIP Incremental JSON Parser
2024-01-03 11:57 Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
2024-01-03 13:45 ` Re: WIP Incremental JSON Parser Robert Haas <[email protected]>
@ 2024-01-03 14:59 ` Andrew Dunstan <[email protected]>
2024-01-03 15:12 ` Re: WIP Incremental JSON Parser Robert Haas <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Dunstan @ 2024-01-03 14:59 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 2024-01-03 We 08:45, Robert Haas wrote:
> On Wed, Jan 3, 2024 at 6:57 AM Andrew Dunstan <[email protected]> wrote:
>> Yeah. One idea I had yesterday was to stash the field names, which in
>> large JSON docs tent to be pretty repetitive, in a hash table instead of
>> pstrduping each instance. The name would be valid until the end of the
>> parse, and would only need to be duplicated by the callback function if
>> it were needed beyond that. That's not the case currently with the
>> parse_manifest code. I'll work on using a hash table.
> IMHO, this is not a good direction. Anybody who is parsing JSON
> probably wants to discard the duplicated labels and convert other
> heavily duplicated strings to enum values or something. (e.g. if every
> record has {"color":"red"} or {"color":"green"}). So the hash table
> lookups will cost but won't really save anything more than just
> freeing the memory not needed, but will probably be more expensive.
I don't quite follow.
Say we have a document with an array 1m objects, each with a field
called "color". As it stands we'll allocate space for that field name 1m
times. Using a hash table we'd allocated space for it once. And
allocating the memory isn't free, although it might be cheaper than
doing hash lookups.
I guess we can benchmark it and see what the performance impact of using
a hash table might be.
Another possibility would be simply to have the callback free the field
name after use. for the parse_manifest code that could be a one-line
addition to the code at the bottom of json_object_manifest_field_start().
>> The parse_manifest code does seem to pfree the scalar values it no
>> longer needs fairly well, so maybe we don't need to to anything there.
> Hmm. This makes me wonder if you've measured how much actual leakage there is?
No I haven't. I have simply theorized about how much memory we might
consume if nothing were done by the callers to free the memory.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: WIP Incremental JSON Parser
2024-01-03 11:57 Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
2024-01-03 13:45 ` Re: WIP Incremental JSON Parser Robert Haas <[email protected]>
2024-01-03 14:59 ` Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
@ 2024-01-03 15:12 ` Robert Haas <[email protected]>
2024-01-03 16:55 ` Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Robert Haas @ 2024-01-03 15:12 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Wed, Jan 3, 2024 at 9:59 AM Andrew Dunstan <[email protected]> wrote:
> Say we have a document with an array 1m objects, each with a field
> called "color". As it stands we'll allocate space for that field name 1m
> times. Using a hash table we'd allocated space for it once. And
> allocating the memory isn't free, although it might be cheaper than
> doing hash lookups.
>
> I guess we can benchmark it and see what the performance impact of using
> a hash table might be.
>
> Another possibility would be simply to have the callback free the field
> name after use. for the parse_manifest code that could be a one-line
> addition to the code at the bottom of json_object_manifest_field_start().
Yeah. So I'm arguing that allocating the memory each time and then
freeing it sounds cheaper than looking it up in the hash table every
time, discovering it's there, and thus skipping the allocate/free.
I might be wrong about that. It's just that allocating and freeing a
small chunk of memory should boil down to popping it off of a linked
list and then pushing it back on. And that sounds cheaper than hashing
the string and looking for it in a hash bucket.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: WIP Incremental JSON Parser
2024-01-03 11:57 Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
2024-01-03 13:45 ` Re: WIP Incremental JSON Parser Robert Haas <[email protected]>
2024-01-03 14:59 ` Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
2024-01-03 15:12 ` Re: WIP Incremental JSON Parser Robert Haas <[email protected]>
@ 2024-01-03 16:55 ` Andrew Dunstan <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Dunstan @ 2024-01-03 16:55 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 2024-01-03 We 10:12, Robert Haas wrote:
> On Wed, Jan 3, 2024 at 9:59 AM Andrew Dunstan <[email protected]> wrote:
>> Say we have a document with an array 1m objects, each with a field
>> called "color". As it stands we'll allocate space for that field name 1m
>> times. Using a hash table we'd allocated space for it once. And
>> allocating the memory isn't free, although it might be cheaper than
>> doing hash lookups.
>>
>> I guess we can benchmark it and see what the performance impact of using
>> a hash table might be.
>>
>> Another possibility would be simply to have the callback free the field
>> name after use. for the parse_manifest code that could be a one-line
>> addition to the code at the bottom of json_object_manifest_field_start().
> Yeah. So I'm arguing that allocating the memory each time and then
> freeing it sounds cheaper than looking it up in the hash table every
> time, discovering it's there, and thus skipping the allocate/free.
>
> I might be wrong about that. It's just that allocating and freeing a
> small chunk of memory should boil down to popping it off of a linked
> list and then pushing it back on. And that sounds cheaper than hashing
> the string and looking for it in a hash bucket.
OK, cleaning up in the client code will be much simpler, so let's go
with that for now and revisit it later if necessary.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-01-03 16:55 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-08-09 07:56 [PATCH v4 2/7] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[email protected]>
2024-01-03 11:57 Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
2024-01-03 13:45 ` Re: WIP Incremental JSON Parser Robert Haas <[email protected]>
2024-01-03 14:59 ` Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
2024-01-03 15:12 ` Re: WIP Incremental JSON Parser Robert Haas <[email protected]>
2024-01-03 16:55 ` Re: WIP Incremental JSON Parser Andrew Dunstan <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox