Use get_call_result_type() more widely
Hi,
A review comment in another thread [1]/messages/by-id/Y41De5NnF2sxmJPI@paquier.xyz by Michael Paquier about the
usage of get_call_result_type() instead of explicit building of
TupleDesc made me think about using it more widely. Actually, the
get_call_result_type() looks at the function definitions to figure the
column names and build the required TupleDesc, usage of which avoids
duplication of the column names between pg_proc.dat/function
definitions and source code. Also, it saves a good number of LOC ~415
[2]: 21 files changed, 97 insertions(+), 514 deletions(-)
~4MB, which means, the postgres binary becomes leaner by ~4MB [3]Source code is built with CFLAGS = -O3. PATCHED: text data bss dec hex filename 1043 0 0 1043 413 contrib/old_snapshot/time_mapping.o 7192 0 0 7192 1c18 contrib/pg_visibility/pg_visibility.o 7144 0 120 7264 1c60 src/backend/access/transam/commit_ts.o 19681 24 248 19953 4df1 src/backend/access/transam/multixact.o 20595 0 88 20683 50cb src/backend/access/transam/twophase.o 6162 0 24 6186 182a src/backend/access/transam/xlogfuncs.o 45540 2736 8 48284 bc9c src/backend/catalog/objectaddress.o 9943 0 0 9943 26d7 src/backend/catalog/pg_publication.o 18239 0 16 18255 474f src/backend/commands/sequence.o 6429 0 0 6429 191d src/backend/tsearch/wparser.o 47049 1840 52 48941 bf2d src/backend/utils/adt/acl.o 43066 168 784 44018 abf2 src/backend/utils/adt/datetime.o 6843 0 0 6843 1abb src/backend/utils/adt/genfile.o 6904 120 0 7024 1b70 src/backend/utils/adt/lockfuncs.o 10512 7008 0 17520 4470 src/backend/utils/adt/misc.o 1569 0 0 1569 621 src/backend/utils/adt/partitionfuncs.o 16266 0 0 16266 3f8a src/backend/utils/adt/pgstatfuncs.o 40985 0 0 40985 a019 src/backend/utils/adt/tsvector_op.o 8322 0 0 8322 2082 src/backend/utils/misc/guc_funcs.o 2109 0 0 2109 83d src/backend/utils/misc/pg_controldata.o 2354 0 0 2354 932 src/test/modules/test_predtest/test_predtest.o 9586047 226936 205536 10018519 98ded7 src/backend/postgres. I'm
attaching a patch for these changes.
While on this, I observed that BlessTupleDesc() is called in many
(~12) places right after get_call_result_type() which actually does
the job of BlessTupleDesc() before returning the TupleDesc. I think we
can get rid of BlessTupleDesc() after get_call_result_type(). I'm
attaching a patch for these changes too.
cirrus-ci members are happy with these patches, please see here
https://github.com/BRupireddy/postgres/tree/use_get_call_result_type()_more_widely_v1.
Thoughts?
[1]: /messages/by-id/Y41De5NnF2sxmJPI@paquier.xyz
[2]: 21 files changed, 97 insertions(+), 514 deletions(-)
[3]: Source code is built with CFLAGS = -O3. PATCHED: text data bss dec hex filename 1043 0 0 1043 413 contrib/old_snapshot/time_mapping.o 7192 0 0 7192 1c18 contrib/pg_visibility/pg_visibility.o 7144 0 120 7264 1c60 src/backend/access/transam/commit_ts.o 19681 24 248 19953 4df1 src/backend/access/transam/multixact.o 20595 0 88 20683 50cb src/backend/access/transam/twophase.o 6162 0 24 6186 182a src/backend/access/transam/xlogfuncs.o 45540 2736 8 48284 bc9c src/backend/catalog/objectaddress.o 9943 0 0 9943 26d7 src/backend/catalog/pg_publication.o 18239 0 16 18255 474f src/backend/commands/sequence.o 6429 0 0 6429 191d src/backend/tsearch/wparser.o 47049 1840 52 48941 bf2d src/backend/utils/adt/acl.o 43066 168 784 44018 abf2 src/backend/utils/adt/datetime.o 6843 0 0 6843 1abb src/backend/utils/adt/genfile.o 6904 120 0 7024 1b70 src/backend/utils/adt/lockfuncs.o 10512 7008 0 17520 4470 src/backend/utils/adt/misc.o 1569 0 0 1569 621 src/backend/utils/adt/partitionfuncs.o 16266 0 0 16266 3f8a src/backend/utils/adt/pgstatfuncs.o 40985 0 0 40985 a019 src/backend/utils/adt/tsvector_op.o 8322 0 0 8322 2082 src/backend/utils/misc/guc_funcs.o 2109 0 0 2109 83d src/backend/utils/misc/pg_controldata.o 2354 0 0 2354 932 src/test/modules/test_predtest/test_predtest.o 9586047 226936 205536 10018519 98ded7 src/backend/postgres
PATCHED:
text data bss dec hex filename
1043 0 0 1043 413 contrib/old_snapshot/time_mapping.o
7192 0 0 7192 1c18 contrib/pg_visibility/pg_visibility.o
7144 0 120 7264 1c60 src/backend/access/transam/commit_ts.o
19681 24 248 19953 4df1 src/backend/access/transam/multixact.o
20595 0 88 20683 50cb src/backend/access/transam/twophase.o
6162 0 24 6186 182a src/backend/access/transam/xlogfuncs.o
45540 2736 8 48284 bc9c src/backend/catalog/objectaddress.o
9943 0 0 9943 26d7 src/backend/catalog/pg_publication.o
18239 0 16 18255 474f src/backend/commands/sequence.o
6429 0 0 6429 191d src/backend/tsearch/wparser.o
47049 1840 52 48941 bf2d src/backend/utils/adt/acl.o
43066 168 784 44018 abf2 src/backend/utils/adt/datetime.o
6843 0 0 6843 1abb src/backend/utils/adt/genfile.o
6904 120 0 7024 1b70 src/backend/utils/adt/lockfuncs.o
10512 7008 0 17520 4470 src/backend/utils/adt/misc.o
1569 0 0 1569 621 src/backend/utils/adt/partitionfuncs.o
16266 0 0 16266 3f8a src/backend/utils/adt/pgstatfuncs.o
40985 0 0 40985 a019 src/backend/utils/adt/tsvector_op.o
8322 0 0 8322 2082 src/backend/utils/misc/guc_funcs.o
2109 0 0 2109 83d src/backend/utils/misc/pg_controldata.o
2354 0 0 2354 932
src/test/modules/test_predtest/test_predtest.o
9586047 226936 205536 10018519 98ded7 src/backend/postgres
HEAD:
text data bss dec hex filename
1019 0 0 1019 3fb contrib/old_snapshot/time_mapping.o
7159 0 0 7159 1bf7 contrib/pg_visibility/pg_visibility.o
6655 0 120 6775 1a77 src/backend/access/transam/commit_ts.o
19636 24 248 19908 4dc4 src/backend/access/transam/multixact.o
20663 0 88 20751 510f src/backend/access/transam/twophase.o
6206 0 24 6230 1856 src/backend/access/transam/xlogfuncs.o
45700 2736 8 48444 bd3c src/backend/catalog/objectaddress.o
9952 0 0 9952 26e0 src/backend/catalog/pg_publication.o
18487 0 16 18503 4847 src/backend/commands/sequence.o
6143 0 0 6143 17ff src/backend/tsearch/wparser.o
47123 1840 52 49015 bf77 src/backend/utils/adt/acl.o
43099 168 784 44051 ac13 src/backend/utils/adt/datetime.o
7016 0 0 7016 1b68 src/backend/utils/adt/genfile.o
7413 120 0 7533 1d6d src/backend/utils/adt/lockfuncs.o
10698 7008 0 17706 452a src/backend/utils/adt/misc.o
1593 0 0 1593 639 src/backend/utils/adt/partitionfuncs.o
17194 0 0 17194 432a src/backend/utils/adt/pgstatfuncs.o
40798 0 0 40798 9f5e src/backend/utils/adt/tsvector_op.o
8871 0 0 8871 22a7 src/backend/utils/misc/guc_funcs.o
3918 0 0 3918 f4e src/backend/utils/misc/pg_controldata.o
2636 0 0 2636 a4c
src/test/modules/test_predtest/test_predtest.o
9589943 226936 205536 10022415 98ee0f src/backend/postgres
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Use-get_call_result_type-more-widely.patchapplication/x-patch; name=v1-0001-Use-get_call_result_type-more-widely.patchDownload+97-515
v1-0002-Remove-unnecessary-BlessTupleDesc-after-get_call_.patchapplication/x-patch; name=v1-0002-Remove-unnecessary-BlessTupleDesc-after-get_call_.patchDownload+2-18
On Tue, Dec 13, 2022 at 01:06:48PM +0530, Bharath Rupireddy wrote:
A review comment in another thread [1] by Michael Paquier about the
usage of get_call_result_type() instead of explicit building of
TupleDesc made me think about using it more widely. Actually, the
get_call_result_type() looks at the function definitions to figure the
column names and build the required TupleDesc, usage of which avoids
duplication of the column names between pg_proc.dat/function
definitions and source code. Also, it saves a good number of LOC ~415
[2] and the size of all the object files put together gets reduced by
~4MB, which means, the postgres binary becomes leaner by ~4MB [3]. I'm
attaching a patch for these changes.
I have wanted to look at that when poking at the interface for
materialized SRFs but lacked of steam back then. Even after this
change, we still have coverage for CreateTemplateTupleDesc() and
TupleDescInitEntry() through the GUCs/SHOW or even WAL sender, so the
coverage does not worry me much. Backpatch conflicts may be a point
of contention, but that's pretty much in the same spirit as
SetSingleFuncCall()/InitMaterializedSRF().
All in that, +1 (still need to check in details what you have here,
looks rather fine at quick glance).
--
Michael
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
A review comment in another thread [1] by Michael Paquier about the
usage of get_call_result_type() instead of explicit building of
TupleDesc made me think about using it more widely. Actually, the
get_call_result_type() looks at the function definitions to figure the
column names and build the required TupleDesc, usage of which avoids
duplication of the column names between pg_proc.dat/function
definitions and source code. Also, it saves a good number of LOC ~415
[2] and the size of all the object files put together gets reduced by
~4MB, which means, the postgres binary becomes leaner by ~4MB [3].
Saving code is nice, but I'd assume the result is slower, because
get_call_result_type has to do a pretty substantial amount of work
to get the data to construct the tupdesc from. Have you tried to
quantify how much overhead this'd add? Which of these functions
can we safely consider to be non-performance-critical?
regards, tom lane
On Tue, Dec 13, 2022 at 9:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
A review comment in another thread [1] by Michael Paquier about the
usage of get_call_result_type() instead of explicit building of
TupleDesc made me think about using it more widely. Actually, the
get_call_result_type() looks at the function definitions to figure the
column names and build the required TupleDesc, usage of which avoids
duplication of the column names between pg_proc.dat/function
definitions and source code. Also, it saves a good number of LOC ~415
[2] and the size of all the object files put together gets reduced by
~4MB, which means, the postgres binary becomes leaner by ~4MB [3].Saving code is nice, but I'd assume the result is slower, because
get_call_result_type has to do a pretty substantial amount of work
to get the data to construct the tupdesc from. Have you tried to
quantify how much overhead this'd add? Which of these functions
can we safely consider to be non-performance-critical?
AFAICS, most of these functions have no direct source code callers,
they're user-facing functions and not in a hot code path. I measured
the test times of these functions and I don't see much difference [1]pg_old_snapshot_time_mapping() - an extension function with no internal source code callers, no test coverage. pg_visibility_map_summary() - an extension function with no internal source code callers, test coverage exists, test times on HEAD:25 ms PATCHED:25 ms pg_last_committed_xact() and pg_xact_commit_timestamp_origin() - no internal source code callers, test coverage exists, test times on HEAD:10 ms PATCHED:10 ms pg_get_multixact_members() - no internal source code callers, no test coverage. pg_prepared_xact() - no internal source code callers, test coverage exists, test times on HEAD:50 ms, subscription 108 wallclock secs, recovery 111 wallclock secs PATCHED:48 ms, subscription 110 wallclock secs, recovery 112 wallclock secs pg_walfile_name_offset() - no internal source code callers, no test coverage. pg_get_object_address() - no internal source code callers, test coverage exists, test times on HEAD:66 ms PATCHED:60 ms pg_identify_object() - no internal source code callers, test coverage exists, test times on HEAD:17 ms PATCHED:18 ms pg_identify_object_as_address() - no internal source code callers, test coverage exists, test times on HEAD:66 ms PATCHED:60 ms pg_get_publication_tables() - internal source code callers exist, test coverage exists, test times on HEAD:159 ms, subscription 108 wallclock secs PATCHED:167 ms, subscription 110 wallclock secs pg_sequence_parameters() - no internal source code callers, test coverage exists, test times on HEAD:96 ms PATCHED:98 ms ts_token_type_byid(), ts_token_type_byname(), ts_parse_byid() and ts_parse_byname() - internal source code callers exists, test coverage exists, test times on HEAD:195 ms, pg_dump 10 wallclock secs PATCHED:186 ms, pg_dump 10 wallclock secs aclexplode() - internal callers exists information_schema.sql, indirect test coverage exists. pg_timezone_abbrevs() - no internal source code callers, test coverage exists, test times on HEAD:40 ms PATCHED:36 ms pg_stat_file() - no internal source code callers, test coverage exists, test times on HEAD:42 ms PATCHED:46 ms pg_lock_status() - no internal source code callers, test coverage exists, test times on HEAD:16 ms PATCHED:22 ms pg_get_keywords() - no internal source code callers, test coverage exists, test times on HEAD:129 ms PATCHED:130 ms pg_get_catalog_foreign_keys() - no internal source code callers, test coverage exists, test times on HEAD:114 ms PATCHED:111 ms pg_partition_tree() - no internal source code callers, test coverage exists, test times on HEAD:30 ms PATCHED:32 ms pg_stat_get_wal(), pg_stat_get_archiver() and pg_stat_get_replication_slot() - no internal source code callers, test coverage exists, test times on HEAD:479 ms PATCHED:483 ms pg_stat_get_subscription_stats() - no internal source code callers, test coverage exists, test times on HEAD:subscription 108 wallclock secs PATCHED:subscription 110 wallclock secs tsvector_unnest() - no internal source code callers, test coverage exists, test times on HEAD:26 ms PATCHED:26 ms ts_setup_firstcall() - test coverage exists, test times on HEAD:195 ms PATCHED:186 ms show_all_settings(), pg_control_system(), pg_control_checkpoint(), pg_control_recovery() and pg_control_init() - test coverage exists, test times on HEAD:42 ms PATCHED:44 ms test_predtest() - no internal source code callers, test coverage exists, test times on HEAD:18 ms PATCHED:18 ms.
[1]: pg_old_snapshot_time_mapping() - an extension function with no internal source code callers, no test coverage. pg_visibility_map_summary() - an extension function with no internal source code callers, test coverage exists, test times on HEAD:25 ms PATCHED:25 ms pg_last_committed_xact() and pg_xact_commit_timestamp_origin() - no internal source code callers, test coverage exists, test times on HEAD:10 ms PATCHED:10 ms pg_get_multixact_members() - no internal source code callers, no test coverage. pg_prepared_xact() - no internal source code callers, test coverage exists, test times on HEAD:50 ms, subscription 108 wallclock secs, recovery 111 wallclock secs PATCHED:48 ms, subscription 110 wallclock secs, recovery 112 wallclock secs pg_walfile_name_offset() - no internal source code callers, no test coverage. pg_get_object_address() - no internal source code callers, test coverage exists, test times on HEAD:66 ms PATCHED:60 ms pg_identify_object() - no internal source code callers, test coverage exists, test times on HEAD:17 ms PATCHED:18 ms pg_identify_object_as_address() - no internal source code callers, test coverage exists, test times on HEAD:66 ms PATCHED:60 ms pg_get_publication_tables() - internal source code callers exist, test coverage exists, test times on HEAD:159 ms, subscription 108 wallclock secs PATCHED:167 ms, subscription 110 wallclock secs pg_sequence_parameters() - no internal source code callers, test coverage exists, test times on HEAD:96 ms PATCHED:98 ms ts_token_type_byid(), ts_token_type_byname(), ts_parse_byid() and ts_parse_byname() - internal source code callers exists, test coverage exists, test times on HEAD:195 ms, pg_dump 10 wallclock secs PATCHED:186 ms, pg_dump 10 wallclock secs aclexplode() - internal callers exists information_schema.sql, indirect test coverage exists. pg_timezone_abbrevs() - no internal source code callers, test coverage exists, test times on HEAD:40 ms PATCHED:36 ms pg_stat_file() - no internal source code callers, test coverage exists, test times on HEAD:42 ms PATCHED:46 ms pg_lock_status() - no internal source code callers, test coverage exists, test times on HEAD:16 ms PATCHED:22 ms pg_get_keywords() - no internal source code callers, test coverage exists, test times on HEAD:129 ms PATCHED:130 ms pg_get_catalog_foreign_keys() - no internal source code callers, test coverage exists, test times on HEAD:114 ms PATCHED:111 ms pg_partition_tree() - no internal source code callers, test coverage exists, test times on HEAD:30 ms PATCHED:32 ms pg_stat_get_wal(), pg_stat_get_archiver() and pg_stat_get_replication_slot() - no internal source code callers, test coverage exists, test times on HEAD:479 ms PATCHED:483 ms pg_stat_get_subscription_stats() - no internal source code callers, test coverage exists, test times on HEAD:subscription 108 wallclock secs PATCHED:subscription 110 wallclock secs tsvector_unnest() - no internal source code callers, test coverage exists, test times on HEAD:26 ms PATCHED:26 ms ts_setup_firstcall() - test coverage exists, test times on HEAD:195 ms PATCHED:186 ms show_all_settings(), pg_control_system(), pg_control_checkpoint(), pg_control_recovery() and pg_control_init() - test coverage exists, test times on HEAD:42 ms PATCHED:44 ms test_predtest() - no internal source code callers, test coverage exists, test times on HEAD:18 ms PATCHED:18 ms
pg_old_snapshot_time_mapping() - an extension function with no
internal source code callers, no test coverage.
pg_visibility_map_summary() - an extension function with no internal
source code callers, test coverage exists, test times on HEAD:25 ms
PATCHED:25 ms
pg_last_committed_xact() and pg_xact_commit_timestamp_origin() - no
internal source code callers, test coverage exists, test times on
HEAD:10 ms PATCHED:10 ms
pg_get_multixact_members() - no internal source code callers, no test coverage.
pg_prepared_xact() - no internal source code callers, test coverage
exists, test times on HEAD:50 ms, subscription 108 wallclock secs,
recovery 111 wallclock secs PATCHED:48 ms, subscription 110 wallclock
secs, recovery 112 wallclock secs
pg_walfile_name_offset() - no internal source code callers, no test coverage.
pg_get_object_address() - no internal source code callers, test
coverage exists, test times on HEAD:66 ms PATCHED:60 ms
pg_identify_object() - no internal source code callers, test coverage
exists, test times on HEAD:17 ms PATCHED:18 ms
pg_identify_object_as_address() - no internal source code callers,
test coverage exists, test times on HEAD:66 ms PATCHED:60 ms
pg_get_publication_tables() - internal source code callers exist, test
coverage exists, test times on HEAD:159 ms, subscription 108 wallclock
secs PATCHED:167 ms, subscription 110 wallclock secs
pg_sequence_parameters() - no internal source code callers, test
coverage exists, test times on HEAD:96 ms PATCHED:98 ms
ts_token_type_byid(), ts_token_type_byname(), ts_parse_byid() and
ts_parse_byname() - internal source code callers exists, test coverage
exists, test times on HEAD:195 ms, pg_dump 10 wallclock secs
PATCHED:186 ms, pg_dump 10 wallclock secs
aclexplode() - internal callers exists information_schema.sql,
indirect test coverage exists.
pg_timezone_abbrevs() - no internal source code callers, test coverage
exists, test times on HEAD:40 ms PATCHED:36 ms
pg_stat_file() - no internal source code callers, test coverage
exists, test times on HEAD:42 ms PATCHED:46 ms
pg_lock_status() - no internal source code callers, test coverage
exists, test times on HEAD:16 ms PATCHED:22 ms
pg_get_keywords() - no internal source code callers, test coverage
exists, test times on HEAD:129 ms PATCHED:130 ms
pg_get_catalog_foreign_keys() - no internal source code callers, test
coverage exists, test times on HEAD:114 ms PATCHED:111 ms
pg_partition_tree() - no internal source code callers, test coverage
exists, test times on HEAD:30 ms PATCHED:32 ms
pg_stat_get_wal(), pg_stat_get_archiver() and
pg_stat_get_replication_slot() - no internal source code callers, test
coverage exists, test times on HEAD:479 ms PATCHED:483 ms
pg_stat_get_subscription_stats() - no internal source code callers,
test coverage exists, test times on HEAD:subscription 108 wallclock
secs PATCHED:subscription 110 wallclock secs
tsvector_unnest() - no internal source code callers, test coverage
exists, test times on HEAD:26 ms PATCHED:26 ms
ts_setup_firstcall() - test coverage exists, test times on HEAD:195 ms
PATCHED:186 ms
show_all_settings(), pg_control_system(), pg_control_checkpoint(),
pg_control_recovery() and pg_control_init() - test coverage exists,
test times on HEAD:42 ms PATCHED:44 ms
test_predtest() - no internal source code callers, test coverage
exists, test times on HEAD:18 ms PATCHED:18 ms
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Wed, Dec 14, 2022 at 11:14:59AM +0530, Bharath Rupireddy wrote:
AFAICS, most of these functions have no direct source code callers,
they're user-facing functions and not in a hot code path. I measured
the test times of these functions and I don't see much difference [1].
Thanks for the summary. It looks like your tests involve single
runs. What is the difference in run-time when invoking this
repeatedly with a large generate_series() for example when few or no
tuples are returned? Do you see a difference in perf profile? Some
of the functions could have their time mostly eaten while looking at
the syscache on repeated calls, but you could see the actual work this
involves with a dummy function that returns a large number of
attributes on a single record in the worst case possible?
Separating things into two buckets..
[1]
pg_old_snapshot_time_mapping() - an extension function with no
internal source code callers, no test coverage.
pg_visibility_map_summary() - an extension function with no internal
source code callers, test coverage exists, test times on HEAD:25 ms
PATCHED:25 ms
pg_last_committed_xact() and pg_xact_commit_timestamp_origin() - no
internal source code callers, test coverage exists, test times on
HEAD:10 ms PATCHED:10 ms> pg_get_multixact_members() - no internal source code callers, no test coverage.
pg_control_recovery() and pg_control_init() - test coverage exists,
test times on HEAD:42 ms PATCHED:44 ms
pg_identify_object() - no internal source code callers, test coverage
exists, test times on HEAD:17 ms PATCHED:18 ms
pg_identify_object_as_address() - no internal source code callers,
test coverage exists, test times on HEAD:66 ms PATCHED:60 ms
pg_get_object_address() - no internal source code callers, test
coverage exists, test times on HEAD:66 ms PATCHED:60 ms
pg_sequence_parameters() - no internal source code callers, test
coverage exists, test times on HEAD:96 ms PATCHED:98 ms
ts_token_type_byid(), ts_token_type_byname(), ts_parse_byid() and
ts_parse_byname() - internal source code callers exists, test coverage
exists, test times on HEAD:195 ms, pg_dump 10 wallclock secs
PATCHED:186 ms, pg_dump 10 wallclock secs
pg_get_keywords() - no internal source code callers, test coverage
exists, test times on HEAD:129 ms PATCHED:130 ms
pg_get_catalog_foreign_keys() - no internal source code callers, test
coverage exists, test times on HEAD:114 ms PATCHED:111 ms
tsvector_unnest() - no internal source code callers, test coverage
exists, test times on HEAD:26 ms PATCHED:26 ms
ts_setup_firstcall() - test coverage exists, test times on HEAD:195 ms
PATCHED:186 ms
pg_partition_tree() - no internal source code callers, test coverage
exists, test times on HEAD:30 ms PATCHED:32 ms
pg_timezone_abbrevs() - no internal source code callers, test coverage
exists, test times on HEAD:40 ms PATCHED:36 ms
These ones don't worry me much, TBH.
pg_stat_get_wal(), pg_stat_get_archiver() and
pg_stat_get_replication_slot() - no internal source code callers, test
coverage exists, test times on HEAD:479 ms PATCHED:483 ms
pg_prepared_xact() - no internal source code callers, test coverage
exists, test times on HEAD:50 ms, subscription 108 wallclock secs,
recovery 111 wallclock secs PATCHED:48 ms, subscription 110 wallclock
secs, recovery 112 wallclock secs
show_all_settings(), pg_control_system(), pg_control_checkpoint(),
test_predtest() - no internal source code callers, test coverage
exists, test times on HEAD:18 ms PATCHED:18 ms
pg_walfile_name_offset() - no internal source code callers, no test coverage.
aclexplode() - internal callers exists information_schema.sql,
indirect test coverage exists.
pg_stat_file() - no internal source code callers, test coverage
exists, test times on HEAD:42 ms PATCHED:46 ms
pg_get_publication_tables() - internal source code callers exist, test
coverage exists, test times on HEAD:159 ms, subscription 108 wallclock
secs PATCHED:167 ms, subscription 110 wallclock secs
pg_lock_status() - no internal source code callers, test coverage
exists, test times on HEAD:16 ms PATCHED:22 ms
pg_stat_get_subscription_stats() - no internal source code callers,
test coverage exists, test times on HEAD:subscription 108 wallclock
secs PATCHED:subscription 110 wallclock secs
These ones could be involved in monitoring queries run on a periodic
basis.
--
Michael
On Thu, Dec 15, 2022 at 11:41 AM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Dec 14, 2022 at 11:14:59AM +0530, Bharath Rupireddy wrote:
AFAICS, most of these functions have no direct source code callers,
they're user-facing functions and not in a hot code path. I measured
the test times of these functions and I don't see much difference [1].Thanks for the summary. It looks like your tests involve single
runs. What is the difference in run-time when invoking this
repeatedly with a large generate_series() for example when few or no
tuples are returned? Do you see a difference in perf profile? Some
of the functions could have their time mostly eaten while looking at
the syscache on repeated calls, but you could see the actual work this
involves with a dummy function that returns a large number of
attributes on a single record in the worst case possible?
Thanks. Yes, using get_call_result_type() for a function that gets
called repeatedly does have some cost as the comment around
get_call_result_type() says - I found in my testing that
get_call_result_type() does seem to cost 45% increase in execution
times over quick iterations of a function returning a single row with
36 columns.
Separating things into two buckets..
[1]
pg_old_snapshot_time_mapping() - an extension function with no
internal source code callers, no test coverage.
pg_visibility_map_summary() - an extension function with no internal
source code callers, test coverage exists, test times on HEAD:25 ms
PATCHED:25 ms
pg_last_committed_xact() and pg_xact_commit_timestamp_origin() - no
internal source code callers, test coverage exists, test times on
HEAD:10 ms PATCHED:10 ms> pg_get_multixact_members() - no internal source code callers, no test coverage.
pg_control_recovery() and pg_control_init() - test coverage exists,
test times on HEAD:42 ms PATCHED:44 ms
pg_identify_object() - no internal source code callers, test coverage
exists, test times on HEAD:17 ms PATCHED:18 ms
pg_identify_object_as_address() - no internal source code callers,
test coverage exists, test times on HEAD:66 ms PATCHED:60 ms
pg_get_object_address() - no internal source code callers, test
coverage exists, test times on HEAD:66 ms PATCHED:60 ms
pg_sequence_parameters() - no internal source code callers, test
coverage exists, test times on HEAD:96 ms PATCHED:98 ms
ts_token_type_byid(), ts_token_type_byname(), ts_parse_byid() and
ts_parse_byname() - internal source code callers exists, test coverage
exists, test times on HEAD:195 ms, pg_dump 10 wallclock secs
PATCHED:186 ms, pg_dump 10 wallclock secs
pg_get_keywords() - no internal source code callers, test coverage
exists, test times on HEAD:129 ms PATCHED:130 ms
pg_get_catalog_foreign_keys() - no internal source code callers, test
coverage exists, test times on HEAD:114 ms PATCHED:111 ms
tsvector_unnest() - no internal source code callers, test coverage
exists, test times on HEAD:26 ms PATCHED:26 ms
ts_setup_firstcall() - test coverage exists, test times on HEAD:195 ms
PATCHED:186 ms
pg_partition_tree() - no internal source code callers, test coverage
exists, test times on HEAD:30 ms PATCHED:32 ms
pg_timezone_abbrevs() - no internal source code callers, test coverage
exists, test times on HEAD:40 ms PATCHED:36 msThese ones don't worry me much, TBH.
pg_stat_get_wal(), pg_stat_get_archiver() and
pg_stat_get_replication_slot() - no internal source code callers, test
coverage exists, test times on HEAD:479 ms PATCHED:483 ms
pg_prepared_xact() - no internal source code callers, test coverage
exists, test times on HEAD:50 ms, subscription 108 wallclock secs,
recovery 111 wallclock secs PATCHED:48 ms, subscription 110 wallclock
secs, recovery 112 wallclock secs
show_all_settings(), pg_control_system(), pg_control_checkpoint(),
test_predtest() - no internal source code callers, test coverage
exists, test times on HEAD:18 ms PATCHED:18 ms
pg_walfile_name_offset() - no internal source code callers, no test coverage.
aclexplode() - internal callers exists information_schema.sql,
indirect test coverage exists.
pg_stat_file() - no internal source code callers, test coverage
exists, test times on HEAD:42 ms PATCHED:46 ms
pg_get_publication_tables() - internal source code callers exist, test
coverage exists, test times on HEAD:159 ms, subscription 108 wallclock
secs PATCHED:167 ms, subscription 110 wallclock secs
pg_lock_status() - no internal source code callers, test coverage
exists, test times on HEAD:16 ms PATCHED:22 ms
pg_stat_get_subscription_stats() - no internal source code callers,
test coverage exists, test times on HEAD:subscription 108 wallclock
secs PATCHED:subscription 110 wallclock secsThese ones could be involved in monitoring queries run on a periodic
basis.
I agree with the bucketization. Please see the attached patches. 0001
- gets rid of explicit tuple desc creation using
get_call_result_type() for functions thought to be not-so-frequently
called. 0002 - gets rid of an unnecessary call to BlessTupleDesc()
after get_call_result_type().
Please find the attached patches.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Use-get_call_result_type-for-not-so-frequently-ca.patchapplication/x-patch; name=v2-0001-Use-get_call_result_type-for-not-so-frequently-ca.patchDownload+60-223
v2-0002-Remove-unnecessary-BlessTupleDesc-after-get_call_.patchapplication/x-patch; name=v2-0002-Remove-unnecessary-BlessTupleDesc-after-get_call_.patchDownload+3-19
On Tue, Dec 13, 2022 at 10:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Saving code is nice, but I'd assume the result is slower, because
get_call_result_type has to do a pretty substantial amount of work
to get the data to construct the tupdesc from. Have you tried to
quantify how much overhead this'd add? Which of these functions
can we safely consider to be non-performance-critical?
Here's a modest proposal: let's do nothing about this. There's no
evidence of a real problem here, so we're going to be trying to judge
the performance benefits against the code size savings without any
real data indicating that either one is an issue. I bet we could
convert all of these to one style or the other and it would make very
little real world difference, but deciding which ones to change and in
which direction will take up time and energy that could otherwise be
spent on more worthwhile projects, and could possibly complicate
back-patching, too.
Basically, I think this is nit-picking. Let's just accept that both
styles have some advantages and leave it up to patch authors to pick
one that they prefer.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 2022-Dec-19, Robert Haas wrote:
Here's a modest proposal: let's do nothing about this. There's no
evidence of a real problem here, so we're going to be trying to judge
the performance benefits against the code size savings without any
real data indicating that either one is an issue. I bet we could
convert all of these to one style or the other and it would make very
little real world difference, but deciding which ones to change and in
which direction will take up time and energy that could otherwise be
spent on more worthwhile projects, and could possibly complicate
back-patching, too.Basically, I think this is nit-picking. Let's just accept that both
styles have some advantages and leave it up to patch authors to pick
one that they prefer.
The code savings are substantial actually, so I think bloating things
for cases where performance is not an issue is not good. Some other
developer is sure to cargo-cult that stuff in the future, and that's not
great.
On the other hand, the measurements have shown that going through the
function is significantly slower. So I kinda like the judgement call
that Michael and Bharath have made: change to use the function when
performance is not an issue, and keep the verbose coding otherwise.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Mon, Dec 19, 2022 at 2:07 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On the other hand, the measurements have shown that going through the
function is significantly slower. So I kinda like the judgement call
that Michael and Bharath have made: change to use the function when
performance is not an issue, and keep the verbose coding otherwise.
Seems fairly arbitrary to me. The ones used for monitoring queries
aren't likely to be run often enough that it matters, but in theory
it's possible that they could be. Many of the ones supposedly not used
for monitoring queries could reasonably be so used, too. You can get
any answer you want by making arbitrary assumptions about which ones
are likely to be used frequently and how frequently they're likely to
be used, and I think different people evaluating the list
independently of each other and with no knowledge of each others work
would likely reach substantially different conclusions, ranging all
the way from "do them all this way" to "do them all the other way" and
various positions in the middle.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Dec 19, 2022 at 2:07 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On the other hand, the measurements have shown that going through the
function is significantly slower. So I kinda like the judgement call
that Michael and Bharath have made: change to use the function when
performance is not an issue, and keep the verbose coding otherwise.
Seems fairly arbitrary to me.
Agreed ... but the decisions embodied in the code-as-it-stands are
even more arbitrary, being no doubt mostly based on "which function
did you copy to start from" not on any thought about performance.
Now that somebody's made an effort to identify which places are
potentially performance-critical, I don't see why we wouldn't use
the fruits of their labor. Yes, somebody else might draw the line
differently, but drawing a line at all seems like a step forward
to me.
regards, tom lane
On Mon, Dec 19, 2022 at 4:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Now that somebody's made an effort to identify which places are
potentially performance-critical, I don't see why we wouldn't use
the fruits of their labor. Yes, somebody else might draw the line
differently, but drawing a line at all seems like a step forward
to me.
All right, well, I just work here. :-)
--
Robert Haas
EDB: http://www.enterprisedb.com
On Mon, Dec 19, 2022 at 05:50:03PM -0500, Robert Haas wrote:
All right, well, I just work here. :-)
Just to give some numbers. The original version of the patch doing
the full switch removed 500 lines of code. The second version that
switches the "non-critical" paths removes 200~ lines.
--
Michael
On Mon, Dec 19, 2022 at 07:41:27PM +0530, Bharath Rupireddy wrote:
I agree with the bucketization. Please see the attached patches. 0001
- gets rid of explicit tuple desc creation using
get_call_result_type() for functions thought to be not-so-frequently
called.
It looks like I am OK with the code paths updated here, which refer to
none of the "critical" function paths.
0002 - gets rid of an unnecessary call to BlessTupleDesc()
after get_call_result_type().
Hmm. I am not sure whether this is right, actually..
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Mon, Dec 19, 2022 at 07:41:27PM +0530, Bharath Rupireddy wrote:
0002 - gets rid of an unnecessary call to BlessTupleDesc()
after get_call_result_type().
Hmm. I am not sure whether this is right, actually..
Hmm ... at least one of the paths through internal_get_result_type
is intentionally blessing the result tupdesc:
if (tupdesc->tdtypeid == RECORDOID &&
tupdesc->tdtypmod < 0)
assign_record_type_typmod(tupdesc);
but it's not clear if they all do, and the comments certainly
aren't promising it.
I'd be in favor of making this a documented API promise,
but it isn't that right now.
regards, tom lane
On Tue, Dec 20, 2022 at 1:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael@paquier.xyz> writes:
On Mon, Dec 19, 2022 at 07:41:27PM +0530, Bharath Rupireddy wrote:
0002 - gets rid of an unnecessary call to BlessTupleDesc()
after get_call_result_type().Hmm. I am not sure whether this is right, actually..
Hmm ... at least one of the paths through internal_get_result_type
is intentionally blessing the result tupdesc:if (tupdesc->tdtypeid == RECORDOID &&
tupdesc->tdtypmod < 0)
assign_record_type_typmod(tupdesc);but it's not clear if they all do, and the comments certainly
aren't promising it.
It looks to be safe to get rid of BlessTupleDesc() after
get_call_result_type() for the functions that have OUT parameters and
return 'record' type. This is because, the
get_call_result_type()->internal_get_result_type()->build_function_result_tupdesc_t()
returns non-NULL tupdesc for such functions and all the functions that
0002 patch touches are having OUT parameters and their return type is
'record'. I've also verified with Assert(tupdesc->tdtypmod >= 0); -
https://github.com/BRupireddy/postgres/tree/test_for_tdypmod_init_v1.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
On Tue, Dec 20, 2022 at 1:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Hmm ... at least one of the paths through internal_get_result_type
is intentionally blessing the result tupdesc:
but it's not clear if they all do, and the comments certainly
aren't promising it.
It looks to be safe to get rid of BlessTupleDesc() after
get_call_result_type() for the functions that have OUT parameters and
return 'record' type.
I think it's an absolutely horrid idea for callers to depend on
such details of get_call_result_type's behavior --- especially
when there is no function documentation promising it.
If we want to do something here, the thing to do would be to
guarantee in get_call_result_type's API spec that any returned
tupledesc is blessed. However, that might make some other
cases slower, if they don't need that.
On the whole, I'm content to leave the BlessTupleDesc calls in
these callers. They are cheap enough if the tupdesc is already
blessed.
regards, tom lane
On Tue, Dec 20, 2022 at 11:12:09AM -0500, Tom Lane wrote:
On the whole, I'm content to leave the BlessTupleDesc calls in
these callers. They are cheap enough if the tupdesc is already
blessed.
Yeah, agreed.
I have applied v2-0001, after fixing one error in wparser.c where some
of the previous style was not removed, leading to unnecessary work and
the same TupleDesc being built twice for the two ts_token_type()'s
(input of OID or text).
--
Michael
On Wed, Dec 21, 2022 at 6:44 AM Michael Paquier <michael@paquier.xyz> wrote:
I have applied v2-0001.
Thanks for taking care of this.
By seeing the impact that get_call_result_type() can have for the
functions that are possibly called repeatedly, I couldn't resist
sharing a patch (attached herewith) that adds a note of caution and
another way to build TupleDesc in the documentation to help developers
out there. Thoughts?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com