Thread: Use get_call_result_type() more widely
Hi, 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. 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] https://www.postgresql.org/message-id/Y41De5NnF2sxmJPI%40paquier.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 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
Attachment
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
Attachment
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]. [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 -- 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
Attachment
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 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. 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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