Re: Use get_call_result_type() more widely

On Thu, Dec 15, 2022 at 11:41 AM Michael Paquier <michael(at)paquier(dot)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