Horton

Thread

Commits

  1. Switch some system functions to use get_call_result_type()

  1. Use get_call_result_type() more widely

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-12-13T07:36:48Z

    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

  2. Re: Use get_call_result_type() more widely

    Michael Paquier <michael@paquier.xyz> — 2022-12-13T08:13:21Z

    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

  3. Re: Use get_call_result_type() more widely

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-12-13T15:42:51Z

    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

  4. Re: Use get_call_result_type() more widely

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-12-14T05:44:59Z

    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

  5. Re: Use get_call_result_type() more widely

    Michael Paquier <michael@paquier.xyz> — 2022-12-15T06:11:45Z

    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

  6. Re: Use get_call_result_type() more widely

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-12-19T14:11:27Z

    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

  7. Re: Use get_call_result_type() more widely

    Robert Haas <robertmhaas@gmail.com> — 2022-12-19T18:43:32Z

    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

  8. Re: Use get_call_result_type() more widely

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-12-19T19:07:44Z

    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/

  9. Re: Use get_call_result_type() more widely

    Robert Haas <robertmhaas@gmail.com> — 2022-12-19T19:33:06Z

    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

  10. Re: Use get_call_result_type() more widely

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-12-19T21:21:16Z

    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

  11. Re: Use get_call_result_type() more widely

    Robert Haas <robertmhaas@gmail.com> — 2022-12-19T22:50:03Z

    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

  12. Re: Use get_call_result_type() more widely

    Michael Paquier <michael@paquier.xyz> — 2022-12-20T07:32:36Z

    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

  13. Re: Use get_call_result_type() more widely

    Michael Paquier <michael@paquier.xyz> — 2022-12-20T07:38:59Z

    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

  14. Re: Use get_call_result_type() more widely

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-12-20T08:11:39Z

    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

  15. Re: Use get_call_result_type() more widely

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-12-20T10:53:52Z

    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

  16. Re: Use get_call_result_type() more widely

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-12-20T16:12:09Z

    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

  17. Re: Use get_call_result_type() more widely

    Michael Paquier <michael@paquier.xyz> — 2022-12-21T01:13:58Z

    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

  18. Re: Use get_call_result_type() more widely

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-12-21T07:19:37Z

    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