On 2017-09-28 19:06:27 -0400, Tom Lane wrote:
> Andres Freund <and...@anarazel.de> writes:
> > On 2017-09-28 18:52:28 -0400, Tom Lane wrote:
> >> Uh, what? Access to fmgr_nbuiltins shouldn't be part of any critical path
> >> anymore after this change.
>
> > Indeed. But the size of the the oid -> fmgr_builtins index array is
> > relevant now. We could of course just make that dependent on
> > FirstBootstrapObjectId, but that'd waste some memory.
>
> Not enough to notice, considering there are pg_proc OIDs up in the 8K
> range already. We blow 2KB of never-accessed space for far less good
> reason than this.
Done that way. It's a bit annoying, because we've to take care to
initialize the "unused" part of the array with a valid signalling it's
an unused mapping. Can't use 0 for that because fmgr_builtins[0] is a
valid entry. We could introduce a dummy element at that position, but
that doesn't really seem nice either. Therefore the first attached
commit moves find_defined_symbol from genbki to Catalog.pm, so we can
easily extract FirstBootstrapObjectId in Gen_fmgrtab.pl.
Greetings,
Andres Freund
>From 16e35356cead73291d676764072abfebc2efa79b Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Mon, 2 Oct 2017 14:14:42 -0700
Subject: [PATCH 1/2] Move genbki.pl's find_defined_symbol to Catalog.pm.
Will be used in Gen_fmgrtab.pl in a followup commit.
---
src/backend/catalog/Catalog.pm | 35 ++++++++++++++++++++++++++++++++++-
src/backend/catalog/genbki.pl | 34 ++++------------------------------
src/backend/utils/Makefile | 2 +-
3 files changed, 39 insertions(+), 32 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 7abfda3d3a..54f83533b6 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -19,7 +19,7 @@ use warnings;
require Exporter;
our @ISA = qw(Exporter);
our @EXPORT = ();
-our @EXPORT_OK = qw(Catalogs SplitDataLine RenameTempFile);
+our @EXPORT_OK = qw(Catalogs SplitDataLine RenameTempFile FindDefinedSymbol);
# Call this function with an array of names of header files to parse.
# Returns a nested data structure describing the data in the headers.
@@ -252,6 +252,39 @@ sub RenameTempFile
rename($temp_name, $final_name) || die "rename: $temp_name: $!";
}
+
+# Find a symbol defined in a particular header file and extract the value.
+#
+# The include path has to be passed as a reference to an array.
+sub FindDefinedSymbol
+{
+ my ($catalog_header, $include_path, $symbol) = @_;
+
+ for my $path (@$include_path)
+ {
+
+ # Make sure include path ends in a slash.
+ if (substr($path, -1) ne '/')
+ {
+ $path .= '/';
+ }
+ my $file = $path . $catalog_header;
+ next if !-f $file;
+ open(my $find_defined_symbol, '<', $file) || die "$file: $!";
+ while (<$find_defined_symbol>)
+ {
+ if (/^#define\s+\Q$symbol\E\s+(\S+)/)
+ {
+ return $1;
+ }
+ }
+ close $find_defined_symbol;
+ die "$file: no definition found for $symbol\n";
+ }
+ die "$catalog_header: not found in any include directory\n";
+}
+
+
# verify the number of fields in the passed-in DATA line
sub check_natts
{
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 2eebb061b7..256a9c9c93 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -87,9 +87,11 @@ open my $shdescr, '>', $shdescrfile . $tmpext
# NB: make sure that the files used here are known to be part of the .bki
# file's dependencies by src/backend/catalog/Makefile.
my $BOOTSTRAP_SUPERUSERID =
- find_defined_symbol('pg_authid.h', 'BOOTSTRAP_SUPERUSERID');
+ Catalog::FindDefinedSymbol('pg_authid.h', \@include_path,
+ 'BOOTSTRAP_SUPERUSERID');
my $PG_CATALOG_NAMESPACE =
- find_defined_symbol('pg_namespace.h', 'PG_CATALOG_NAMESPACE');
+ Catalog::FindDefinedSymbol('pg_namespace.h', \@include_path,
+ 'PG_CATALOG_NAMESPACE');
# Read all the input header files into internal data structures
my $catalogs = Catalog::Catalogs(@input_files);
@@ -500,34 +502,6 @@ sub emit_schemapg_row
return $row;
}
-# Find a symbol defined in a particular header file and extract the value.
-sub find_defined_symbol
-{
- my ($catalog_header, $symbol) = @_;
- for my $path (@include_path)
- {
-
- # Make sure include path ends in a slash.
- if (substr($path, -1) ne '/')
- {
- $path .= '/';
- }
- my $file = $path . $catalog_header;
- next if !-f $file;
- open(my $find_defined_symbol, '<', $file) || die "$file: $!";
- while (<$find_defined_symbol>)
- {
- if (/^#define\s+\Q$symbol\E\s+(\S+)/)
- {
- return $1;
- }
- }
- close $find_defined_symbol;
- die "$file: no definition found for $symbol\n";
- }
- die "$catalog_header: not found in any include directory\n";
-}
-
sub usage
{
die <<EOM;
diff --git a/src/backend/utils/Makefile b/src/backend/utils/Makefile
index 2e35ca58cc..efb8b53f49 100644
--- a/src/backend/utils/Makefile
+++ b/src/backend/utils/Makefile
@@ -25,7 +25,7 @@ fmgrprotos.h: fmgroids.h ;
fmgroids.h: fmgrtab.c ;
fmgrtab.c: Gen_fmgrtab.pl $(catalogdir)/Catalog.pm $(top_srcdir)/src/include/catalog/pg_proc.h
- $(PERL) -I $(catalogdir) $< $(top_srcdir)/src/include/catalog/pg_proc.h
+ $(PERL) -I $(catalogdir) $< -I $(top_srcdir)/src/include/ $(top_srcdir)/src/include/catalog/pg_proc.h
errcodes.h: $(top_srcdir)/src/backend/utils/errcodes.txt generate-errcodes.pl
$(PERL) $(srcdir)/generate-errcodes.pl $< > $@
--
2.14.1.536.g6867272d5b.dirty
>From 2dfe8a94a8bea160e9cac512f0d3346fdc90cf44 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Mon, 2 Oct 2017 14:27:05 -0700
Subject: [PATCH 2/2] Replace binary search in fmgr_isbuiltin with a lookup
array.
Turns out we have enough functions that the binary search is quite
noticeable in profiles.
Thus have Gen_fmgrtab.pl build a new mapping from a builtin function's
oid to an index in the existing fmgr_builtins array. That keeps the
additional memory usage at a reasonable amount.
Author: Andres Freund
Discussion: https://postgr.es/m/20170914065128.a5sk7z4xde5uy...@alap3.anarazel.de
---
src/backend/utils/Gen_fmgrtab.pl | 84 +++++++++++++++++++++++++++++++++-------
src/backend/utils/fmgr/fmgr.c | 27 ++++++-------
src/include/utils/fmgrtab.h | 8 ++++
3 files changed, 90 insertions(+), 29 deletions(-)
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index 17851fe2a4..b8821798e6 100644
--- a/src/backend/utils/Gen_fmgrtab.pl
+++ b/src/backend/utils/Gen_fmgrtab.pl
@@ -21,6 +21,8 @@ use warnings;
# Collect arguments
my $infile; # pg_proc.h
my $output_path = '';
+my @include_path;
+
while (@ARGV)
{
my $arg = shift @ARGV;
@@ -32,6 +34,10 @@ while (@ARGV)
{
$output_path = length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
}
+ elsif ($arg =~ /^-I/)
+ {
+ push @include_path, length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
+ }
else
{
usage();
@@ -44,6 +50,13 @@ if ($output_path ne '' && substr($output_path, -1) ne '/')
$output_path .= '/';
}
+# Sanity check arguments.
+die "No input files.\n" if !$infile;
+die "No include path; you must specify -I at least once.\n" if !@include_path;
+
+my $FirstBootstrapObjectId =
+ Catalog::FindDefinedSymbol('access/transam.h', \@include_path, 'FirstBootstrapObjectId');
+
# Read all the data from the include/catalog files.
my $catalogs = Catalog::Catalogs($infile);
@@ -176,6 +189,7 @@ qq|/*-------------------------------------------------------------------------
#include "postgres.h"
+#include "access/transam.h"
#include "utils/fmgrtab.h"
#include "utils/fmgrprotos.h"
@@ -191,32 +205,76 @@ foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr)
print $pfh "extern Datum $s->{prosrc}(PG_FUNCTION_ARGS);\n";
}
-# Create the fmgr_builtins table
+# Create the fmgr_builtins table, collect data for fmgr_builtin_oid_index
print $tfh "\nconst FmgrBuiltin fmgr_builtins[] = {\n";
my %bmap;
$bmap{'t'} = 'true';
$bmap{'f'} = 'false';
+my $fmgr_builtin_max_oid = -1;
+my @fmgr_builtin_oid_index;
+my $fmgr_count = 0;
foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr)
{
print $tfh
-" { $s->{oid}, \"$s->{prosrc}\", $s->{nargs}, $bmap{$s->{strict}}, $bmap{$s->{retset}}, $s->{prosrc} },\n";
+" { $s->{oid}, \"$s->{prosrc}\", $s->{nargs}, $bmap{$s->{strict}}, $bmap{$s->{retset}}, $s->{prosrc} }";
+
+ if ($s->{oid} > $fmgr_builtin_max_oid)
+ {
+ $fmgr_builtin_max_oid = $s->{oid};
+ }
+
+ $fmgr_builtin_oid_index[$s->{oid}] = $fmgr_count++;
+
+ if ($fmgr_count <= $#fmgr)
+ {
+ print $tfh ",\n";
+ }
+ else
+ {
+ print $tfh "\n";
+ }
}
+print $tfh "};";
+
+print $tfh qq|
+const int fmgr_nbuiltins = (sizeof(fmgr_builtins) / sizeof(FmgrBuiltin));
+
+const Oid fmgr_builtin_max_oid = $fmgr_builtin_max_oid;
+
+|;
+
+# Create fmgr_builtins_oid_index table.
+#
+# Note that the array has to be filled up to FirstBootstrapObjectId,
+# as we can't rely on zero initialization as 0 is a valid mapping.
+print $tfh "const uint16 fmgr_builtin_oid_index[FirstBootstrapObjectId] = {\n";
+for (my $i = 0; $i < $FirstBootstrapObjectId; $i++)
+{
+ my $oid = $fmgr_builtin_oid_index[$i];
+
+ # fmgr_builtin_oid_index is sparse, map nonexistant functions to
+ # InvalidOidBuiltinMapping
+ if (not defined $oid)
+ {
+ $oid = 'InvalidOidBuiltinMapping';
+ }
+
+ if ($i + 1 == $FirstBootstrapObjectId)
+ {
+ print $tfh " $oid\n";
+ }
+ else
+ {
+ print $tfh " $oid,\n";
+ }
+}
+print $tfh "};";
+
# And add the file footers.
print $ofh "\n#endif /* FMGROIDS_H */\n";
print $pfh "\n#endif /* FMGRPROTOS_H */\n";
-print $tfh
-qq| /* dummy entry is easier than getting rid of comma after last real one */
- /* (not that there has ever been anything wrong with *having* a
- comma after the last field in an array initializer) */
- { 0, NULL, 0, false, false, NULL }
-};
-
-/* Note fmgr_nbuiltins excludes the dummy entry */
-const int fmgr_nbuiltins = (sizeof(fmgr_builtins) / sizeof(FmgrBuiltin)) - 1;
-|;
-
close($ofh);
close($pfh);
close($tfh);
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 919733517b..971ea40bee 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -70,26 +70,21 @@ static Datum fmgr_security_definer(PG_FUNCTION_ARGS);
static const FmgrBuiltin *
fmgr_isbuiltin(Oid id)
{
- int low = 0;
- int high = fmgr_nbuiltins - 1;
+ int16 index;
+
+ /* fast lookup only possible if original oid still assigned */
+ if (id >= FirstBootstrapObjectId)
+ return NULL;
/*
- * Loop invariant: low is the first index that could contain target entry,
- * and high is the last index that could contain it.
+ * Lookup function data. If there's a miss in that range it's likely a
+ * nonexistant function, returning NULL here will trigger an ERROR later.
*/
- while (low <= high)
- {
- int i = (high + low) / 2;
- const FmgrBuiltin *ptr = &fmgr_builtins[i];
+ index = fmgr_builtin_oid_index[id];
+ if (index == InvalidOidBuiltinMapping)
+ return NULL;
- if (id == ptr->foid)
- return ptr;
- else if (id > ptr->foid)
- low = i + 1;
- else
- high = i - 1;
- }
- return NULL;
+ return &fmgr_builtins[index];
}
/*
diff --git a/src/include/utils/fmgrtab.h b/src/include/utils/fmgrtab.h
index 6130ef8f9c..dbd2c0b8e1 100644
--- a/src/include/utils/fmgrtab.h
+++ b/src/include/utils/fmgrtab.h
@@ -13,6 +13,7 @@
#ifndef FMGRTAB_H
#define FMGRTAB_H
+#include "access/transam.h"
#include "fmgr.h"
@@ -36,4 +37,11 @@ extern const FmgrBuiltin fmgr_builtins[];
extern const int fmgr_nbuiltins; /* number of entries in table */
+/*
+ * Mapping from a builtin function's oid to the index in the fmgr_builtins
+ * array.
+ */
+#define InvalidOidBuiltinMapping -1
+extern const uint16 fmgr_builtin_oid_index[FirstBootstrapObjectId];
+
#endif /* FMGRTAB_H */
--
2.14.1.536.g6867272d5b.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
