agora inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedR: 14.1 immutable function, bad performance if check number = 'NaN'
4+ messages / 3 participants
[nested] [flat]
* R: 14.1 immutable function, bad performance if check number = 'NaN'
@ 2022-04-26 07:45 Federico Travaglini <federico.travaglini@collaboration.aubay.it>
2022-04-26 14:11 ` Re: R: 14.1 immutable function, bad performance if check number = 'NaN' Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 4+ messages in thread
From: Federico Travaglini @ 2022-04-26 07:45 UTC (permalink / raw)
To: Merlin Moncure <mmoncure@gmail.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Good morning, thank you very much for the time you spent for my question.
Yes inlining could be the problem, because maybe does not allow to use the
IMMUTABLE feature?
The context of the query is quite complex, therefore I avoided to provide
it in previous email
Here it is what I tested. I’s a code fragment from a bigger procedure. The
strings in green are passed as parameters, as well as the thresholds
1,2,3,4,5. To test just this fragment of code I replaced them with fixed
values
*SET* random_page_cost = 0.1; (otherwise it takes more than 4 minutes in
place of 33 sec)
*EXPLAIN* (*ANALYZE*, BUFFERS, *verbose*)
*select*
* *from*
(
*select*
tms,
fh.file_id,
(e.measure_list #>> ('{' || 'cluster_comuni_italiani' ||
',s}')::*text*[]) *as* value_s_1,
(e.measure_list #> ('{' || 'cluster_comuni_italiani' ||
',n}')::*text*[])::*numeric* *as* value_n_1,
(e.measure_list #> ('{' || 'cluster_comuni_italiani' ||
',o}')::*text*[])::*numeric* *as* value_o_1,
antsgeo_get_severity_thr((e.measure_list #> ('{' ||
'cluster_comuni_italiani' || ',o}')::*text*[])::*numeric*, 1, 2, 3, 4, 5)
*AS* severity_1,
(e.measure_list #>> ('{' ||
'act_geoposition_pers_act_confidence' || ',s}')::*text*[]) *as* value_s_2,
(e.measure_list #> ('{' ||
'act_geoposition_pers_act_confidence' || ',n}')::*text*[])::*numeric* *as*
value_n_2,
(e.measure_list #> ('{' ||
'act_geoposition_pers_act_confidence' || ',o}')::*text*[])::*numeric* *as*
value_o_2,
antsgeo_get_severity_thr((e.measure_list #> ('{' ||
'act_geoposition_pers_act_confidence' || ',o}')::*text*[])::*numeric*, 1, 2,
3, 4, 5) *AS* severity_2,
(e.measure_list #>> ('{' || 'act_coverage_band_pcell' ||
',s}')::*text*[]) *as* value_s_3,
(e.measure_list #> ('{' || 'act_coverage_band_pcell' ||
',n}')::*text*[])::*numeric* *as* value_n_3,
(e.measure_list #> ('{' || 'act_coverage_band_pcell' ||
',o}')::*text*[])::*numeric* *as* value_o_3,
antsgeo_get_severity_thr((e.measure_list #> ('{' ||
'act_coverage_band_pcell' || ',o}')::*text*[])::*numeric*, 1, 2, 3, 4, 5)
*AS* severity_3,
(e.measure_list #>> ('{' || *null*::*text* ||
',s}')::*text*[])
*as* value_s_4,
(e.measure_list #> ('{' || *null*::*text* || ',n}')::*text*
[])::*numeric* *as* value_n_4,
(e.measure_list #> ('{' || *null*::*text* || ',o}')::*text*
[])::*numeric* *as* value_o_4,
antsgeo_get_severity_thr((e.measure_list #> ('{' || *null*::
*text* || ',o}')::*text*[])::*numeric*, 1, 2, 3, 4, 5) *AS* severity_4
*from*
file_hist fh,
geo_measr_sample e
*where*
(
(fh.agn_group_id = 21)
*and* fh.data_min_tms <= '2022-04-25 00:00:00' *and*
fh.data_max_tms >= '2022-02-28 00:00:00' --lo usa
)
*and* fh.act_id = e.act_id
*and* (e.tms >= '2022-02-28 00:00:00' *and* e.tms <=
'2022-04-25
00:00:00')
*and* (e.measure_list #>> ('{act_edit,s}')::*text*[] *not*
*in* ('excld') *or* e.measure_list #>> ('{act_edit,s}')::*text*[] *is*
*null*)
)t1
e.measure_list is a jsonb, with a variable structure
{
"act_plmn": {
"s": "222/1"
},
"struct_day": {
"s": "2022-04-22"
},
"struct_week": {
"s": "2022-04-18"
},
"act_plmn_name": {
"s": "Tim.Ita (222-01)"
},
"struct_act_id": {
"s": "1809464"
},
"struct_tc_name": {
"s": "VoiceCall_MO"
},
"struct_yyyy_mm": {
"s": "2022-04"
},
"act_coverage_ci": {
"s": "63"
},
"act_coverage_ta": {
"n": 4,
"o": 4
},
"act_environment": {
"s": "in-door"
},
"cell_code_pcell": {
"s": "FE23E3"
},
"struct_act_code": {
"s": "20220422_164238_SDTU100010.01"
},
"struct_act_name": {
"s": "20220422_164238_SDTU100010.01. Copy of Voice MO 0687201815"
},…
Nested Loop (cost=0.43..2055500.00 rows=1441783 width=524) (actual
time=0.761..33647.744 rows=415401 loops=1)
Output: e.tms, fh.file_id, (e.measure_list #>>
('{cluster_comuni_italiani,s}'::cstring)::text[]), ((e.measure_list #>
('{cluster_comuni_italiani,n}'::cstring)::text[]))::numeric,
((e.measure_list #>
('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric, CASE WHEN
((((e.measure_list #>
('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
precision >= '4'::double precision) THEN '1 Clear'::text WHEN
((((e.measure_list #>
('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
precision >= '3'::double precision) THEN '2 Warning'::text WHEN
((((e.measure_list #>
('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
precision >= '2'::double precision) THEN '3 Minor'::text WHEN
((((e.measure_list #>
('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
precision >= '1'::double precision) THEN '4 Major'::text WHEN
((((e.measure_list #>
('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
precision < '1'::double precision) THEN '5 Critical'::text ELSE '6
Unk'::text END, (e.measure_list #>>
('{act_geoposition_pers_act_confidence,s}'::cstring)::text[]),
((e.measure_list #>
('{act_geoposition_pers_act_confidence,n}'::cstring)::text[]))::numeric,
((e.measure_list #>
('{act_geoposition_pers_act_confidence,o}'::cstring)::text[]))::numeric,
CASE WHEN ((((e.measure_list #>
('{act_geoposition_pers_act_confidence,o}'::cstring)::text[]))::numeric)::double
precision >= '4'::double precision) THEN '1 Clear'::text WHEN
((((e.measure_list #>
('{act_geoposition_pers_act_confidence,o}'::cstring)::text[]))::numeric)::double
precision >= '3'::double precision) THEN '2 Warning'::text WHEN
((((e.measure_list #>
('{act_geoposition_pers_act_confidence,o}'::cstring)::text[]))::numeric)::double
precision >= '2'::double precision) THEN '3 Minor'::text WHEN
((((e.measure_list #>
('{act_geoposition_pers_act_confidence,o}'::cstring)::text[]))::numeric)::double
precision >= '1'::double precision) THEN '4 Major'::text WHEN
((((e.measure_list #>
('{act_geoposition_pers_act_confidence,o}'::cstring)::text[]))::numeric)::double
precision < '1'::double precision) THEN '5 Critical'::text ELSE '6
Unk'::text END, (e.measure_list #>>
('{act_coverage_band_pcell,s}'::cstring)::text[]), ((e.measure_list #>
('{act_coverage_band_pcell,n}'::cstring)::text[]))::numeric,
((e.measure_list #>
('{act_coverage_band_pcell,o}'::cstring)::text[]))::numeric, CASE WHEN
((((e.measure_list #>
('{act_coverage_band_pcell,o}'::cstring)::text[]))::numeric)::double
precision >= '4'::double precision) THEN '1 Clear'::text WHEN
((((e.measure_list #>
('{act_coverage_band_pcell,o}'::cstring)::text[]))::numeric)::double
precision >= '3'::double precision) THEN '2 Warning'::text WHEN
((((e.measure_list #>
('{act_coverage_band_pcell,o}'::cstring)::text[]))::numeric)::double
precision >= '2'::double precision) THEN '3 Minor'::text WHEN
((((e.measure_list #>
('{act_coverage_band_pcell,o}'::cstring)::text[]))::numeric)::double
precision >= '1'::double precision) THEN '4 Major'::text WHEN
((((e.measure_list #>
('{act_coverage_band_pcell,o}'::cstring)::text[]))::numeric)::double
precision < '1'::double precision) THEN '5 Critical'::text ELSE '6
Unk'::text END, NULL::text, NULL::numeric, NULL::numeric, '6 Unk'::text
Buffers: shared hit=365255
-> Seq Scan on geo_ants.file_hist fh (cost=0.00..443.28 rows=311
width=8) (actual time=0.698..1.434 rows=315 loops=1)
Output: fh.file_id, fh.file_name, fh.rtu, fh.port, fh.act_code,
fh.file_size, fh.file_tms, fh.loaded_tms, fh.update_tms, fh.status,
fh.data_min_tms, fh.data_max_tms, fh.enh_tms, fh.file_type,
fh.partial_output_flag, fh.record_count, fh.status_description,
fh.act_lenght, fh.act_id, fh.file_act_done, fh.enh_start_tms, fh.agn_code,
fh.agn_group_id, fh.ts_sched_id, fh.ts_sched_ver, fh.enh_attempt,
fh.act_done_list, fh.data_max_proc_tms, fh.data_max_loaded_tms,
fh.error_count, fh.dbg_mode
Filter: ((fh.data_min_tms <= '2022-04-25 00:00:00'::timestamp
without time zone) AND (fh.data_max_tms >= '2022-02-28 00:00:00'::timestamp
without time zone) AND (fh.agn_group_id = 21))
Rows Removed by Filter: 3358
Buffers: shared hit=379
-> Append (cost=0.43..4609.77 rows=57257 width=1552) (actual
time=0.012..9.971 rows=1319 loops=315)
Buffers: shared hit=106416
-> Index Scan using geo_measr_sample_2022_02_act_id_tms_idx on
geo_ants.geo_measr_sample_2022_02 e_1 (cost=0.43..14.42 rows=166
width=1362) (actual time=0.003..0.003 rows=0 loops=315)
Output: e_1.tms, e_1.measure_list, e_1.act_id
Index Cond: ((e_1.act_id = fh.act_id) AND (e_1.tms >=
'2022-02-28 00:00:00'::timestamp without time zone) AND (e_1.tms <=
'2022-04-25 00:00:00'::timestamp without time zone))
Filter: (((e_1.measure_list #>> '{act_edit,s}'::text[]) <>
'excld'::text) OR ((e_1.measure_list #>> '{act_edit,s}'::text[]) IS NULL))
Buffers: shared hit=946
-> Index Scan using geo_measr_sample_2022_03_act_id_tms_idx on
geo_ants.geo_measr_sample_2022_03 e_2 (cost=0.56..2333.98 rows=30845
width=1552) (actual time=0.006..7.586 rows=1061 loops=315)
Output: e_2.tms, e_2.measure_list, e_2.act_id
Index Cond: ((e_2.act_id = fh.act_id) AND (e_2.tms >=
'2022-02-28 00:00:00'::timestamp without time zone) AND (e_2.tms <=
'2022-04-25 00:00:00'::timestamp without time zone))
Filter: (((e_2.measure_list #>> '{act_edit,s}'::text[]) <>
'excld'::text) OR ((e_2.measure_list #>> '{act_edit,s}'::text[]) IS NULL))
Rows Removed by Filter: 3
Buffers: shared hit=75873
-> Index Scan using geo_measr_sample_2022_04_act_id_tms_idx on
geo_ants.geo_measr_sample_2022_04 e_3 (cost=0.43..1975.08 rows=26246
width=1557) (actual time=0.005..2.232 rows=258 loops=315)
Output: e_3.tms, e_3.measure_list, e_3.act_id
Index Cond: ((e_3.act_id = fh.act_id) AND (e_3.tms >=
'2022-02-28 00:00:00'::timestamp without time zone) AND (e_3.tms <=
'2022-04-25 00:00:00'::timestamp without time zone))
Filter: (((e_3.measure_list #>> '{act_edit,s}'::text[]) <>
'excld'::text) OR ((e_3.measure_list #>> '{act_edit,s}'::text[]) IS NULL))
Buffers: shared hit=29597
Query Identifier: -6803725219970975357
Planning:
Buffers: shared hit=933
Planning Time: 2.057 ms
Execution Time: 33677.292 ms
*CREATE* *OR* *REPLACE* *FUNCTION*
geo_ants.antsgeo_get_severity_thr(v_measure_value *double* *precision*,
thr_value_1 *double* *precision*, thr_value_2 *double* *precision*,
thr_value_3 *double* *precision*, thr_value_4 *double* *precision*,
thr_value_5 *double* *precision*)
*RETURNS* *text*
*LANGUAGE* *sql*
*IMMUTABLE*
*AS* *$function$*
----------------------------------------------------------------------------------------------------------------------
-- Author: Federico Travaglini
-- Date: 2020
-- Description:
-- Change Hist: please mark changes in code as "yyyy-mm-dd, Author, change
request id in Merant, brief description"
----------------------------------------------------------------------------------------------------------------------
-- 20220426 non so perchè ma in questa versione non è efifciente
*select*
*case*
--WHEN v_measure_value= 'NaN' THEN '6 Unk'::text non
scommentare o le performance per qualche motivo iragionevole degradano di
molto.
*when* thr_value_1 = thr_value_4 *then* -- colorazione
disabilitata, ad esempio per lat, long...
'6 none'::*text*
*when* thr_value_1 > thr_value_4 *then* -- valori critical >
clear
-- SIAMO NEL CASO: valori critical > clear ( thr_5 clear
thr_4 warning thr_3 minor thr_2 major thr_1 critical)
*CASE*
*WHEN* v_measure_value >= thr_value_1 *THEN* '5
Critical'::*text* --critical
*WHEN* v_measure_value < thr_value_1 *AND*
v_measure_value >= thr_value_2 *THEN* '4 Major'::*text* --major
*WHEN* v_measure_value < thr_value_2 *AND*
v_measure_value >= thr_value_3 *THEN* '3 Minor'::*text* --minor
*WHEN* v_measure_value < thr_value_3 *AND*
v_measure_value >= thr_value_4 *THEN* '2 Warning'::*text* --warning
*WHEN* v_measure_value < thr_value_4 *THEN* '1 Clear'::
*text* --clear
*ELSE* '6 Unk'::*text* -- null values
*end*
*else*
-- SIAMO NEL CASO: valori critical < clear (critical thr_1
maj thr_2 minor thr_3 war thr_4 clear thr_5)
*CASE*
*WHEN* v_measure_value < thr_value_1 *THEN* '5 Critical'
::*text* --critical
*WHEN* v_measure_value >= thr_value_1 *AND*
v_measure_value < thr_value_2 *THEN* '4 Major'::*text* --major
*WHEN* v_measure_value >= thr_value_2 *AND*
v_measure_value < thr_value_3 *THEN* '3 Minor'::*text* --minor
*WHEN* v_measure_value >= thr_value_3 *AND*
v_measure_value < thr_value_4 *THEN* '2 Warning'::*text* --warning
*WHEN* v_measure_value >= thr_value_4 *THEN* '1 Clear'::
*text* --clear
*ELSE* '6 Unk'::*text* -- null values
*end*
*end*::*text*
*$function$*
;
By the way, if I call the overall function where it is this code fragment,
I get much better performance (22 sec in place of 41) re-writing function
case without nesting sub-cases, unfortunately I’m not so cleaver to get the
query plan for a query executed inside a function
*CREATE* *OR* *REPLACE* *FUNCTION*
geo_ants.antsgeo_get_severity_thr(v_measure_value *double* *precision*,
thr_value_1 *double* *precision*, thr_value_2 *double* *precision*,
thr_value_3 *double* *precision*, thr_value_4 *double* *precision*,
thr_value_5 *double* *precision*)
*RETURNS* *text*
*LANGUAGE* *sql*
*IMMUTABLE*
*AS* *$function$*
----------------------------------------------------------------------------------------------------------------------
-- Author: Federico Travaglini
-- Date: 2020
-- Description:
-- Change Hist: please mark changes in code as "yyyy-mm-dd, Author, change
request id in Merant, brief description"
----------------------------------------------------------------------------------------------------------------------
*select*
*case*
--WHEN v_measure_value= 'NaN' THEN '6 Unk'::text this must be
commented, it is not a problem because the semantic does not change (same
case of the ELSE), but I don’t understand why it changes performance.
*when* thr_value_1 = thr_value_4 *then* '6 Unk'::*text* --
colorazione disabilitata, ad esempio per lat, long...
-- SIAMO NEL CASO: valori critical > clear ( thr_5 clear thr_4
warning thr_3 minor thr_2 major thr_1 critical)
*WHEN* thr_value_1 > thr_value_4 *and* v_measure_value <
thr_value_4 *THEN* '1 Clear'::*text* --clear
*WHEN* thr_value_1 > thr_value_4 *and* v_measure_value <
thr_value_3 *THEN* '2 Warning'::*text* --warning
*WHEN* thr_value_1 > thr_value_4 *and* v_measure_value <
thr_value_2 *THEN* '3 Minor'::*text* --minor
*WHEN* thr_value_1 > thr_value_4 *and* v_measure_value <
thr_value_1 *THEN* '4 Major'::*text* --major
*WHEN* thr_value_1 > thr_value_4 *and* v_measure_value >=
thr_value_1 *THEN* '5 Critical'::*text* --major
-- SIAMO NEL CASO: valori critical < clear (critical thr_1 maj
thr_2 minor thr_3 war thr_4 clear thr_5)
*WHEN* thr_value_1 < thr_value_4 *and* v_measure_value >=
thr_value_4 *THEN* '1 Clear'::*text* --clear
*WHEN* thr_value_1 < thr_value_4 *and* v_measure_value >=
thr_value_3 *THEN* '2 Warning'::*text* --warning
*WHEN* thr_value_1 < thr_value_4 *and* v_measure_value >=
thr_value_2 *THEN* '3 Minor'::*text* --minor
*WHEN* thr_value_1 < thr_value_4 *and* v_measure_value >=
thr_value_1 *THEN* '4 Major'::*text* --major
*WHEN* thr_value_1 < thr_value_4 *and* v_measure_value <
thr_value_1 *THEN* '5 Critical'::*text* --critical
*ELSE* '6 Unk'::*text* -- null values
*end*::*text*
*$function$*
;
*Da:* Merlin Moncure <mmoncure@gmail.com>
*Inviato:* lunedì 25 aprile 2022 21:24
*A:* Federico Travaglini <federico.travaglini@aubay.it>
*Cc:* pgsql-bugs <pgsql-bugs@lists.postgresql.org>
*Oggetto:* Re: 14.1 immutable function, bad performance if check number =
'NaN'
On Mon, Apr 25, 2022 at 11:58 AM Federico Travaglini <
federico.travaglini@aubay.it> wrote:
Good evening, and thanks to your excellent Postgres.
This funcion in used as a column in a select on about 400k records
If I leave the highlighted row it takes 27 seconds, otherwise 14 seconds!
Such behaviour looks not to be reasonable.
lightly testing this, I got 10million iterations in about two seconds,
about the same after commenting the NaN test. Given that, problem is
probably failure to inline query. Careful examination of explain of
wrapping query should prove that.
merlin
--
This message is confidential and solely for the intended
address(es). If
you are not the intended recipient of this message, please
notify the sender
immediately and delete it from your system. Unauthorised
reproduction,
disclosure, modification and or distribution of this e-mail
is strictly
prohibited. The contents of this e-mail do not constitute a
commitment by Aubay
S.p.A., except where expressly provided for in a
written agreement between you
and Aubay.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: R: 14.1 immutable function, bad performance if check number = 'NaN'
2022-04-26 07:45 R: 14.1 immutable function, bad performance if check number = 'NaN' Federico Travaglini <federico.travaglini@collaboration.aubay.it>
@ 2022-04-26 14:11 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 4+ messages in thread
From: Tom Lane @ 2022-04-26 14:11 UTC (permalink / raw)
To: Federico Travaglini <federico.travaglini@collaboration.aubay.it>; +Cc: Merlin Moncure <mmoncure@gmail.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Federico Travaglini <federico.travaglini@collaboration.aubay.it> writes:
> Here it is what I tested. I’s a code fragment from a bigger procedure. The
> strings in green are passed as parameters, as well as the thresholds
> 1,2,3,4,5. To test just this fragment of code I replaced them with fixed
> values
Is that different from what you do normally?
In this example, the function clearly is getting inlined, which means that
the parameter values are potentially evaluated multiple times:
> antsgeo_get_severity_thr((e.measure_list #> ('{' ||
> 'cluster_comuni_italiani' || ',o}')::*text*[])::*numeric*, 1, 2, 3, 4, 5)
> *AS* severity_1,
expands to
> CASE WHEN
> ((((e.measure_list #>
> ('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
> precision >= '4'::double precision) THEN '1 Clear'::text WHEN
> ((((e.measure_list #>
> ('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
> precision >= '3'::double precision) THEN '2 Warning'::text WHEN
> ((((e.measure_list #>
> ('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
> precision >= '2'::double precision) THEN '3 Minor'::text WHEN
> ((((e.measure_list #>
> ('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
> precision >= '1'::double precision) THEN '4 Major'::text WHEN
> ((((e.measure_list #>
> ('{cluster_comuni_italiani,o}'::cstring)::text[]))::numeric)::double
> precision < '1'::double precision) THEN '5 Critical'::text ELSE '6
> Unk'::text END,
That seems pretty inefficient, becase #> isn't the fastest thing
in the world. Maybe the speed differential you're seeing is just
from adding one more evaluation of the #> for the NaN test.
So my advice is to fix things so that #> isn't evaluated multiple
times. There are ways to prevent the inlining from happening but
they're all underdocumented hacks. A more reliable fix would be to
convert the function to plpgsql language.
regards, tom lane
^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH] Have RepackDecodingState carry a magic number
@ 2026-06-01 16:43 Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 0 replies; 4+ messages in thread
From: Álvaro Herrera @ 2026-06-01 16:43 UTC (permalink / raw)
... which is set by the REPACK executing code and checked by the
logical decoding plugin; this way, if the plugin is invoked in some way
other than REPACK, we can throw an error.
Assorted code rearrangement is included.
Discussion: https://postgr.es/m/19500-38a02529a69353a5@postgresql.org
---
src/backend/commands/repack_worker.c | 70 +++++++++------------
src/backend/replication/pgrepack/pgrepack.c | 29 ++++++++-
src/include/commands/repack_internal.h | 3 +
3 files changed, 60 insertions(+), 42 deletions(-)
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index b6b7b604b4f..f3d2720b246 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -197,7 +197,7 @@ repack_setup_logical_decoding(Oid relid)
Relation rel;
Oid toastrelid;
LogicalDecodingContext *ctx;
- NameData slotname;
+ char slotname[NAMEDATALEN];
RepackDecodingState *dstate;
MemoryContext oldcxt;
@@ -218,12 +218,30 @@ repack_setup_logical_decoding(Oid relid)
*
* RS_TEMPORARY so that the slot gets cleaned up on ERROR.
*/
- snprintf(NameStr(slotname), NAMEDATALEN, "repack_%d", MyProcPid);
- ReplicationSlotCreate(NameStr(slotname), true, RS_TEMPORARY, false, true,
+ snprintf(slotname, NAMEDATALEN, "repack_%d", MyProcPid);
+ ReplicationSlotCreate(slotname, true, RS_TEMPORARY, false, true,
false, false);
EnsureLogicalDecodingEnabled();
+ /*
+ * Setup repacked_rel_locator and repacked_rel_toast_locator, which we use
+ * to skip decoding of unrelated relations.
+ */
+ rel = table_open(relid, AccessShareLock);
+ repacked_rel_locator = rel->rd_locator;
+ toastrelid = rel->rd_rel->reltoastrelid;
+ if (OidIsValid(toastrelid))
+ {
+ Relation toastrel;
+
+ /* Avoid logical decoding of other TOAST relations. */
+ toastrel = table_open(toastrelid, AccessShareLock);
+ repacked_rel_toast_locator = toastrel->rd_locator;
+ table_close(toastrel, AccessShareLock);
+ }
+ table_close(rel, AccessShareLock);
+
/*
* Neither prepare_write nor do_write callback nor update_progress is
* useful for us.
@@ -237,27 +255,16 @@ repack_setup_logical_decoding(Oid relid)
.segment_open = wal_segment_open,
.segment_close = wal_segment_close),
NULL, NULL, NULL);
+ /* The pgrepack output plugin allocated this; now we can complete it */
+ dstate = ctx->output_writer_private;
+ dstate->magic = REPACK_DECODER_MAGIC;
+#ifdef USE_ASSERT_CHECKING
+ dstate->relid = relid;
+#endif
- /*
- * We don't have control on setting fast_forward, so at least check it.
- */
+ /* We don't have control on setting fast_forward, so at least check it. */
Assert(!ctx->fast_forward);
- /* Avoid logical decoding of other relations. */
- rel = table_open(relid, AccessShareLock);
- repacked_rel_locator = rel->rd_locator;
- toastrelid = rel->rd_rel->reltoastrelid;
- if (OidIsValid(toastrelid))
- {
- Relation toastrel;
-
- /* Avoid logical decoding of other TOAST relations. */
- toastrel = table_open(toastrelid, AccessShareLock);
- repacked_rel_toast_locator = toastrel->rd_locator;
- table_close(toastrel, AccessShareLock);
- }
- table_close(rel, AccessShareLock);
-
DecodingContextFindStartpoint(ctx);
/*
@@ -283,28 +290,9 @@ repack_setup_logical_decoding(Oid relid)
* WAL.
*/
ctx->reader->private_data = palloc0_object(ReadLocalXLogPageNoWaitPrivate);
- dstate = palloc0_object(RepackDecodingState);
+
MemoryContextSwitchTo(oldcxt);
-#ifdef USE_ASSERT_CHECKING
- dstate->relid = relid;
-#endif
-
- dstate->change_cxt = AllocSetContextCreate(ctx->context,
- "REPACK - change",
- ALLOCSET_DEFAULT_SIZES);
-
- /* The file will be set as soon as we have it opened. */
- dstate->file = NULL;
-
- /*
- * Memory context and resource owner for long-lived resources.
- */
- dstate->worker_cxt = CurrentMemoryContext;
- dstate->worker_resowner = CurrentResourceOwner;
-
- ctx->output_writer_private = dstate;
-
return ctx;
}
diff --git a/src/backend/replication/pgrepack/pgrepack.c b/src/backend/replication/pgrepack/pgrepack.c
index a2615ce54c1..c9106882f8e 100644
--- a/src/backend/replication/pgrepack/pgrepack.c
+++ b/src/backend/replication/pgrepack/pgrepack.c
@@ -47,7 +47,25 @@ static void
repack_startup(LogicalDecodingContext *ctx, OutputPluginOptions *opt,
bool is_init)
{
- ctx->output_plugin_private = NULL;
+ RepackDecodingState *dstate;
+
+ /*
+ * Set up private state, but only partially: we want
+ * repack_setup_logical_decoding to bless it to prevent this plugin from
+ * being used outside of REPACK.
+ */
+ Assert(CurrentMemoryContext == ctx->context);
+ dstate = palloc0_object(RepackDecodingState);
+ dstate->change_cxt = AllocSetContextCreate(ctx->context,
+ "REPACK - change",
+ ALLOCSET_DEFAULT_SIZES);
+ /* The file will be set as soon as we have it opened. */
+ dstate->file = NULL;
+ /* Memory context and resource owner for long-lived resources. */
+ dstate->worker_cxt = CurrentMemoryContext;
+ dstate->worker_resowner = CurrentResourceOwner;
+
+ ctx->output_writer_private = dstate;
/* Probably unnecessary, as we don't use the SQL interface ... */
opt->output_type = OUTPUT_PLUGIN_BINARY_OUTPUT;
@@ -77,6 +95,15 @@ repack_shutdown(LogicalDecodingContext *ctx)
static void
repack_begin_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
{
+ RepackDecodingState *private =
+ (RepackDecodingState *) ctx->output_writer_private;
+
+ if (private->magic != REPACK_DECODER_MAGIC)
+ ereport(ERROR,
+ errmsg("wrong magic 0x%x in \"%s\" decoder plugin, expected 0x%x",
+ private->magic, "pgrepack", REPACK_DECODER_MAGIC),
+ errhint("The \"%s\" decoder plugin may only be called by %s.",
+ "pgrepack", "REPACK (CONCURRENTLY)"));
}
/* COMMIT callback */
diff --git a/src/include/commands/repack_internal.h b/src/include/commands/repack_internal.h
index 6a85cee8910..4236f0b6fde 100644
--- a/src/include/commands/repack_internal.h
+++ b/src/include/commands/repack_internal.h
@@ -37,8 +37,11 @@ typedef char ConcurrentChangeKind;
* The output plugin uses it to store the data changes that it decodes from
* WAL while the table contents is being copied to a new storage.
*/
+#define REPACK_DECODER_MAGIC 0x75A91DF7
typedef struct RepackDecodingState
{
+ uint32 magic;
+
#ifdef USE_ASSERT_CHECKING
/* The relation whose changes we're decoding. */
Oid relid;
--
2.47.3
--g2p5afb23t36fp4l--
^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH] Have RepackDecodingState carry a magic number
@ 2026-06-01 16:43 Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 0 replies; 4+ messages in thread
From: Álvaro Herrera @ 2026-06-01 16:43 UTC (permalink / raw)
... which is set by the REPACK executing code and checked by the
logical decoding plugin; this way, if the plugin is invoked in some way
other than REPACK, we can throw an error.
Assorted code rearrangement is included.
Discussion: https://postgr.es/m/19500-38a02529a69353a5@postgresql.org
---
src/backend/commands/repack_worker.c | 70 +++++++++------------
src/backend/replication/pgrepack/pgrepack.c | 29 ++++++++-
src/include/commands/repack_internal.h | 3 +
3 files changed, 60 insertions(+), 42 deletions(-)
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index b6b7b604b4f..f3d2720b246 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -197,7 +197,7 @@ repack_setup_logical_decoding(Oid relid)
Relation rel;
Oid toastrelid;
LogicalDecodingContext *ctx;
- NameData slotname;
+ char slotname[NAMEDATALEN];
RepackDecodingState *dstate;
MemoryContext oldcxt;
@@ -218,12 +218,30 @@ repack_setup_logical_decoding(Oid relid)
*
* RS_TEMPORARY so that the slot gets cleaned up on ERROR.
*/
- snprintf(NameStr(slotname), NAMEDATALEN, "repack_%d", MyProcPid);
- ReplicationSlotCreate(NameStr(slotname), true, RS_TEMPORARY, false, true,
+ snprintf(slotname, NAMEDATALEN, "repack_%d", MyProcPid);
+ ReplicationSlotCreate(slotname, true, RS_TEMPORARY, false, true,
false, false);
EnsureLogicalDecodingEnabled();
+ /*
+ * Setup repacked_rel_locator and repacked_rel_toast_locator, which we use
+ * to skip decoding of unrelated relations.
+ */
+ rel = table_open(relid, AccessShareLock);
+ repacked_rel_locator = rel->rd_locator;
+ toastrelid = rel->rd_rel->reltoastrelid;
+ if (OidIsValid(toastrelid))
+ {
+ Relation toastrel;
+
+ /* Avoid logical decoding of other TOAST relations. */
+ toastrel = table_open(toastrelid, AccessShareLock);
+ repacked_rel_toast_locator = toastrel->rd_locator;
+ table_close(toastrel, AccessShareLock);
+ }
+ table_close(rel, AccessShareLock);
+
/*
* Neither prepare_write nor do_write callback nor update_progress is
* useful for us.
@@ -237,27 +255,16 @@ repack_setup_logical_decoding(Oid relid)
.segment_open = wal_segment_open,
.segment_close = wal_segment_close),
NULL, NULL, NULL);
+ /* The pgrepack output plugin allocated this; now we can complete it */
+ dstate = ctx->output_writer_private;
+ dstate->magic = REPACK_DECODER_MAGIC;
+#ifdef USE_ASSERT_CHECKING
+ dstate->relid = relid;
+#endif
- /*
- * We don't have control on setting fast_forward, so at least check it.
- */
+ /* We don't have control on setting fast_forward, so at least check it. */
Assert(!ctx->fast_forward);
- /* Avoid logical decoding of other relations. */
- rel = table_open(relid, AccessShareLock);
- repacked_rel_locator = rel->rd_locator;
- toastrelid = rel->rd_rel->reltoastrelid;
- if (OidIsValid(toastrelid))
- {
- Relation toastrel;
-
- /* Avoid logical decoding of other TOAST relations. */
- toastrel = table_open(toastrelid, AccessShareLock);
- repacked_rel_toast_locator = toastrel->rd_locator;
- table_close(toastrel, AccessShareLock);
- }
- table_close(rel, AccessShareLock);
-
DecodingContextFindStartpoint(ctx);
/*
@@ -283,28 +290,9 @@ repack_setup_logical_decoding(Oid relid)
* WAL.
*/
ctx->reader->private_data = palloc0_object(ReadLocalXLogPageNoWaitPrivate);
- dstate = palloc0_object(RepackDecodingState);
+
MemoryContextSwitchTo(oldcxt);
-#ifdef USE_ASSERT_CHECKING
- dstate->relid = relid;
-#endif
-
- dstate->change_cxt = AllocSetContextCreate(ctx->context,
- "REPACK - change",
- ALLOCSET_DEFAULT_SIZES);
-
- /* The file will be set as soon as we have it opened. */
- dstate->file = NULL;
-
- /*
- * Memory context and resource owner for long-lived resources.
- */
- dstate->worker_cxt = CurrentMemoryContext;
- dstate->worker_resowner = CurrentResourceOwner;
-
- ctx->output_writer_private = dstate;
-
return ctx;
}
diff --git a/src/backend/replication/pgrepack/pgrepack.c b/src/backend/replication/pgrepack/pgrepack.c
index a2615ce54c1..c9106882f8e 100644
--- a/src/backend/replication/pgrepack/pgrepack.c
+++ b/src/backend/replication/pgrepack/pgrepack.c
@@ -47,7 +47,25 @@ static void
repack_startup(LogicalDecodingContext *ctx, OutputPluginOptions *opt,
bool is_init)
{
- ctx->output_plugin_private = NULL;
+ RepackDecodingState *dstate;
+
+ /*
+ * Set up private state, but only partially: we want
+ * repack_setup_logical_decoding to bless it to prevent this plugin from
+ * being used outside of REPACK.
+ */
+ Assert(CurrentMemoryContext == ctx->context);
+ dstate = palloc0_object(RepackDecodingState);
+ dstate->change_cxt = AllocSetContextCreate(ctx->context,
+ "REPACK - change",
+ ALLOCSET_DEFAULT_SIZES);
+ /* The file will be set as soon as we have it opened. */
+ dstate->file = NULL;
+ /* Memory context and resource owner for long-lived resources. */
+ dstate->worker_cxt = CurrentMemoryContext;
+ dstate->worker_resowner = CurrentResourceOwner;
+
+ ctx->output_writer_private = dstate;
/* Probably unnecessary, as we don't use the SQL interface ... */
opt->output_type = OUTPUT_PLUGIN_BINARY_OUTPUT;
@@ -77,6 +95,15 @@ repack_shutdown(LogicalDecodingContext *ctx)
static void
repack_begin_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
{
+ RepackDecodingState *private =
+ (RepackDecodingState *) ctx->output_writer_private;
+
+ if (private->magic != REPACK_DECODER_MAGIC)
+ ereport(ERROR,
+ errmsg("wrong magic 0x%x in \"%s\" decoder plugin, expected 0x%x",
+ private->magic, "pgrepack", REPACK_DECODER_MAGIC),
+ errhint("The \"%s\" decoder plugin may only be called by %s.",
+ "pgrepack", "REPACK (CONCURRENTLY)"));
}
/* COMMIT callback */
diff --git a/src/include/commands/repack_internal.h b/src/include/commands/repack_internal.h
index 6a85cee8910..4236f0b6fde 100644
--- a/src/include/commands/repack_internal.h
+++ b/src/include/commands/repack_internal.h
@@ -37,8 +37,11 @@ typedef char ConcurrentChangeKind;
* The output plugin uses it to store the data changes that it decodes from
* WAL while the table contents is being copied to a new storage.
*/
+#define REPACK_DECODER_MAGIC 0x75A91DF7
typedef struct RepackDecodingState
{
+ uint32 magic;
+
#ifdef USE_ASSERT_CHECKING
/* The relation whose changes we're decoding. */
Oid relid;
--
2.47.3
--g2p5afb23t36fp4l--
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-06-01 16:43 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-26 07:45 R: 14.1 immutable function, bad performance if check number = 'NaN' Federico Travaglini <federico.travaglini@collaboration.aubay.it>
2022-04-26 14:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-06-01 16:43 [PATCH] Have RepackDecodingState carry a magic number Álvaro Herrera <alvherre@kurilemu.de>
2026-06-01 16:43 [PATCH] Have RepackDecodingState carry a magic number Álvaro Herrera <alvherre@kurilemu.de>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox