Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitf8ace54

Browse files
committed
Fix type-safety problem with parallel aggregate serial/deserialization.
The original specification for this called for the deserialization functionto have signature "deserialize(serialtype) returns transtype", which is asecurity violation if transtype is INTERNAL (which it always would be inpractice) and serialtype is not (which ditto). The patch blithely overrodethe opr_sanity check for that, which was sloppy-enough work in itself,but the indisputable reason this cannot be allowed to stand is that CREATEFUNCTION will reject such a signature and thus it'd be impossible forextensions to create parallelizable aggregates.The minimum fix to make the signature type-safe is to add a second, dummyargument of type INTERNAL. But to lock it down a bit more and make misuseof INTERNAL-accepting functions less likely, let's get rid of the abilityto specify a "serialtype" for an aggregate and just say that the onlyuseful serialtype is BYTEA --- which, in practice, is the only interestingvalue anyway, due to the usefulness of the send/recv infrastructure forthis purpose. That means we only have to allow "serialize(internal)returns bytea" and "deserialize(bytea, internal) returns internal" asthe signatures for these support functions.In passing fix bogus signature of int4_avg_combine, which I found thanksto adding an opr_sanity check on combinefunc signatures.catversion bump due to removing pg_aggregate.aggserialtype and adjustingsignatures of assorted built-in functions.David Rowley and Tom LaneDiscussion: <27247.1466185504@sss.pgh.pa.us>
1 parente45e990 commitf8ace54

File tree

18 files changed

+506
-715
lines changed

18 files changed

+506
-715
lines changed

‎doc/src/sgml/catalogs.sgml

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -463,12 +463,6 @@
463463
<entry><literal><link linkend="catalog-pg-type"><structname>pg_type</structname></link>.oid</literal></entry>
464464
<entry>Data type of the aggregate function's internal transition (state) data</entry>
465465
</row>
466-
<row>
467-
<entry><structfield>aggserialtype</structfield></entry>
468-
<entry><type>oid</type></entry>
469-
<entry><literal><link linkend="catalog-pg-type"><structname>pg_type</structname></link>.oid</literal></entry>
470-
<entry>Return data type of the aggregate function's serialization function (zero if none)</entry>
471-
</row>
472466
<row>
473467
<entry><structfield>aggtransspace</structfield></entry>
474468
<entry><type>int4</type></entry>

‎doc/src/sgml/ref/create_aggregate.sgml

Lines changed: 49 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ CREATE AGGREGATE <replaceable class="parameter">name</replaceable> ( [ <replacea
3030
[ , COMBINEFUNC = <replaceable class="PARAMETER">combinefunc</replaceable> ]
3131
[ , SERIALFUNC = <replaceable class="PARAMETER">serialfunc</replaceable> ]
3232
[ , DESERIALFUNC = <replaceable class="PARAMETER">deserialfunc</replaceable> ]
33-
[ , SERIALTYPE = <replaceable class="PARAMETER">serialtype</replaceable> ]
3433
[ , INITCOND = <replaceable class="PARAMETER">initial_condition</replaceable> ]
3534
[ , MSFUNC = <replaceable class="PARAMETER">msfunc</replaceable> ]
3635
[ , MINVFUNC = <replaceable class="PARAMETER">minvfunc</replaceable> ]
@@ -50,10 +49,6 @@ CREATE AGGREGATE <replaceable class="parameter">name</replaceable> ( [ [ <replac
5049
[ , SSPACE = <replaceable class="PARAMETER">state_data_size</replaceable> ]
5150
[ , FINALFUNC = <replaceable class="PARAMETER">ffunc</replaceable> ]
5251
[ , FINALFUNC_EXTRA ]
53-
[ , COMBINEFUNC = <replaceable class="PARAMETER">combinefunc</replaceable> ]
54-
[ , SERIALFUNC = <replaceable class="PARAMETER">serialfunc</replaceable> ]
55-
[ , DESERIALFUNC = <replaceable class="PARAMETER">deserialfunc</replaceable> ]
56-
[ , SERIALTYPE = <replaceable class="PARAMETER">serialtype</replaceable> ]
5752
[ , INITCOND = <replaceable class="PARAMETER">initial_condition</replaceable> ]
5853
[ , HYPOTHETICAL ]
5954
[ , PARALLEL = { SAFE | RESTRICTED | UNSAFE } ]
@@ -72,7 +67,6 @@ CREATE AGGREGATE <replaceable class="PARAMETER">name</replaceable> (
7267
[ , COMBINEFUNC = <replaceable class="PARAMETER">combinefunc</replaceable> ]
7368
[ , SERIALFUNC = <replaceable class="PARAMETER">serialfunc</replaceable> ]
7469
[ , DESERIALFUNC = <replaceable class="PARAMETER">deserialfunc</replaceable> ]
75-
[ , SERIALTYPE = <replaceable class="PARAMETER">serialtype</replaceable> ]
7670
[ , INITCOND = <replaceable class="PARAMETER">initial_condition</replaceable> ]
7771
[ , MSFUNC = <replaceable class="PARAMETER">msfunc</replaceable> ]
7872
[ , MINVFUNC = <replaceable class="PARAMETER">minvfunc</replaceable> ]
@@ -255,7 +249,7 @@ SELECT col FROM tab ORDER BY col USING sortop LIMIT 1;
255249
To be able to create an aggregate function, you must
256250
have <literal>USAGE</literal> privilege on the argument types, the state
257251
type(s), and the return type, as well as <literal>EXECUTE</literal>
258-
privilege on thetransition and final functions.
252+
privilege on thesupporting functions.
259253
</para>
260254
</refsect1>
261255

@@ -412,38 +406,51 @@ SELECT col FROM tab ORDER BY col USING sortop LIMIT 1;
412406
<term><replaceable class="PARAMETER">combinefunc</replaceable></term>
413407
<listitem>
414408
<para>
415-
The <replaceable class="PARAMETER">combinefunc</replaceable> may
416-
optionally be specified in order to allow the aggregate function to
417-
support partial aggregation. This is a prerequisite to allow the
418-
aggregate to participate in certain optimizations such as parallel
419-
aggregation.
409+
The <replaceable class="PARAMETER">combinefunc</replaceable> function
410+
may optionally be specified to allow the aggregate function to support
411+
partial aggregation. This is a prerequisite to allow the aggregate to
412+
participate in certain optimizations such as parallel aggregation.
420413
</para>
421414

422415
<para>
423-
This function can be thought of as an <replaceable class="PARAMETER">
424-
sfunc</replaceable>, where instead of acting upon individual input rows
425-
and adding these to the aggregate state, it adds other aggregate states
426-
to the aggregate state.
416+
If provided,
417+
the <replaceable class="PARAMETER">combinefunc</replaceable> must
418+
combine two <replaceable class="PARAMETER">state_data_type</replaceable>
419+
values, each containing the result of aggregation over some subset of
420+
the input values, to produce a
421+
new <replaceable class="PARAMETER">state_data_type</replaceable> that
422+
represents the result of aggregating over both sets of inputs. This
423+
function can be thought of as
424+
an <replaceable class="PARAMETER">sfunc</replaceable>, where instead of
425+
acting upon individual input rows and adding these to the aggregate
426+
state, it adds another aggregate state to the aggregate state.
427+
Typically, it is not possible to define
428+
a <replaceable class="PARAMETER">combinefunc</replaceable> for aggregate
429+
functions that are sensitive to the order of the input values, since the
430+
relative ordering of the inputs that went into the subset states is
431+
indeterminate.
427432
</para>
428433

429434
<para>
430435
The <replaceable class="PARAMETER">combinefunc</replaceable> must accept
431-
two arguments of <replaceable class="PARAMETER">state_data_type
432-
</replaceable> and return <replaceable class="PARAMETER">state_data_type
433-
</replaceable>. Optionally this function may be <quote>strict</quote>. In
434-
this case the function will not be called when either of the input states
435-
are null.
436+
two arguments of
437+
the <replaceable class="PARAMETER">state_data_type</replaceable> and
438+
return a value of
439+
the <replaceable class="PARAMETER">state_data_type</replaceable>.
440+
Optionally this function may be <quote>strict</quote>. In this case the
441+
function will not be called when either of the input states are null;
442+
the other state will be taken as the correct result.
436443
</para>
437-
444+
438445
<para>
439-
For aggregate functions with an <literal>INTERNAL</literal>
440-
<replaceable class="PARAMETER">state_data_type</replaceable> the
441-
<replaceable class="PARAMETER">combinefunc</replaceable> must not be
442-
<quote>strict</quote>. In this scenario the
443-
<replaceable class="PARAMETER">combinefunc</replaceable> must take charge
444-
and ensure thatthenull states are handled correctly and that the state
445-
being returned is a pointer to memory which belongs in the aggregate
446-
memory context.
446+
For aggregate functions
447+
whose<replaceable class="PARAMETER">state_data_type</replaceable>
448+
is <type>internal</type>,
449+
the <replaceable class="PARAMETER">combinefunc</replaceable> must not be
450+
strict. In this scenario
451+
the<replaceable class="PARAMETER">combinefunc</replaceable> must ensure
452+
that null states are handled correctly and that the state being returned
453+
is properly stored in the aggregatememory context.
447454
</para>
448455
</listitem>
449456
</varlistentry>
@@ -452,14 +459,13 @@ SELECT col FROM tab ORDER BY col USING sortop LIMIT 1;
452459
<term><replaceable class="PARAMETER">serialfunc</replaceable></term>
453460
<listitem>
454461
<para>
455-
In order to allow aggregate functions with an <literal>INTERNAL</>
456-
<replaceable class="PARAMETER">state_data_type</replaceable> to
457-
participate in parallel aggregation, the aggregate must have a valid
458-
<replaceable class="PARAMETER">serialfunc</replaceable>, which must
459-
serialize the aggregate state into <replaceable class="PARAMETER">
460-
serialtype</replaceable>. This function must take a single argument of
461-
<replaceable class="PARAMETER">state_data_type</replaceable> and return
462-
<replaceable class="PARAMETER">serialtype</replaceable>. A
462+
An aggregate function
463+
whose <replaceable class="PARAMETER">state_data_type</replaceable>
464+
is <type>internal</> can participate in parallel aggregation only if it
465+
has a <replaceable class="PARAMETER">serialfunc</replaceable> function,
466+
which must serialize the aggregate state into a <type>bytea</> value for
467+
transmission to another process. This function must take a single
468+
argument of type <type>internal</> and return type <type>bytea</>. A
463469
corresponding <replaceable class="PARAMETER">deserialfunc</replaceable>
464470
is also required.
465471
</para>
@@ -470,21 +476,12 @@ SELECT col FROM tab ORDER BY col USING sortop LIMIT 1;
470476
<term><replaceable class="PARAMETER">deserialfunc</replaceable></term>
471477
<listitem>
472478
<para>
473-
Deserializes a previously serialized aggregate state back into
479+
Deserialize a previously serialized aggregate state back into
474480
<replaceable class="PARAMETER">state_data_type</replaceable>. This
475-
function must take a single argument of <replaceable class="PARAMETER">
476-
serialtype</replaceable> and return <replaceable class="PARAMETER">
477-
state_data_type</replaceable>.
478-
</para>
479-
</listitem>
480-
</varlistentry>
481-
482-
<varlistentry>
483-
<term><replaceable class="PARAMETER">serialtype</replaceable></term>
484-
<listitem>
485-
<para>
486-
The data type to into which an <literal>INTERNAL</literal> aggregate
487-
state should be serialized.
481+
function must take two arguments of types <type>bytea</>
482+
and <type>internal</>, and produce a result of type <type>internal</>.
483+
(Note: the second, <type>internal</> argument is unused, but is required
484+
for type safety reasons.)
488485
</para>
489486
</listitem>
490487
</varlistentry>

‎src/backend/catalog/pg_aggregate.c

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@ AggregateCreate(const char *aggName,
6767
boolmfinalfnExtraArgs,
6868
List*aggsortopName,
6969
OidaggTransType,
70-
OidaggSerialType,
7170
int32aggTransSpace,
7271
OidaggmTransType,
7372
int32aggmTransSpace,
@@ -440,44 +439,42 @@ AggregateCreate(const char *aggName,
440439
}
441440

442441
/*
443-
* Validate the serialization function, if present. We must ensure that
444-
* the return type of this function is the same as the specified
445-
* serialType.
442+
* Validate the serialization function, if present.
446443
*/
447444
if (aggserialfnName)
448445
{
449-
fnArgs[0]=aggTransType;
446+
fnArgs[0]=INTERNALOID;
450447

451448
serialfn=lookup_agg_function(aggserialfnName,1,
452449
fnArgs,variadicArgType,
453450
&rettype);
454451

455-
if (rettype!=aggSerialType)
452+
if (rettype!=BYTEAOID)
456453
ereport(ERROR,
457454
(errcode(ERRCODE_DATATYPE_MISMATCH),
458455
errmsg("return type of serialization function %s is not %s",
459456
NameListToString(aggserialfnName),
460-
format_type_be(aggSerialType))));
457+
format_type_be(BYTEAOID))));
461458
}
462459

463460
/*
464-
* Validate the deserialization function, if present. We must ensure that
465-
* the return type of this function is the same as the transType.
461+
* Validate the deserialization function, if present.
466462
*/
467463
if (aggdeserialfnName)
468464
{
469-
fnArgs[0]=aggSerialType;
465+
fnArgs[0]=BYTEAOID;
466+
fnArgs[1]=INTERNALOID;/* dummy argument for type safety */
470467

471-
deserialfn=lookup_agg_function(aggdeserialfnName,1,
468+
deserialfn=lookup_agg_function(aggdeserialfnName,2,
472469
fnArgs,variadicArgType,
473470
&rettype);
474471

475-
if (rettype!=aggTransType)
472+
if (rettype!=INTERNALOID)
476473
ereport(ERROR,
477474
(errcode(ERRCODE_DATATYPE_MISMATCH),
478475
errmsg("return type of deserialization function %s is not %s",
479476
NameListToString(aggdeserialfnName),
480-
format_type_be(aggTransType))));
477+
format_type_be(INTERNALOID))));
481478
}
482479

483480
/*
@@ -661,7 +658,6 @@ AggregateCreate(const char *aggName,
661658
values[Anum_pg_aggregate_aggmfinalextra-1]=BoolGetDatum(mfinalfnExtraArgs);
662659
values[Anum_pg_aggregate_aggsortop-1]=ObjectIdGetDatum(sortop);
663660
values[Anum_pg_aggregate_aggtranstype-1]=ObjectIdGetDatum(aggTransType);
664-
values[Anum_pg_aggregate_aggserialtype-1]=ObjectIdGetDatum(aggSerialType);
665661
values[Anum_pg_aggregate_aggtransspace-1]=Int32GetDatum(aggTransSpace);
666662
values[Anum_pg_aggregate_aggmtranstype-1]=ObjectIdGetDatum(aggmTransType);
667663
values[Anum_pg_aggregate_aggmtransspace-1]=Int32GetDatum(aggmTransSpace);
@@ -688,8 +684,7 @@ AggregateCreate(const char *aggName,
688684
* Create dependencies for the aggregate (above and beyond those already
689685
* made by ProcedureCreate). Note: we don't need an explicit dependency
690686
* on aggTransType since we depend on it indirectly through transfn.
691-
* Likewise for aggmTransType using the mtransfunc, and also for
692-
* aggSerialType using the serialfn, if they exist.
687+
* Likewise for aggmTransType using the mtransfunc, if it exists.
693688
*/
694689

695690
/* Depends on transition function */

‎src/backend/commands/aggregatecmds.c

Lines changed: 8 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
7272
List*sortoperatorName=NIL;
7373
TypeName*baseType=NULL;
7474
TypeName*transType=NULL;
75-
TypeName*serialType=NULL;
7675
TypeName*mtransType=NULL;
7776
int32transSpace=0;
7877
int32mtransSpace=0;
@@ -88,7 +87,6 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
8887
List*parameterDefaults;
8988
OidvariadicArgType;
9089
OidtransTypeId;
91-
OidserialTypeId=InvalidOid;
9290
OidmtransTypeId=InvalidOid;
9391
chartransTypeType;
9492
charmtransTypeType=0;
@@ -164,8 +162,6 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
164162
}
165163
elseif (pg_strcasecmp(defel->defname,"stype")==0)
166164
transType=defGetTypeName(defel);
167-
elseif (pg_strcasecmp(defel->defname,"serialtype")==0)
168-
serialType=defGetTypeName(defel);
169165
elseif (pg_strcasecmp(defel->defname,"stype1")==0)
170166
transType=defGetTypeName(defel);
171167
elseif (pg_strcasecmp(defel->defname,"sspace")==0)
@@ -333,73 +329,25 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
333329
format_type_be(transTypeId))));
334330
}
335331

336-
if (serialType)
332+
if (serialfuncName&&deserialfuncName)
337333
{
338334
/*
339-
* There's little point in having a serialization/deserialization
340-
* function on aggregates that don't have an internal state, so let's
341-
* just disallow this as it may help clear up any confusion or
342-
* needless authoring of these functions.
335+
* Serialization is only needed/allowed for transtype INTERNAL.
343336
*/
344337
if (transTypeId!=INTERNALOID)
345338
ereport(ERROR,
346339
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
347-
errmsg("aserializationtype must onlybe specified when the aggregate transition data type is %s",
340+
errmsg("serializationfunctions maybe specified only when the aggregate transition data type is %s",
348341
format_type_be(INTERNALOID))));
349-
350-
serialTypeId=typenameTypeId(NULL,serialType);
351-
352-
if (get_typtype(mtransTypeId)==TYPTYPE_PSEUDO&&
353-
!IsPolymorphicType(serialTypeId))
354-
ereport(ERROR,
355-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
356-
errmsg("aggregate serialization data type cannot be %s",
357-
format_type_be(serialTypeId))));
358-
359-
/*
360-
* We disallow INTERNAL serialType as the whole point of the
361-
* serialized types is to allow the aggregate state to be output, and
362-
* we cannot output INTERNAL. This check, combined with the one above
363-
* ensures that the trans type and serialization type are not the
364-
* same.
365-
*/
366-
if (serialTypeId==INTERNALOID)
367-
ereport(ERROR,
368-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
369-
errmsg("aggregate serialization data type cannot be %s",
370-
format_type_be(serialTypeId))));
371-
372-
/*
373-
* If serialType is specified then serialfuncName and deserialfuncName
374-
* must be present; if not, then none of the serialization options
375-
* should have been specified.
376-
*/
377-
if (serialfuncName==NIL)
378-
ereport(ERROR,
379-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
380-
errmsg("aggregate serialization function must be specified when serialization type is specified")));
381-
382-
if (deserialfuncName==NIL)
383-
ereport(ERROR,
384-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
385-
errmsg("aggregate deserialization function must be specified when serialization type is specified")));
386342
}
387-
else
343+
elseif (serialfuncName||deserialfuncName)
388344
{
389345
/*
390-
* If serialization type was not specified then there shouldn't be a
391-
* serialization function.
346+
* Cannot specify one function without the other.
392347
*/
393-
if (serialfuncName!=NIL)
394-
ereport(ERROR,
395-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
396-
errmsg("must specify serialization type when specifying serialization function")));
397-
398-
/* likewise for the deserialization function */
399-
if (deserialfuncName!=NIL)
400-
ereport(ERROR,
401-
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
402-
errmsg("must specify serialization type when specifying deserialization function")));
348+
ereport(ERROR,
349+
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
350+
errmsg("must specify both or neither of serialization and deserialization functions")));
403351
}
404352

405353
/*
@@ -493,7 +441,6 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters,
493441
mfinalfuncExtraArgs,
494442
sortoperatorName,/* sort operator name */
495443
transTypeId,/* transition data type */
496-
serialTypeId,/* serialization data type */
497444
transSpace,/* transition space */
498445
mtransTypeId,/* transition data type */
499446
mtransSpace,/* transition space */

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp