Skip to content

Commit 6563e9e

Browse files
tglsfdctglsfdc
committed
Add a "provariadic" column to pg_proc to eliminate the remarkably expensive
need to deconstruct proargmodes for each pg_proc entry inspected by FuncnameGetCandidates(). Fixes function lookup performance regression caused by yesterday's variadic-functions patch. In passing, make pg_proc.probin be NULL, rather than a dummy value '-', in cases where it is not actually used for the particular type of function. This should buy back some of the space cost of the extra column.
1 parent 895a4bc commit 6563e9e

File tree

14 files changed

+2312
-2239
lines changed

14 files changed

+2312
-2239
lines changed

‎doc/src/sgml/catalogs.sgml

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<!-- $PostgreSQL: pgsql/doc/src/sgml/catalogs.sgml,v 2.169 2008/07/16 01:30:21 tgl Exp $ -->
1+
<!-- $PostgreSQL: pgsql/doc/src/sgml/catalogs.sgml,v 2.170 2008/07/16 16:55:23 tgl Exp $ -->
22
<!--
33
Documentation of the system catalogs, directed toward PostgreSQL developers
44
-->
@@ -3569,6 +3569,14 @@
35693569
<entry>Estimated number of result rows (zero if not <structfield>proretset</>)</entry>
35703570
</row>
35713571

3572+
<row>
3573+
<entry><structfield>provariadic</structfield></entry>
3574+
<entry><type>oid</type></entry>
3575+
<entry><literal><link linkend="catalog-pg-type"><structname>pg_type</structname></link>.oid</literal></entry>
3576+
<entry>Data type of the variadic array parameter's elements,
3577+
or zero if the function does not have a variadic parameter</entry>
3578+
</row>
3579+
35723580
<row>
35733581
<entry><structfield>proisagg</structfield></entry>
35743582
<entry><type>bool</type></entry>

‎src/backend/catalog/namespace.c

Lines changed: 11 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
* Portions Copyright (c) 1994, Regents of the University of California
1414
*
1515
* IDENTIFICATION
16-
* $PostgreSQL: pgsql/src/backend/catalog/namespace.c,v 1.108 2008/07/16 01:30:21 tgl Exp $
16+
* $PostgreSQL: pgsql/src/backend/catalog/namespace.c,v 1.109 2008/07/16 16:55:23 tgl Exp $
1717
*
1818
*-------------------------------------------------------------------------
1919
*/
@@ -619,50 +619,23 @@ FuncnameGetCandidates(List *names, int nargs, bool expand_variadic)
619619
int pronargs = procform->pronargs;
620620
int effective_nargs;
621621
int pathpos = 0;
622-
bool variadic = false;
623-
Oid va_elem_type = InvalidOid;
622+
bool variadic;
623+
Oid va_elem_type;
624624
FuncCandidateList newResult;
625625

626626
/*
627627
* Check if function is variadic, and get variadic element type if so.
628-
* If expand_variadic is false, we can just ignore variadic-ness.
629-
*
630-
* XXX it's annoying to inject something as expensive as this even
631-
* when there are no variadic functions involved. Find a better way.
628+
* If expand_variadic is false, we should just ignore variadic-ness.
632629
*/
633630
if (expand_variadic)
634631
{
635-
Datum proargmodes;
636-
bool isnull;
637-
638-
proargmodes = SysCacheGetAttr(PROCOID, proctup,
639-
Anum_pg_proc_proargmodes, &isnull);
640-
if (!isnull)
641-
{
642-
ArrayType *ar = DatumGetArrayTypeP(proargmodes);
643-
char *argmodes;
644-
int j;
645-
646-
argmodes = ARR_DATA_PTR(ar);
647-
j = ARR_DIMS(ar)[0] - 1;
648-
if (j >= 0 && argmodes[j] == PROARGMODE_VARIADIC)
649-
{
650-
variadic = any_variadic = true;
651-
switch (procform->proargtypes.values[j])
652-
{
653-
case ANYOID:
654-
va_elem_type = ANYOID;
655-
break;
656-
case ANYARRAYOID:
657-
va_elem_type = ANYELEMENTOID;
658-
break;
659-
default:
660-
va_elem_type = get_element_type(procform->proargtypes.values[j]);
661-
Assert(OidIsValid(va_elem_type));
662-
break;
663-
}
664-
}
665-
}
632+
va_elem_type = procform->provariadic;
633+
variadic = OidIsValid(va_elem_type);
634+
}
635+
else
636+
{
637+
va_elem_type = InvalidOid;
638+
variadic = false;
666639
}
667640

668641
/* Ignore if it doesn't match requested argument count */

‎src/backend/catalog/pg_aggregate.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/catalog/pg_aggregate.c,v 1.94 2008/07/16 01:30:22 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/catalog/pg_aggregate.c,v 1.95 2008/07/16 16:55:23 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -202,7 +202,7 @@ AggregateCreate(const char *aggName,
202202
INTERNALlanguageId, /* languageObjectId */
203203
InvalidOid, /* no validator */
204204
"aggregate_dummy", /* placeholder proc */
205-
"-", /* probin */
205+
NULL, /* probin */
206206
true, /* isAgg */
207207
false, /* security invoker (currently not
208208
* definable for agg) */

‎src/backend/catalog/pg_proc.c

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/catalog/pg_proc.c,v 1.151 2008/03/27 03:57:33 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/catalog/pg_proc.c,v 1.152 2008/07/16 16:55:23 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -85,6 +85,7 @@ ProcedureCreate(const char *procedureName,
8585
bool genericOutParam = false;
8686
bool internalInParam = false;
8787
bool internalOutParam = false;
88+
Oid variadicType = InvalidOid;
8889
Relation rel;
8990
HeapTuple tup;
9091
HeapTuple oldtup;
@@ -103,7 +104,6 @@ ProcedureCreate(const char *procedureName,
103104
* sanity checks
104105
*/
105106
Assert(PointerIsValid(prosrc));
106-
Assert(PointerIsValid(probin));
107107

108108
parameterCount = parameterTypes->dim1;
109109
if (parameterCount < 0 || parameterCount > FUNC_MAX_ARGS)
@@ -211,6 +211,64 @@ ProcedureCreate(const char *procedureName,
211211
procedureName,
212212
format_type_be(parameterTypes->values[0]))));
213213

214+
if (parameterModes != PointerGetDatum(NULL))
215+
{
216+
/*
217+
* We expect the array to be a 1-D CHAR array; verify that. We don't
218+
* need to use deconstruct_array() since the array data is just going
219+
* to look like a C array of char values.
220+
*/
221+
ArrayType *modesArray = (ArrayType *) DatumGetPointer(parameterModes);
222+
char *modes;
223+
224+
if (ARR_NDIM(modesArray) != 1 ||
225+
ARR_DIMS(modesArray)[0] != allParamCount ||
226+
ARR_HASNULL(modesArray) ||
227+
ARR_ELEMTYPE(modesArray) != CHAROID)
228+
elog(ERROR, "parameterModes is not a 1-D char array");
229+
modes = (char *) ARR_DATA_PTR(modesArray);
230+
/*
231+
* Only the last input parameter can be variadic; if it is, save
232+
* its element type. Errors here are just elog since caller should
233+
* have checked this already.
234+
*/
235+
for (i = 0; i < allParamCount; i++)
236+
{
237+
switch (modes[i])
238+
{
239+
case PROARGMODE_IN:
240+
case PROARGMODE_INOUT:
241+
if (OidIsValid(variadicType))
242+
elog(ERROR, "variadic parameter must be last");
243+
break;
244+
case PROARGMODE_OUT:
245+
/* okay */
246+
break;
247+
case PROARGMODE_VARIADIC:
248+
if (OidIsValid(variadicType))
249+
elog(ERROR, "variadic parameter must be last");
250+
switch (allParams[i])
251+
{
252+
case ANYOID:
253+
variadicType = ANYOID;
254+
break;
255+
case ANYARRAYOID:
256+
variadicType = ANYELEMENTOID;
257+
break;
258+
default:
259+
variadicType = get_element_type(allParams[i]);
260+
if (!OidIsValid(variadicType))
261+
elog(ERROR, "variadic parameter is not an array");
262+
break;
263+
}
264+
break;
265+
default:
266+
elog(ERROR, "invalid parameter mode '%c'", modes[i]);
267+
break;
268+
}
269+
}
270+
}
271+
214272
/*
215273
* All seems OK; prepare the data to be inserted into pg_proc.
216274
*/
@@ -229,6 +287,7 @@ ProcedureCreate(const char *procedureName,
229287
values[Anum_pg_proc_prolang - 1] = ObjectIdGetDatum(languageObjectId);
230288
values[Anum_pg_proc_procost - 1] = Float4GetDatum(procost);
231289
values[Anum_pg_proc_prorows - 1] = Float4GetDatum(prorows);
290+
values[Anum_pg_proc_provariadic - 1] = ObjectIdGetDatum(variadicType);
232291
values[Anum_pg_proc_proisagg - 1] = BoolGetDatum(isAgg);
233292
values[Anum_pg_proc_prosecdef - 1] = BoolGetDatum(security_definer);
234293
values[Anum_pg_proc_proisstrict - 1] = BoolGetDatum(isStrict);
@@ -250,7 +309,10 @@ ProcedureCreate(const char *procedureName,
250309
else
251310
nulls[Anum_pg_proc_proargnames - 1] = 'n';
252311
values[Anum_pg_proc_prosrc - 1] = CStringGetTextDatum(prosrc);
253-
values[Anum_pg_proc_probin - 1] = CStringGetTextDatum(probin);
312+
if (probin)
313+
values[Anum_pg_proc_probin - 1] = CStringGetTextDatum(probin);
314+
else
315+
nulls[Anum_pg_proc_probin - 1] = 'n';
254316
if (proconfig != PointerGetDatum(NULL))
255317
values[Anum_pg_proc_proconfig - 1] = proconfig;
256318
else
@@ -497,12 +559,12 @@ fmgr_c_validator(PG_FUNCTION_ARGS)
497559

498560
tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_prosrc, &isnull);
499561
if (isnull)
500-
elog(ERROR, "null prosrc");
562+
elog(ERROR, "null prosrc for C function %u", funcoid);
501563
prosrc = TextDatumGetCString(tmp);
502564

503565
tmp = SysCacheGetAttr(PROCOID, tuple, Anum_pg_proc_probin, &isnull);
504566
if (isnull)
505-
elog(ERROR, "null probin");
567+
elog(ERROR, "null probin for C function %u", funcoid);
506568
probin = TextDatumGetCString(tmp);
507569

508570
(void) load_external_function(probin, prosrc, true, &libraryhandle);

‎src/backend/commands/functioncmds.c

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
*
1111
*
1212
* IDENTIFICATION
13-
* $PostgreSQL: pgsql/src/backend/commands/functioncmds.c,v 1.96 2008/07/16 01:30:22 tgl Exp $
13+
* $PostgreSQL: pgsql/src/backend/commands/functioncmds.c,v 1.97 2008/07/16 16:55:23 tgl Exp $
1414
*
1515
* DESCRIPTION
1616
* These routines take the parse tree and pick out the
@@ -590,7 +590,8 @@ compute_attributes_with_style(List *parameters, bool *isStrict_p, char *volatili
590590
* AS <object reference, or sql code>
591591
*/
592592
static void
593-
interpret_AS_clause(Oid languageOid, const char *languageName, List *as,
593+
interpret_AS_clause(Oid languageOid, const char *languageName,
594+
char *funcname, List *as,
594595
char **prosrc_str_p, char **probin_str_p)
595596
{
596597
Assert(as != NIL);
@@ -599,25 +600,47 @@ interpret_AS_clause(Oid languageOid, const char *languageName, List *as,
599600
{
600601
/*
601602
* For "C" language, store the file name in probin and, when given,
602-
* the link symbol name in prosrc.
603+
* the link symbol name in prosrc. If link symbol is omitted,
604+
* substitute procedure name. We also allow link symbol to be
605+
* specified as "-", since that was the habit in PG versions before
606+
* 8.4, and there might be dump files out there that don't translate
607+
* that back to "omitted".
603608
*/
604609
*probin_str_p = strVal(linitial(as));
605610
if (list_length(as) == 1)
606-
*prosrc_str_p = "-";
611+
*prosrc_str_p = funcname;
607612
else
613+
{
608614
*prosrc_str_p = strVal(lsecond(as));
615+
if (strcmp(*prosrc_str_p, "-") == 0)
616+
*prosrc_str_p = funcname;
617+
}
609618
}
610619
else
611620
{
612621
/* Everything else wants the given string in prosrc. */
613622
*prosrc_str_p = strVal(linitial(as));
614-
*probin_str_p = "-";
623+
*probin_str_p = NULL;
615624

616625
if (list_length(as) != 1)
617626
ereport(ERROR,
618627
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
619628
errmsg("only one AS item needed for language \"%s\"",
620629
languageName)));
630+
631+
if (languageOid == INTERNALlanguageId)
632+
{
633+
/*
634+
* In PostgreSQL versions before 6.5, the SQL name of the created
635+
* function could not be different from the internal name, and
636+
* "prosrc" wasn't used. So there is code out there that does
637+
* CREATE FUNCTION xyz AS '' LANGUAGE internal. To preserve some
638+
* modicum of backwards compatibility, accept an empty "prosrc"
639+
* value as meaning the supplied SQL function name.
640+
*/
641+
if (strlen(*prosrc_str_p) == 0)
642+
*prosrc_str_p = funcname;
643+
}
621644
}
622645
}
623646

@@ -759,30 +782,9 @@ CreateFunction(CreateFunctionStmt *stmt)
759782

760783
compute_attributes_with_style(stmt->withClause, &isStrict, &volatility);
761784

762-
interpret_AS_clause(languageOid, languageName, as_clause,
785+
interpret_AS_clause(languageOid, languageName, funcname, as_clause,
763786
&prosrc_str, &probin_str);
764787

765-
if (languageOid == INTERNALlanguageId)
766-
{
767-
/*
768-
* In PostgreSQL versions before 6.5, the SQL name of the created
769-
* function could not be different from the internal name, and
770-
* "prosrc" wasn't used. So there is code out there that does CREATE
771-
* FUNCTION xyz AS '' LANGUAGE internal. To preserve some modicum of
772-
* backwards compatibility, accept an empty "prosrc" value as meaning
773-
* the supplied SQL function name.
774-
*/
775-
if (strlen(prosrc_str) == 0)
776-
prosrc_str = funcname;
777-
}
778-
779-
if (languageOid == ClanguageId)
780-
{
781-
/* If link symbol is specified as "-", substitute procedure name */
782-
if (strcmp(prosrc_str, "-") == 0)
783-
prosrc_str = funcname;
784-
}
785-
786788
/*
787789
* Set default values for COST and ROWS depending on other parameters;
788790
* reject ROWS if it's not returnsSet. NB: pg_dump knows these default

‎src/backend/utils/adt/ruleutils.c

Lines changed: 7 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
*
1010
*
1111
* IDENTIFICATION
12-
* $PostgreSQL: pgsql/src/backend/utils/adt/ruleutils.c,v 1.276 2008/07/16 01:30:22 tgl Exp $
12+
* $PostgreSQL: pgsql/src/backend/utils/adt/ruleutils.c,v 1.277 2008/07/16 16:55:23 tgl Exp $
1313
*
1414
*-------------------------------------------------------------------------
1515
*/
@@ -5389,29 +5389,12 @@ generate_function_name(Oid funcid, int nargs, Oid *argtypes,
53895389
/* Check variadic-ness if caller cares */
53905390
if (is_variadic)
53915391
{
5392-
/* XXX change this if we simplify code in FuncnameGetCandidates */
5393-
Datum proargmodes;
5394-
bool isnull;
5395-
5396-
*is_variadic = false;
5397-
5398-
proargmodes = SysCacheGetAttr(PROCOID, proctup,
5399-
Anum_pg_proc_proargmodes, &isnull);
5400-
if (!isnull)
5401-
{
5402-
ArrayType *ar = DatumGetArrayTypeP(proargmodes);
5403-
char *argmodes;
5404-
int j;
5405-
5406-
argmodes = ARR_DATA_PTR(ar);
5407-
j = ARR_DIMS(ar)[0] - 1;
5408-
if (j >= 0 && argmodes[j] == PROARGMODE_VARIADIC)
5409-
{
5410-
/* "any" variadics are not treated as variadics for listing */
5411-
if (procform->proargtypes.values[j] != ANYOID)
5412-
*is_variadic = true;
5413-
}
5414-
}
5392+
/* "any" variadics are not treated as variadics for listing */
5393+
if (OidIsValid(procform->provariadic) &&
5394+
procform->provariadic != ANYOID)
5395+
*is_variadic = true;
5396+
else
5397+
*is_variadic = false;
54155398
}
54165399

54175400
ReleaseSysCache(proctup);

0 commit comments

Comments
 (0)