Thread: [HACKERS] generating fmgr prototypes automatically
During some recent patch reviews, there was some back and forth about where to put prototypes for fmgr-callable functions. We don't actually need these prototypes except to satisfy the compiler, so we end up sprinkling them around random header files. I figured we could generate these prototypes automatically using the existing Gen_fmgrtab.pl script. That ends up saving more than 2000 lines of manual code, and it helps detect when a function exists in code but is not registered in pg_proc.h. ... which this actually found in cash.c. I ended up adding the missing entries in pg_proc and pg_operator, but we could also opt to just remove the unused code. There are long-standing symbol clashes from the lo_* functions between the backend and libpq. So far this hasn't bothered anyone, but because this patch rearranges some header files, the libpqwalreceiver code gets upset. So I renamed the C symbols for the backends functions in a separate patch. The user-visible function names remain the same. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Hi
2016-12-31 6:46 GMT+01:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:
During some recent patch reviews, there was some back and forth about
where to put prototypes for fmgr-callable functions. We don't actually
need these prototypes except to satisfy the compiler, so we end up
sprinkling them around random header files.
I figured we could generate these prototypes automatically using the
existing Gen_fmgrtab.pl script. That ends up saving more than 2000
lines of manual code, and it helps detect when a function exists in code
but is not registered in pg_proc.h.
... which this actually found in cash.c. I ended up adding the missing
entries in pg_proc and pg_operator, but we could also opt to just remove
the unused code.
There are long-standing symbol clashes from the lo_* functions between
the backend and libpq. So far this hasn't bothered anyone, but because
this patch rearranges some header files, the libpqwalreceiver code gets
upset. So I renamed the C symbols for the backends functions in a
separate patch. The user-visible function names remain the same.
I am sending a review of these patches
1. there was not any problem with patching, compiling, all test passed
2. the doc and regress tests are not necessary ??
patch 0001 .. trivial cleaning
patch 0002 .. renaming lo_* to be_lo_* -- the prefix "be" is not what I expect - maybe "pg" instead. More because the be-fsstubs.h will be holds only lo_read, lo_write on end
patch 0003 .. trivial, but doesn't mean so we have not regress tests for these functions?
patch 0004 .. bigger part - I miss a comments when there are a exceptions:
extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS);
extern Datum nextval(PG_FUNCTION_ARGS);
extern Datum fmgr_sql(PG_FUNCTION_ARGS);
extern Datum aggregate_dummy(PG_FUNCTION_ARGS);
I found some obsolete definitions in c files
pgstatfuncs.c
/* bogus ... these externs should be in a header file */
extern Datum pg_stat_get_numscans(PG_FUNCTION_ARGS);
extern Datum pg_stat_get_tuples_returned(PG_FUNCTION_ARGS);
extern Datum pg_stat_get_tuples_fetched(PG_FUNCTION_ARGS);
extern Datum pg_stat_get_tuples_inserted(PG_FUNCTION_ARGS);
extern Datum pg_stat_get_tuples_updated(PG_FUNCTION_ARGS);
namespace.c
Datum><-->pg_table_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_type_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_function_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_operator_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_opclass_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_opfamily_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_collation_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_conversion_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_ts_parser_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_ts_dict_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_ts_template_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_ts_config_is_visible(PG_FUNCTION_ARGS);
Datum<-><-->pg_my_temp_schema(PG_FUNCTION_ARGS);
Datum<-><-->pg_is_other_temp_schema(PG_FUNCTION_ARGS);
Regards
Pavel
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/3/17 2:16 PM, Pavel Stehule wrote: > patch 0001 .. trivial cleaning > patch 0002 .. renaming lo_* to be_lo_* -- the prefix "be" is not what I > expect - maybe "pg" instead. More because the be-fsstubs.h will be > holds only lo_read, lo_write on end It's the naming that the OpenSSL functions use, e.g., be_tls_init. > patch 0003 .. trivial, but doesn't mean so we have not regress tests for > these functions? OK, added tests. > patch 0004 .. bigger part - I miss a comments when there are a exceptions: > > extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS); > extern Datum nextval(PG_FUNCTION_ARGS); > extern Datum fmgr_sql(PG_FUNCTION_ARGS); > extern Datum aggregate_dummy(PG_FUNCTION_ARGS); These functions and their special purpose are commented in the .c files, so I think that is covered OK. I added an additional comment to the numeric_float8_no_overflow(). > I found some obsolete definitions in c files OK, fixed those as well. I think I initially only looked in .h files. That just underlines how inconsistent this is, e.g., > pgstatfuncs.c > > /* bogus ... these externs should be in a header file */ > namespace.c > > /* These don't really need to appear in any header file */ New patch set attached. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
- v2-0001-Remove-unnecessary-prototypes-in-loadable-modules.patch
- v2-0002-Remove-unnecessary-include.patch
- v2-0003-Rename-C-symbols-for-backend-lo_-functions.patch
- v2-0004-Add-more-tests-for-money-type.patch
- v2-0005-Register-missing-money-operators-in-system-catalo.patch
- v2-0006-Generate-fmgr-prototypes-automatically.patch
2017-01-04 21:09 GMT+01:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:
On 1/3/17 2:16 PM, Pavel Stehule wrote:
> patch 0001 .. trivial cleaning
> patch 0002 .. renaming lo_* to be_lo_* -- the prefix "be" is not what I
> expect - maybe "pg" instead. More because the be-fsstubs.h will be
> holds only lo_read, lo_write on end
It's the naming that the OpenSSL functions use, e.g., be_tls_init.
lo_* functions are used by OpenSSL ?
> patch 0003 .. trivial, but doesn't mean so we have not regress tests for
> these functions?
OK, added tests.
> patch 0004 .. bigger part - I miss a comments when there are a exceptions:
>
> extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS);
> extern Datum nextval(PG_FUNCTION_ARGS);
> extern Datum fmgr_sql(PG_FUNCTION_ARGS);
> extern Datum aggregate_dummy(PG_FUNCTION_ARGS);
These functions and their special purpose are commented in the .c files,
so I think that is covered OK. I added an additional comment to the
numeric_float8_no_overflow().
> I found some obsolete definitions in c files
OK, fixed those as well. I think I initially only looked in .h files.
That just underlines how inconsistent this is, e.g.,
> pgstatfuncs.c
>
> /* bogus ... these externs should be in a header file */
> namespace.c
>
> /* These don't really need to appear in any header file */
New patch set attached.
Thank you
I'll check it tomorrow
Regards
Pavel
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/4/17 3:35 PM, Pavel Stehule wrote: > On 1/3/17 2:16 PM, Pavel Stehule wrote: > > patch 0001 .. trivial cleaning > > patch 0002 .. renaming lo_* to be_lo_* -- the prefix "be" is not what I > > expect - maybe "pg" instead. More because the be-fsstubs.h will be > > holds only lo_read, lo_write on end > > It's the naming that the OpenSSL functions use, e.g., be_tls_init. > > > lo_* functions are used by OpenSSL ? There are tls functions that exist in similar ways in the backend and in libpq, and the backend versions are called be_*. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
2017-01-04 22:17 GMT+01:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:
On 1/4/17 3:35 PM, Pavel Stehule wrote:
> On 1/3/17 2:16 PM, Pavel Stehule wrote:
> > patch 0001 .. trivial cleaning
> > patch 0002 .. renaming lo_* to be_lo_* -- the prefix "be" is not what I
> > expect - maybe "pg" instead. More because the be-fsstubs.h will be
> > holds only lo_read, lo_write on end
>
> It's the naming that the OpenSSL functions use, e.g., be_tls_init.
>
>
> lo_* functions are used by OpenSSL ?
There are tls functions that exist in similar ways in the backend and in
libpq, and the backend versions are called be_*.
tls functions are not available via V1 API. It is still strange to me.
Regards
Pavel
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi
2017-01-04 21:09 GMT+01:00 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:
On 1/3/17 2:16 PM, Pavel Stehule wrote:
> patch 0001 .. trivial cleaning
> patch 0002 .. renaming lo_* to be_lo_* -- the prefix "be" is not what I
> expect - maybe "pg" instead. More because the be-fsstubs.h will be
> holds only lo_read, lo_write on end
It's the naming that the OpenSSL functions use, e.g., be_tls_init.
> patch 0003 .. trivial, but doesn't mean so we have not regress tests for
> these functions?
OK, added tests.
> patch 0004 .. bigger part - I miss a comments when there are a exceptions:
>
> extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS);
> extern Datum nextval(PG_FUNCTION_ARGS);
> extern Datum fmgr_sql(PG_FUNCTION_ARGS);
> extern Datum aggregate_dummy(PG_FUNCTION_ARGS);
These functions and their special purpose are commented in the .c files,
so I think that is covered OK. I added an additional comment to the
numeric_float8_no_overflow().
> I found some obsolete definitions in c files
OK, fixed those as well. I think I initially only looked in .h files.
That just underlines how inconsistent this is, e.g.,
> pgstatfuncs.c
>
> /* bogus ... these externs should be in a header file */
> namespace.c
>
> /* These don't really need to appear in any header file */
New patch set attached.
I checked last set of patches and I didn't find any issue.
There are no problems with patching, compilation and all regress tests passed.
I'll mark this patch as ready for commiter
Regards
Pavel
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/5/17 12:24 PM, Pavel Stehule wrote: > I checked last set of patches and I didn't find any issue. > > There are no problems with patching, compilation and all regress tests > passed. > > I'll mark this patch as ready for commiter This has been committed. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services