pg.ddx.io pgsql-docs@postgresql.org mailing list archive
help / color / mirror / Atom feedRe: VALUES clause memory optimization
4+ messages / 3 participants
[nested] [flat]
* Re: VALUES clause memory optimization
@ 2006-08-04 04:27 Joe Conway <mail@joeconway.com>
2006-08-04 04:56 ` Re: VALUES clause memory optimization Tom Lane <tgl@sss.pgh.pa.us>
2006-08-26 03:35 ` Re: VALUES clause memory optimization Bruce Momjian <bruce@momjian.us>
0 siblings, 2 replies; 4+ messages in thread
From: Joe Conway @ 2006-08-04 04:27 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Hackers (PostgreSQL) <pgsql-hackers@postgresql.org>
Tom Lane wrote:
> The reason we could safely list_free inside transformInsertRow is that
> we know all its callers have just built the passed-in list and so there
> are no other pointers to it. That doesn't apply in the general case of
> grammar output.
What about for the specific case of an InsertStmt? It seems that we
could at least get away with freeing the raw-expression list in that case.
In terms of freeing an entire arbitrary node, could we create a
backend/nodes/freefuncs.c file that does a recursive freeObject()
similar to the way copyObject() does in backend/nodes/copyfuncs.c?
> My advice is to get that low-hanging fruit
> in transformInsertRow and leave the other ideas for 8.3.
OK. This should be safe also, correct?
Thanks,
Joe
8<----------------------------------------
inline patch 1: src/backend/parser/analyze.c (+1/-0 lines) download inline-1.patch
diff -c -r1.341 analyze.c
*** src/backend/parser/analyze.c 2 Aug 2006 01:59:46 -0000 1.341
--- src/backend/parser/analyze.c 2 Aug 2006 05:13:20 -0000
***************
*** 2191,2196 ****
--- 2196,2202 ----
for (i = 0; i < sublist_length; i++)
{
coltypes[i] = select_common_type(coltype_lists[i], "VALUES");
+ list_free(coltype_lists[i]);
}
newExprsLists = NIL;
* Re: VALUES clause memory optimization
2006-08-04 04:27 Re: VALUES clause memory optimization Joe Conway <mail@joeconway.com>
@ 2006-08-04 04:56 ` Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 4+ messages in thread
From: Tom Lane @ 2006-08-04 04:56 UTC (permalink / raw)
To: Joe Conway <mail@joeconway.com>; +Cc: Hackers (PostgreSQL) <pgsql-hackers@postgresql.org>
Joe Conway <mail@joeconway.com> writes:
> What about for the specific case of an InsertStmt? It seems that we
> could at least get away with freeing the raw-expression list in that case.
Not sure ... what about rules, BETWEEN, yadda yadda?
> In terms of freeing an entire arbitrary node, could we create a
> backend/nodes/freefuncs.c file that does a recursive freeObject()
> similar to the way copyObject() does in backend/nodes/copyfuncs.c?
We got rid of freefuncs.c years ago, for good and sufficient reasons
that have not gone away. The problem is exactly that you don't know
whether any shortcuts were taken in constructing the node tree:
multiple links, pointers to constants, pointers to stuff that wasn't
supposed to be freed are all severe hazards.
>> My advice is to get that low-hanging fruit
>> in transformInsertRow and leave the other ideas for 8.3.
> OK. This should be safe also, correct?
Yes, but what's your point? The case that seems worth trying to
optimize is "INSERT INTO foo VALUES ... real long list ...". Certainly
the MySQL crowd is not going to be stressing transformValuesClause,
because they don't know it exists.
$ mysql test
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A
Welcome to the MySQL monitor. Commands end with ; or \g.
Your MySQL connection id is 12 to server version: 5.0.22
Type 'help;' or '\h' for help. Type '\c' to clear the buffer.
mysql> values (1),(2);
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'values (1),(2)' at line 1
mysql> select * from (values (1),(2)) as x(y);
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'values (1),(2)) as x(y)' at line 1
mysql> select * from foo where x in (values (1),(2));
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '1),(2))' at line 1
mysql>
mysql shortcomings aside, I don't really see the use-case for enormously
long VALUES lists anywhere except the bulk-data-load scenario, ie,
exactly INSERT ... VALUES. So I don't feel a need to be real tense
in transformValuesClause.
regards, tom lane
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: VALUES clause memory optimization
2006-08-04 04:27 Re: VALUES clause memory optimization Joe Conway <mail@joeconway.com>
@ 2006-08-26 03:35 ` Bruce Momjian <bruce@momjian.us>
2006-08-26 20:08 ` Re: VALUES clause memory optimization Joe Conway <mail@joeconway.com>
1 sibling, 1 reply; 4+ messages in thread
From: Bruce Momjian @ 2006-08-26 03:35 UTC (permalink / raw)
To: Joe Conway <mail@joeconway.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Hackers (PostgreSQL) <pgsql-hackers@postgresql.org>
Has this been addressed?
---------------------------------------------------------------------------
Joe Conway wrote:
> Tom Lane wrote:
> > The reason we could safely list_free inside transformInsertRow is that
> > we know all its callers have just built the passed-in list and so there
> > are no other pointers to it. That doesn't apply in the general case of
> > grammar output.
>
> What about for the specific case of an InsertStmt? It seems that we
> could at least get away with freeing the raw-expression list in that case.
>
> In terms of freeing an entire arbitrary node, could we create a
> backend/nodes/freefuncs.c file that does a recursive freeObject()
> similar to the way copyObject() does in backend/nodes/copyfuncs.c?
>
> > My advice is to get that low-hanging fruit
> > in transformInsertRow and leave the other ideas for 8.3.
>
> OK. This should be safe also, correct?
>
> Thanks,
>
> Joe
>
> 8<----------------------------------------
> diff -c -r1.341 analyze.c
> *** src/backend/parser/analyze.c 2 Aug 2006 01:59:46 -0000 1.341
> --- src/backend/parser/analyze.c 2 Aug 2006 05:13:20 -0000
> ***************
> *** 2191,2196 ****
> --- 2196,2202 ----
> for (i = 0; i < sublist_length; i++)
> {
> coltypes[i] = select_common_type(coltype_lists[i], "VALUES");
> + list_free(coltype_lists[i]);
> }
>
> newExprsLists = NIL;
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: VALUES clause memory optimization
2006-08-04 04:27 Re: VALUES clause memory optimization Joe Conway <mail@joeconway.com>
2006-08-26 03:35 ` Re: VALUES clause memory optimization Bruce Momjian <bruce@momjian.us>
@ 2006-08-26 20:08 ` Joe Conway <mail@joeconway.com>
0 siblings, 0 replies; 4+ messages in thread
From: Joe Conway @ 2006-08-26 20:08 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Hackers (PostgreSQL) <pgsql-hackers@postgresql.org>
Bruce Momjian wrote:
> Has this been addressed?
>
>>Tom Lane wrote:
>>>The reason we could safely list_free inside transformInsertRow is that
>>>we know all its callers have just built the passed-in list and so there
>>>are no other pointers to it. That doesn't apply in the general case of
>>>grammar output.
After another look, even in this case there could be other pointers.
Starting around line 667 of analyze.c:
8<------------------
foreach(lc, selectQuery->targetList)
{
TargetEntry *tle = (TargetEntry *) lfirst(lc);
Expr *expr;
if (tle->resjunk)
continue;
if (tle->expr &&
(IsA(tle->expr, Const) ||IsA(tle->expr, Param)) &&
exprType((Node *) tle->expr) == UNKNOWNOID)
expr = tle->expr;
else
expr = (Expr *) makeVar(rtr->rtindex,
tle->resno,
exprType((Node *) tle->expr),
exprTypmod((Node *) tle->expr),
0);
exprList = lappend(exprList, expr);
}
/* Prepare row for assignment to target table */
exprList = transformInsertRow(pstate, exprList,
stmt->cols,
icolumns, attrnos);
8<------------------
So in the UNKNOWNOID case, it wouldn't be safe to free the node after
transformation because it originates in the grammar output. The *only*
safe thing to free up would be a the input exprList itself. Not much to
be saved there.
>>>My advice is to get that low-hanging fruit
>>>in transformInsertRow and leave the other ideas for 8.3.
This one case doesn't provide that much memory savings by itself anyway.
I guess we'll just live with it until we can come up with a safe way to
free grammar output.
Joe
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2006-08-26 20:08 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2006-08-04 04:27 Re: VALUES clause memory optimization Joe Conway <mail@joeconway.com>
2006-08-04 04:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2006-08-26 03:35 ` Bruce Momjian <bruce@momjian.us>
2006-08-26 20:08 ` Joe Conway <mail@joeconway.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox