Hello
2011/1/15 Noah Misch <n...@leadboat.com>:
> Hello Pavel,
>
> I'm reviewing this patch for CommitFest 2011-01.
>
Thank you very much,
I am sending a updated version with little bit more comments. But I am
sure, so somebody with good English have to edit my comments.
Minimally I hope, so your questions will be answered.
> The patch seems fully desirable. It appropriately contains no documentation
> updates. It contains no new tests, and that's probably fine, too; I can't
> think
> of any corner cases where this would do something other than work correctly or
> break things comprehensively.
>
> Using your test case from here:
> http://archives.postgresql.org/message-id/aanlktikdcw+c-c4u4ngaobhpfszkb5uy_zuatdzfp...@mail.gmail.com
> I observed a 28x speedup, similar to Álvaro's report. I also made my own test
> case, attached, to evaluate this from a somewhat different angle and also to
> consider the worst case. With a 100 KiB string (good case), I see a 4.8x
> speedup. With a 1 KiB string (worst case - never toasted), I see no
> statistically significant change. This is to be expected.
>
> A few specific questions and comments follow, all minor. Go ahead and mark
> the
> patch ready for committer when you've acted on them, or declined to do so, to
> your satisfaction. I don't think a re-review will be needed.
>
> Thanks,
> nm
>
> On Mon, Nov 22, 2010 at 01:46:51PM +0100, Pavel Stehule wrote:
>> *** ./pl_exec.c.orig 2010-11-16 10:28:42.000000000 +0100
>> --- ./pl_exec.c 2010-11-22 13:33:01.597726809 +0100
>
> The patch applies cleanly, but the format is slightly nonstandard (-p0 when
> applied from src/pl/plpgsql/src, rather than -p1 from the root).
>
>> ***************
>> *** 3944,3949 ****
>> --- 3965,3993 ----
>>
>> *typeid = var->datatype->typoid;
>> *typetypmod = var->datatype->atttypmod;
>> +
>> + /*
>> + * explicit deTOAST and decomprim for varlena
>> types
>
> "decompress", perhaps?
>
fixed
>> + */
>> + if (var->should_be_detoasted)
>> + {
>> + Datum dvalue;
>> +
>> + Assert(!var->isnull);
>> +
>> + oldcontext =
>> MemoryContextSwitchTo(estate->fn_mcxt);
>> + dvalue =
>> PointerGetDatum(PG_DETOAST_DATUM(var->value));
>> + if (dvalue != var->value)
>> + {
>> + if (var->freeval)
>> + free_var(var);
>> + var->value = dvalue;
>> + var->freeval = true;
>> + }
>> + MemoryContextSwitchTo(oldcontext);
>
> This line adds trailing white space.
>
>> + var->should_be_detoasted = false;
>> + }
>> +
>> *value = var->value;
>> *isnull = var->isnull;
>> break;
>
>> *** ./plpgsql.h.orig 2010-11-16 10:28:42.000000000 +0100
>> --- ./plpgsql.h 2010-11-22 13:12:38.897851879 +0100
>
>> ***************
>> *** 644,649 ****
>> --- 645,651 ----
>> bool fn_is_trigger;
>> PLpgSQL_func_hashkey *fn_hashkey; /* back-link to hashtable key
>> */
>> MemoryContext fn_cxt;
>> + MemoryContext fn_mcxt; /* link to function's memory
>> context */
>>
>> Oid fn_rettype;
>> int fn_rettyplen;
>> ***************
>> *** 692,697 ****
>> --- 694,701 ----
>> Oid rettype; /* type of current
>> retval */
>>
>> Oid fn_rettype; /* info about declared
>> function rettype */
>> + MemoryContext fn_mcxt; /* link to function's memory
>> context */
>> +
>> bool retistuple;
>> bool retisset;
>>
>
> I only see PLpgSQL_execstate.fn_mcxt getting populated in this patch. Is the
> PLpgSQL_function.fn_mcxt leftover from an earlier design?
I have to access to top execution context from exec_eval_datum
function. This function can be called from parser's context, and
without explicit switch to top execution context a variables are
detoasted in wrong context.
>
> I could not readily tell the difference between fn_cxt and fn_mcxt. As I
> understand it, fn_mcxt is the SPI procCxt, and fn_cxt is the long-lived
> context
> used to cache facts across many transactions. Perhaps name the member
> something
> like "top_cxt", "exec_cxt" or "proc_cxt" and comment it like "/* SPI Proc
> memory
> context */" to make this clearer.
I used a top_exec_cxt name
Pavel Stehule
Regards
>
*** ./src/pl/plpgsql/src/pl_exec.c.orig 2011-01-16 14:18:59.000000000 +0100
--- ./src/pl/plpgsql/src/pl_exec.c 2011-01-16 18:45:59.659254108 +0100
***************
*** 255,260 ****
--- 255,264 ----
var->value = fcinfo->arg[i];
var->isnull = fcinfo->argnull[i];
var->freeval = false;
+
+ /* only varlena types should be detoasted */
+ var->should_be_detoasted = !var->isnull && !var->datatype->typbyval
+ && var->datatype->typlen == -1;
}
break;
***************
*** 570,581 ****
--- 574,587 ----
elog(ERROR, "unrecognized trigger action: not INSERT, DELETE, UPDATE, or TRUNCATE");
var->isnull = false;
var->freeval = true;
+ var->should_be_detoasted = false;
var = (PLpgSQL_var *) (estate.datums[func->tg_name_varno]);
var->value = DirectFunctionCall1(namein,
CStringGetDatum(trigdata->tg_trigger->tgname));
var->isnull = false;
var->freeval = true;
+ var->should_be_detoasted = false;
var = (PLpgSQL_var *) (estate.datums[func->tg_when_varno]);
if (TRIGGER_FIRED_BEFORE(trigdata->tg_event))
***************
*** 588,593 ****
--- 594,600 ----
elog(ERROR, "unrecognized trigger execution time: not BEFORE, AFTER, or INSTEAD OF");
var->isnull = false;
var->freeval = true;
+ var->should_be_detoasted = false;
var = (PLpgSQL_var *) (estate.datums[func->tg_level_varno]);
if (TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))
***************
*** 598,620 ****
--- 605,631 ----
elog(ERROR, "unrecognized trigger event type: not ROW or STATEMENT");
var->isnull = false;
var->freeval = true;
+ var->should_be_detoasted = false;
var = (PLpgSQL_var *) (estate.datums[func->tg_relid_varno]);
var->value = ObjectIdGetDatum(trigdata->tg_relation->rd_id);
var->isnull = false;
var->freeval = false;
+ var->should_be_detoasted = false;
var = (PLpgSQL_var *) (estate.datums[func->tg_relname_varno]);
var->value = DirectFunctionCall1(namein,
CStringGetDatum(RelationGetRelationName(trigdata->tg_relation)));
var->isnull = false;
var->freeval = true;
+ var->should_be_detoasted = false;
var = (PLpgSQL_var *) (estate.datums[func->tg_table_name_varno]);
var->value = DirectFunctionCall1(namein,
CStringGetDatum(RelationGetRelationName(trigdata->tg_relation)));
var->isnull = false;
var->freeval = true;
+ var->should_be_detoasted = false;
var = (PLpgSQL_var *) (estate.datums[func->tg_table_schema_varno]);
var->value = DirectFunctionCall1(namein,
***************
*** 624,634 ****
--- 635,647 ----
trigdata->tg_relation))));
var->isnull = false;
var->freeval = true;
+ var->should_be_detoasted = false;
var = (PLpgSQL_var *) (estate.datums[func->tg_nargs_varno]);
var->value = Int16GetDatum(trigdata->tg_trigger->tgnargs);
var->isnull = false;
var->freeval = false;
+ var->should_be_detoasted = false;
var = (PLpgSQL_var *) (estate.datums[func->tg_argv_varno]);
if (trigdata->tg_trigger->tgnargs > 0)
***************
*** 654,665 ****
--- 667,680 ----
-1, false, 'i'));
var->isnull = false;
var->freeval = true;
+ var->should_be_detoasted = false;
}
else
{
var->value = (Datum) 0;
var->isnull = true;
var->freeval = false;
+ var->should_be_detoasted = false;
}
estate.err_text = gettext_noop("during function entry");
***************
*** 841,846 ****
--- 856,862 ----
new->value = 0;
new->isnull = true;
new->freeval = false;
+ new->should_be_detoasted = false;
result = (PLpgSQL_datum *) new;
}
***************
*** 2652,2657 ****
--- 2668,2683 ----
estate->exitlabel = NULL;
estate->cur_error = NULL;
+ /*
+ * Store a execution top memory context. The top context isn't usually
+ * explicitly selected, but there is one exception. We would to detoast
+ * datuns in this context. Varlena datums are explicitly detoasted in
+ * exec_eval_datum routine. This routine can be called from plpgsql_param_fetch,
+ * what is callback for parser - etc different memory context, and then
+ * we have to access top execution memory context.
+ */
+ estate->top_exec_cxt = CurrentMemoryContext;
+
estate->tuple_store = NULL;
if (rsi)
{
***************
*** 3544,3550 ****
--- 3570,3579 ----
var->value = newvalue;
var->isnull = *isNull;
if (!var->datatype->typbyval && !*isNull)
+ {
var->freeval = true;
+ var->should_be_detoasted = var->datatype->typlen == -1;
+ }
break;
}
***************
*** 3944,3949 ****
--- 3973,4007 ----
*typeid = var->datatype->typoid;
*typetypmod = var->datatype->atttypmod;
+
+ /*
+ * explicit deTOAST and decompress for varlena types
+ */
+ if (var->should_be_detoasted)
+ {
+ Datum dvalue;
+
+ Assert(!var->isnull);
+
+ /*
+ * We should to detoast variables in top execution context,
+ * and then is necessary to switch to it (this routine
+ * can be called from parser with different current
+ * context.
+ */
+ oldcontext = MemoryContextSwitchTo(estate->top_exec_cxt);
+ dvalue = PointerGetDatum(PG_DETOAST_DATUM(var->value));
+ if (dvalue != var->value)
+ {
+ if (var->freeval)
+ free_var(var);
+ var->value = dvalue;
+ var->freeval = true;
+ }
+ MemoryContextSwitchTo(oldcontext);
+ var->should_be_detoasted = false;
+ }
+
*value = var->value;
*isnull = var->isnull;
break;
***************
*** 5552,5557 ****
--- 5610,5616 ----
var->value = CStringGetTextDatum(str);
var->isnull = false;
var->freeval = true;
+ var->should_be_detoasted = false;
}
/*
*** ./src/pl/plpgsql/src/plpgsql.h.orig 2011-01-16 14:18:59.000000000 +0100
--- ./src/pl/plpgsql/src/plpgsql.h 2011-01-16 18:41:54.104294898 +0100
***************
*** 242,247 ****
--- 242,248 ----
Datum value;
bool isnull;
bool freeval;
+ bool should_be_detoasted;
} PLpgSQL_var;
***************
*** 643,649 ****
ItemPointerData fn_tid;
bool fn_is_trigger;
PLpgSQL_func_hashkey *fn_hashkey; /* back-link to hashtable key */
! MemoryContext fn_cxt;
Oid fn_rettype;
int fn_rettyplen;
--- 644,650 ----
ItemPointerData fn_tid;
bool fn_is_trigger;
PLpgSQL_func_hashkey *fn_hashkey; /* back-link to hashtable key */
! MemoryContext fn_cxt; /* function's persistent context */
Oid fn_rettype;
int fn_rettyplen;
***************
*** 692,697 ****
--- 693,700 ----
Oid rettype; /* type of current retval */
Oid fn_rettype; /* info about declared function rettype */
+ MemoryContext top_exec_cxt; /* function's top execution memory context */
+
bool retistuple;
bool retisset;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
