On Mon, 2025-08-11 at 17:39 +0800, Japin Li wrote: > > 1. > +static struct > +{ > + int transfn_oid; /* Transition function's funcoid. > Arrays are > + * sorted in ascending order */ > + Oid transtype; /* Transition data type */ > + PGFunction merge_trans; /* Function pointer set required for > parallel > + * aggregation for each transfn_oid > */ > + vci_aggtranstype_kind kind; /* If transtype is INTERNALOID, its > details */ > +} trans_funcs_table[] = { > + {F_FLOAT4_ACCUM, 1022, merge_floatX_accum, > VCI_AGG_NOT_INTERNAL}, /* 208 */ > + {F_FLOAT8_ACCUM, 1022, merge_floatX_accum, > VCI_AGG_NOT_INTERNAL}, /* 222 */ > + {F_INT8INC, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1833 */ > + {F_NUMERIC_ACCUM, 2281, numeric_combine, > VCI_AGG_NUMERIC_AGG_STATE}, /* 1834 */ > + {F_INT2_ACCUM, 2281, numeric_poly_combine, > VCI_AGG_POLY_NUM_AGG_STATE}, /* 1836 */ > + {F_INT4_ACCUM, 2281, numeric_poly_combine, > VCI_AGG_POLY_NUM_AGG_STATE}, /* 1835 */ > + {F_INT8_ACCUM, 2281, numeric_combine, > VCI_AGG_NUMERIC_AGG_STATE}, /* 1836 */ > + {F_INT2_SUM, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1840 */ > + {F_INT4_SUM, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1841 */ > + {F_INTERVAL_AVG_COMBINE, 2281, merge_interval_avg_accum, > VCI_AGG_NOT_INTERNAL}, /* 3325 */ > + {F_INT2_AVG_ACCUM, 1016, merge_intX_accum, > VCI_AGG_NOT_INTERNAL}, /* 1962 */ > + {F_INT4_AVG_ACCUM, 1016, merge_intX_accum, > VCI_AGG_NOT_INTERNAL}, /* 1963 */ > + {F_INT8INC_ANY, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 2804 */ > + {F_INT8_AVG_ACCUM, 2281, int8_avg_combine, > VCI_AGG_POLY_AVG_NUM_AGG_STATE}, /* 2746 */ > + {F_NUMERIC_AVG_ACCUM, 2281, numeric_avg_combine, > VCI_AGG_AVG_NUMERIC_AGG_STATE}, /* 2858 */ > +}; > > The comments state that this is sorted in ascending order, but the > code doesn't > follow that rule. While the current linear search works, a future > change to > binary search could cause problems. >
Hi Japin! I've looked at the code, vci_set_merge_and_copy_trans_funcs() function is unused and almost all code of vci_aggmergetranstype.c file including trans_funcs_table[] can be either removed either replaced with simple switch-case. Only transfn_oid fields of trans_funcs_table[] were actually used. Here is my patch made on top of v17. Peter, what do you think? Is it OK to remove those code? -- Regards, Timur Magomedov
From 08bd1cdfe1e0ee41776fe4d439b0febecaca09e8 Mon Sep 17 00:00:00 2001 From: Timur Magomedov <t.magome...@postgrespro.ru> Date: Wed, 13 Aug 2025 19:04:49 +0300 Subject: [PATCH] Removed vci_set_merge_and_copy_trans_funcs() The function was unused. Some code that only was used in this function was removed also. --- contrib/vci/executor/vci_aggmergetranstype.c | 395 +------------------ contrib/vci/include/postgresql_copy.h | 58 --- contrib/vci/include/vci_executor.h | 1 - contrib/vci/include/vci_pg_copy.h | 29 +- 4 files changed, 24 insertions(+), 459 deletions(-) diff --git a/contrib/vci/executor/vci_aggmergetranstype.c b/contrib/vci/executor/vci_aggmergetranstype.c index 4c136e8660a..52ac7cb2b05 100644 --- a/contrib/vci/executor/vci_aggmergetranstype.c +++ b/contrib/vci/executor/vci_aggmergetranstype.c @@ -36,74 +36,6 @@ #include "postgresql_copy.h" -static Datum merge_intX_accum(PG_FUNCTION_ARGS); -static Datum merge_floatX_accum(PG_FUNCTION_ARGS); -static Datum merge_interval_avg_accum(PG_FUNCTION_ARGS); - -static Datum copy_numeric_agg_state(Datum src, bool typByVal, int typLen); -static Datum copy_numeric_avg_agg_state(Datum src, bool typByVal, int typLen); -static Datum copy_poly_num_agg_state(Datum src, bool typByVal, int typLen); -static Datum copy_poly_avg_num_agg_state(Datum src, bool typByVal, int typLen); - -/** - * If Transition data is INTERNALOID, an enum indicating its type - */ -typedef enum vci_aggtranstype_kind -{ - VCI_AGG_NOT_INTERNAL, /* not INTERNALOID */ - VCI_AGG_NUMERIC_AGG_STATE, /* INTERNALOID (use NumericAggState struct) */ - VCI_AGG_POLY_NUM_AGG_STATE, /* INTERNALOID (use PolyNumAggState struct) */ - VCI_AGG_POLY_AVG_NUM_AGG_STATE, /* INTERNALOID (use PolyNumAggState - * without sumx2 struct) */ - VCI_AGG_AVG_NUMERIC_AGG_STATE, /* INTERNALOID (use NumericAggState - * without sumx2 struct) */ - VCI_AGG_ARRAY_BUILD_STATE, /* INTERNALOID (use ArrayBuildState struct) */ -} vci_aggtranstype_kind; - -/** - * Record information necessary for parallel aggregation for each transition function - */ -static struct -{ - int transfn_oid; /* Transition function's funcoid. Arrays are - * sorted in ascending order */ - Oid transtype; /* Transition data type */ - PGFunction merge_trans; /* Function pointer set required for parallel - * aggregation for each transfn_oid */ - vci_aggtranstype_kind kind; /* If transtype is INTERNALOID, its details */ -} trans_funcs_table[] = { - {F_FLOAT4_ACCUM, 1022, merge_floatX_accum, VCI_AGG_NOT_INTERNAL}, /* 208 */ - {F_FLOAT8_ACCUM, 1022, merge_floatX_accum, VCI_AGG_NOT_INTERNAL}, /* 222 */ - {F_INT8INC, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1833 */ - {F_NUMERIC_ACCUM, 2281, numeric_combine, VCI_AGG_NUMERIC_AGG_STATE}, /* 1834 */ - {F_INT2_ACCUM, 2281, numeric_poly_combine, VCI_AGG_POLY_NUM_AGG_STATE}, /* 1836 */ - {F_INT4_ACCUM, 2281, numeric_poly_combine, VCI_AGG_POLY_NUM_AGG_STATE}, /* 1835 */ - {F_INT8_ACCUM, 2281, numeric_combine, VCI_AGG_NUMERIC_AGG_STATE}, /* 1836 */ - {F_INT2_SUM, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1840 */ - {F_INT4_SUM, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 1841 */ - {F_INTERVAL_AVG_COMBINE, 2281, merge_interval_avg_accum, VCI_AGG_NOT_INTERNAL}, /* 3325 */ - {F_INT2_AVG_ACCUM, 1016, merge_intX_accum, VCI_AGG_NOT_INTERNAL}, /* 1962 */ - {F_INT4_AVG_ACCUM, 1016, merge_intX_accum, VCI_AGG_NOT_INTERNAL}, /* 1963 */ - {F_INT8INC_ANY, 20, int8pl, VCI_AGG_NOT_INTERNAL}, /* 2804 */ - {F_INT8_AVG_ACCUM, 2281, int8_avg_combine, VCI_AGG_POLY_AVG_NUM_AGG_STATE}, /* 2746 */ - {F_NUMERIC_AVG_ACCUM, 2281, numeric_avg_combine, VCI_AGG_AVG_NUMERIC_AGG_STATE}, /* 2858 */ -}; - -/** - * If INTEROID, copy functions, SEND function, RECV function - */ -static vci_agg_trans_copy_funcs trans_internal_type_funcs_table[] = { - {datumCopy, NULL, NULL}, /* VCI_AGG_NOT_INTERNAL */ - {copy_numeric_agg_state, numeric_serialize, numeric_deserialize}, /* VCI_AGG_NUMERIC_AGG_STATE - * */ - {copy_poly_num_agg_state, numeric_poly_serialize, numeric_poly_deserialize}, /* VCI_AGG_NUMERIC_POLY_AGG_STATE - * */ - {copy_poly_avg_num_agg_state, int8_avg_serialize, int8_avg_deserialize}, /* VCI_AGG_POLY_AVG_NUM_AGG_STATE - * */ - {copy_numeric_avg_agg_state, numeric_avg_serialize, numeric_avg_deserialize}, /* VCI_AGG_AVG_NUMERIC_AGG_STATE - * */ -}; - /** * Determine if the given aggregation function is a type that can be supported by VCI * @@ -166,15 +98,28 @@ vci_is_supported_aggregation(Aggref *aggref) } else { - int i; - - for (i = 0; i < lengthof(trans_funcs_table); i++) + switch (transfn_oid) { - if (transfn_oid == trans_funcs_table[i].transfn_oid) - { + case F_FLOAT4_ACCUM: + case F_FLOAT8_ACCUM: + case F_INT8INC: + case F_NUMERIC_ACCUM: + case F_INT2_ACCUM: + case F_INT4_ACCUM: + case F_INT8_ACCUM: + case F_INT2_SUM: + case F_INT4_SUM: + case F_INTERVAL_AVG_COMBINE: + case F_INT2_AVG_ACCUM: + case F_INT4_AVG_ACCUM: + case F_INT8INC_ANY: + case F_INT8_AVG_ACCUM: + case F_NUMERIC_AVG_ACCUM: ret = true; break; - } + + default: + break; } } @@ -185,305 +130,3 @@ vci_is_supported_aggregation(Aggref *aggref) return ret; } - -/** - * Obtain the merge and copy-related functions for the transition internal state - * generated by the given transition function. - * - * @param[in] transfn_oid specify a transition function - * @param[out] merge_trans_p set the merge function - * @param[out] copy_trans_p set the copy function - * @param[out] send_trans_p set the send function - * @param[out] recv_trans_p set the recv function - * - * @retval true Found the copy and merge function - * @retval false Not exist - * - * @note Output parameters are set only if the return value is true. - */ -bool -vci_set_merge_and_copy_trans_funcs(Oid transfn_oid, PGFunction *merge_trans_p, VciCopyDatumFunc *copy_trans_p, PGFunction *send_trans_p, PGFunction *recv_trans_p) -{ - int i; - - for (i = 0; i < lengthof(trans_funcs_table); i++) - { - if (transfn_oid == trans_funcs_table[i].transfn_oid) - { - vci_aggtranstype_kind kind = trans_funcs_table[i].kind; - - *merge_trans_p = trans_funcs_table[i].merge_trans; - *copy_trans_p = trans_internal_type_funcs_table[kind].copy_trans; - *send_trans_p = trans_internal_type_funcs_table[kind].send_trans; - *recv_trans_p = trans_internal_type_funcs_table[kind].recv_trans; - - return true; - } - } - - return false; -} - -static Datum -merge_intX_accum(PG_FUNCTION_ARGS) -{ - ArrayType *transarray0 = PG_GETARG_ARRAYTYPE_P(0); - ArrayType *transarray1 = PG_GETARG_ARRAYTYPE_P(1); - Int8TransTypeData *transdata0; - Int8TransTypeData *transdata1; - - transdata0 = (Int8TransTypeData *) ARR_DATA_PTR(transarray0); - transdata1 = (Int8TransTypeData *) ARR_DATA_PTR(transarray1); - - transdata0->count += transdata1->count; - transdata0->sum += transdata1->sum; - - PG_RETURN_ARRAYTYPE_P(transarray0); -} - -static Datum -merge_floatX_accum(PG_FUNCTION_ARGS) -{ - ArrayType *transarray1 = PG_GETARG_ARRAYTYPE_P(0); - ArrayType *transarray2 = PG_GETARG_ARRAYTYPE_P(1); - float8 *transvalues1; - float8 *transvalues2; - float8 N1, - Sx1, - Sxx1, - N2, - Sx2, - Sxx2, - tmp, - N, - Sx, - Sxx; - - transvalues1 = check_float8_array(transarray1, "merge_floatX_accum", 3); - transvalues2 = check_float8_array(transarray2, "merge_floatX_accum", 3); - - N1 = transvalues1[0]; - Sx1 = transvalues1[1]; - Sxx1 = transvalues1[2]; - - N2 = transvalues2[0]; - Sx2 = transvalues2[1]; - Sxx2 = transvalues2[2]; - - /*-------------------- - * The transition values combine using a generalization of the - * Youngs-Cramer algorithm as follows: - * - * N = N1 + N2 - * Sx = Sx1 + Sx2 - * Sxx = Sxx1 + Sxx2 + N1 * N2 * (Sx1/N1 - Sx2/N2)^2 / N; - * - * It's worth handling the special cases N1 = 0 and N2 = 0 separately - * since those cases are trivial, and we then don't need to worry about - * division-by-zero errors in the general case. - *-------------------- - */ - if (N1 == 0.0) - { - N = N2; - Sx = Sx2; - Sxx = Sxx2; - } - else if (N2 == 0.0) - { - N = N1; - Sx = Sx1; - Sxx = Sxx1; - } - else - { - N = N1 + N2; - /* Sx = float8_pl(Sx1, Sx2); */ - Sx = Sx1 + Sx2; - CHECKFLOATVAL(Sx, isinf(Sx1) || isinf(Sx2), true); - tmp = Sx1 / N1 - Sx2 / N2; - Sxx = Sxx1 + Sxx2 + N1 * N2 * tmp * tmp / N; - CHECKFLOATVAL(Sxx, isinf(Sxx1) || isinf(Sxx2), true); - } - - transvalues1[0] = N; - transvalues1[1] = Sx; - transvalues1[2] = Sxx; - - PG_RETURN_ARRAYTYPE_P(transarray1); -} - -static Datum -merge_interval_avg_accum(PG_FUNCTION_ARGS) -{ - IntervalAggState *state1; - IntervalAggState *state2; - - state1 = PG_ARGISNULL(0) ? NULL : (IntervalAggState *) PG_GETARG_POINTER(0); - state2 = PG_ARGISNULL(1) ? NULL : (IntervalAggState *) PG_GETARG_POINTER(1); - - - /* Create the state data on the first call */ - if (state2 == NULL) - PG_RETURN_POINTER(state1); - - if (state1 == NULL) - { - state1 = makeIntervalAggState(fcinfo); - - state1->N = state2->N; - state1->pInfcount = state2->pInfcount; - state1->nInfcount = state2->nInfcount; - - state1->sumX.day = state2->sumX.day; - state1->sumX.month = state2->sumX.month; - state1->sumX.time = state2->sumX.time; - - PG_RETURN_POINTER(state1); - } - - state1->N += state2->N; - state1->pInfcount += state2->pInfcount; - state1->nInfcount += state2->nInfcount; - - /* Accumulate finite interval values, if any. */ - if (state2->N > 0) - finite_interval_pl(&state1->sumX, &state2->sumX, &state1->sumX); - - PG_RETURN_POINTER(state1); -} - -/*****************************************************************************/ -/* numeric */ -/*****************************************************************************/ - -/* - * Copy an accumulator's state. - * - * 'dst' is assumed to be uninitialized beforehand. No attempt is made at - * freeing old values. - * - * copied from src/backend/utils/adt/numeric.c - */ -static void -accum_sum_copy(NumericSumAccum *dst, NumericSumAccum *src) -{ - dst->pos_digits = palloc0(src->ndigits * sizeof(int32)); - dst->neg_digits = palloc0(src->ndigits * sizeof(int32)); - - memcpy(dst->pos_digits, src->pos_digits, src->ndigits * sizeof(int32)); - memcpy(dst->neg_digits, src->neg_digits, src->ndigits * sizeof(int32)); - dst->num_uncarried = src->num_uncarried; - dst->ndigits = src->ndigits; - dst->weight = src->weight; - dst->dscale = src->dscale; -} - -static Datum -copy_numeric_agg_state(Datum src, bool typByVal, int typLen) -{ - NumericAggState *from, - *to; - - from = (NumericAggState *) DatumGetPointer(src); - - if (from == NULL) - return (Datum) NULL; - - to = palloc0(sizeof(NumericAggState)); - - to->calcSumX2 = from->calcSumX2; - to->agg_context = CurrentMemoryContext; - to->N = from->N; - to->NaNcount = from->NaNcount; - to->pInfcount = from->pInfcount; - to->nInfcount = from->nInfcount; - to->maxScale = from->maxScale; - to->maxScaleCount = from->maxScaleCount; - - accum_sum_copy(&to->sumX, &from->sumX); - accum_sum_copy(&to->sumX2, &from->sumX2); - - return PointerGetDatum(to); -} - -static Datum -copy_numeric_avg_agg_state(Datum src, bool typByVal, int typLen) -{ - NumericAggState *from, - *to; - - from = (NumericAggState *) DatumGetPointer(src); - - if (from == NULL) - return (Datum) NULL; - - to = palloc0(sizeof(NumericAggState)); - - to->calcSumX2 = from->calcSumX2; - to->agg_context = CurrentMemoryContext; - to->N = from->N; - to->NaNcount = from->NaNcount; - to->pInfcount = from->pInfcount; - to->nInfcount = from->nInfcount; - to->maxScale = from->maxScale; - to->maxScaleCount = from->maxScaleCount; - - accum_sum_copy(&to->sumX, &from->sumX); - - return PointerGetDatum(to); -} - -static Datum -copy_poly_num_agg_state(Datum src, bool typByVal, int typLen) -{ - PolyNumAggState *from, - *to; - - from = (PolyNumAggState *) DatumGetPointer(src); - - if (from == NULL) - return (Datum) NULL; - - to = palloc0(sizeof(PolyNumAggState)); - - to->calcSumX2 = from->calcSumX2; - to->N = from->N; - -#ifdef HAVE_INT128 - to->sumX = from->sumX; - to->sumX2 = from->sumX2; -#else - to->agg_context = CurrentMemoryContext; - accum_sum_copy(&to->sumX, &from->sumX); - accum_sum_copy(&to->sumX2, &from->sumX2); -#endif - - return PointerGetDatum(to); -} - -static Datum -copy_poly_avg_num_agg_state(Datum src, bool typByVal, int typLen) -{ - PolyNumAggState *from, - *to; - - from = (PolyNumAggState *) DatumGetPointer(src); - - if (from == NULL) - return (Datum) NULL; - - to = palloc0(sizeof(PolyNumAggState)); - - to->calcSumX2 = from->calcSumX2; - to->N = from->N; - -#ifdef HAVE_INT128 - to->sumX = from->sumX; -#else - to->agg_context = CurrentMemoryContext; - accum_sum_copy(&to->sumX, &from->sumX); -#endif - - return PointerGetDatum(to); -} diff --git a/contrib/vci/include/postgresql_copy.h b/contrib/vci/include/postgresql_copy.h index aec0bb97f9e..aedfe7a3184 100644 --- a/contrib/vci/include/postgresql_copy.h +++ b/contrib/vci/include/postgresql_copy.h @@ -60,70 +60,12 @@ check_float8_array(ArrayType *transarray, const char *caller, int n) return (float8 *) ARR_DATA_PTR(transarray); } -/* - * src/backend/utils/adt/numeric.c - */ -typedef struct NumericSumAccum -{ - int ndigits; - int weight; - int dscale; - int num_uncarried; - bool have_carry_space; - int32 *pos_digits; - int32 *neg_digits; -} NumericSumAccum; - -typedef struct NumericAggState -{ - bool calcSumX2; /* if true, calculate sumX2 */ - MemoryContext agg_context; /* context we're calculating in */ - int64 N; /* count of processed numbers */ - NumericSumAccum sumX; /* sum of processed numbers */ - NumericSumAccum sumX2; /* sum of squares of processed numbers */ - int maxScale; /* maximum scale seen so far */ - int64 maxScaleCount; /* number of values seen with maximum scale */ - /* These counts are *not* included in N! Use NA_TOTAL_COUNT() as needed */ - int64 NaNcount; /* count of NaN values */ - int64 pInfcount; /* count of +Inf values */ - int64 nInfcount; /* count of -Inf values */ -} NumericAggState; - typedef struct Int8TransTypeData { int64 count; int64 sum; } Int8TransTypeData; -/* - * Integer data types in general use Numeric accumulators to share code - * and avoid risk of overflow. - * - * However for performance reasons optimized special-purpose accumulator - * routines are used when possible. - * - * On platforms with 128-bit integer support, the 128-bit routines will be - * used when sum(X) or sum(X*X) fit into 128-bit. - * - * For 16 and 32 bit inputs, the N and sum(X) fit into 64-bit so the 64-bit - * accumulators will be used for SUM and AVG of these data types. - */ - -#ifdef HAVE_INT128 -typedef struct Int128AggState -{ - bool calcSumX2; /* if true, calculate sumX2 */ - int64 N; /* count of processed numbers */ - int128 sumX; /* sum of processed numbers */ - int128 sumX2; /* sum of squares of processed numbers */ -} Int128AggState; - -typedef Int128AggState PolyNumAggState; -#else -typedef NumericAggState PolyNumAggState; -#endif - - #ifdef VCI_USE_CMP_FUNC /* * interval_relop - is interval1 relop interval2 diff --git a/contrib/vci/include/vci_executor.h b/contrib/vci/include/vci_executor.h index af9fc539c9b..2f57b85b380 100644 --- a/contrib/vci/include/vci_executor.h +++ b/contrib/vci/include/vci_executor.h @@ -857,7 +857,6 @@ typedef struct vci_agg_trans_copy_funcs } vci_agg_trans_copy_funcs; extern bool vci_is_supported_aggregation(Aggref *aggref); -extern bool vci_set_merge_and_copy_trans_funcs(Oid transfn_oid, PGFunction *merge_trans_p, VciCopyDatumFunc *copy_trans_p, PGFunction *send_trans_p, PGFunction *recv_trans_p); /* ---------------- * vci_gather.c diff --git a/contrib/vci/include/vci_pg_copy.h b/contrib/vci/include/vci_pg_copy.h index ff6b465baed..8b0f2860998 100644 --- a/contrib/vci/include/vci_pg_copy.h +++ b/contrib/vci/include/vci_pg_copy.h @@ -1,17 +1,17 @@ /*------------------------------------------------------------------------- * - * vci_numeric.h + * vci_pg_copy.h * Some useful functions of PostgreSQL * * Portions Copyright (c) 2025, PostgreSQL Global Development Group * * IDENTIFICATION - * contrib/vci/include/vci_numeric.h + * contrib/vci/include/vci_pg_copy.h *------------------------------------------------------------------------- */ -#ifndef VCI_NUMERIC_H -#define VCI_NUMERIC_H +#ifndef VCI_PG_COPY_H +#define VCI_PG_COPY_H #include "datatype/timestamp.h" @@ -54,23 +54,4 @@ typedef struct NumericVar NumericDigit *digits; /* base-NBASE digits */ } NumericVar; -/* taken from src/backend/utils/adt/timestamp.c */ -static inline TimeOffset -interval_cmp_value(const Interval *interval) -{ - TimeOffset span; - - span = interval->time; - -#ifdef HAVE_INT64_TIMESTAMP - span += interval->month * INT64CONST(30) * USECS_PER_DAY; - span += interval->day * INT64CONST(24) * USECS_PER_HOUR; -#else - span += interval->month * ((double) DAYS_PER_MONTH * SECS_PER_DAY); - span += interval->day * ((double) HOURS_PER_DAY * SECS_PER_HOUR); -#endif - - return span; -} - -#endif /* #ifndef VCI_NUMERIC_H */ +#endif /* #ifndef VCI_PG_COPY_H */ -- 2.43.0