Hi,
On 2018-12-09 18:43:14 -0500, Tom Lane wrote:
> Andres Freund <and...@anarazel.de> writes:
> > On 2018-12-09 17:14:42 -0500, Tom Lane wrote:
> >> Well, that's just a different very-easily-broken assumption. There are
> >> a lot of things that make auto-assigned OIDs unstable, and I do not think
> >> that we want to guarantee that they'll hold still across a release series.
>
> > Why wouldn't they be for genbki (rather than initdb) assigned oids? I
> > don't think it's reasonable to add new functions or such post release
> > that would have move oid assignments for other objects?
>
> As you've got this set up, we couldn't change *anything* for fear of
> it moving auto-assignments; there's no isolation between catalogs.
But there wasn't any previously either?
> Another thing I seriously dislike is that this allows people to omit OIDs
> from .dat entries in catalogs where we traditionally hand-assign OIDs.
That's not new, is it? Sure, now genbki.pl assigns the oid, but
previously it'd just have been heap_insert()? bootparse.y/bootstrap.c
never enforced that oids are assigned for tables that have oids.
> That's not a good idea because it would mean those entries don't have
> stable OIDs, whereas the whole point of hand assignment is to ensure
> all built-in objects of a particular type have stable OIDs. Now, you
> could argue about the usefulness of that policy for any given catalog;
> but if we decide that catalog X doesn't need stable OIDs then that should
> be an intentional policy change, not something that can happen because
> one lazy hacker didn't follow the policy.
I think we should change that policy, but I also think that there wasn't
any meaningful "assignment policy" change in what I did. So that just
seems like a separate argument.
Note that changing that for "prominent" catalogs would be a bit more
work than just changing the policy, as we'd need to assign oids before
the lookup tables are built - although the current behaviour would kind
of allow us to implement the "not crazy" policy of allowing
auto-assignment as long as the object isn't referenced; but via an imo
fairly opaque mechanism.
> > I'm fine with adding a distinct range, the earlier version of the patch
> > had that. I'd asked for comments if anybody felt a need to keep that,
> > nobody replied... I alternatively proposed that we could just start at
> > FirstNormalObjectId for those and update the server's oid start value to
> > the maximum genbki assigned oid. Do you have preferences around that?
>
> Yeah, I thought about the latter as well. But it adds complexity to the
> bootstrap process and makes it harder to tell what assigned a particular
> OID, so I'd rather go with the former, at least until the OID situation
> gets too tight to allow for daylight between the ranges.
Yea, it doesn't seem perfect, that's basically why I didn't go for it
last time.
> It looks to me like as of HEAD, genbki.pl is auto-assigning about 1470
> OIDs. Meanwhile, on my RHEL6 machine, initdb is auto-assigning about
> 1740 OIDs (what a coincidence); of those, 872 are collation entries
> that are absorbed from the system environment. So the second number is
> likely to vary a lot from platform to platform. (I don't have ICU
> enabled; I wonder how many that typically adds.)
>
> I'd be inclined to allow say 2000 OIDs for genbki.pl, with 4384 therefore
> available for initdb. We could expect to have to raise the boundary
> from time to time, but not very often.
I've attached a patch implementing that. I'm not particularly in love
with FirstGenbkiObjectId as the symbol, but I couldn't think of
something more descriptive.
I changed the length of fmgr_builtin_oid_index to FirstGenbkiObjectId -
until we allow pg_proc oids to be auto-assigned that'd just be wasted
memory otherwise?
I did *not* change record_plan_function_dependency(), it seems correct
that it doesn't track genbki assigned oids, they certainly can't change
while a server is running. But I'm not entirely clear to why that's not
using FirstNormalObjectId as the cutoff, so perhaps I'm missing
something. Similar with logic in predicate.c.
I did however change postgres_fdw's is_builtin(), as that says:
/*
* Return true if given object is one of PostgreSQL's built-in objects.
*
- * We use FirstBootstrapObjectId as the cutoff, so that we only consider
+ * We use FirstGenbkiObjectId as the cutoff, so that we only consider
* objects with hand-assigned OIDs to be "built in", not for instance any
* function or type defined in the information_schema.
*
@@ -154,7 +154,7 @@ lookup_shippable(Oid objectId, Oid classId,
PgFdwRelationInfo *fpinfo)
and >= FirstGenbkiObjectId would not be maniually assigned.
I added a throwaway "with 9000-9999 tentatively reserved for forks." to
transam.h, but I'm not sure we really want that, or whether that's good
wording.
Greetings,
Andres Freund
diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c
index 7f2ed0499c0..fb98faa5fd7 100644
--- a/contrib/postgres_fdw/shippable.c
+++ b/contrib/postgres_fdw/shippable.c
@@ -137,7 +137,7 @@ lookup_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo)
/*
* Return true if given object is one of PostgreSQL's built-in objects.
*
- * We use FirstBootstrapObjectId as the cutoff, so that we only consider
+ * We use FirstGenbkiObjectId as the cutoff, so that we only consider
* objects with hand-assigned OIDs to be "built in", not for instance any
* function or type defined in the information_schema.
*
@@ -154,7 +154,7 @@ lookup_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo)
bool
is_builtin(Oid objectId)
{
- return (objectId < FirstBootstrapObjectId);
+ return (objectId < FirstGenbkiObjectId);
}
/*
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index 0865240f11f..aaa3af7e5a1 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -88,7 +88,9 @@ generated-header-symlinks: $(top_builddir)/src/include/catalog/header-stamp
# instead is cheating a bit, but it will achieve the goal of updating the
# version number when it changes.
bki-stamp: genbki.pl Catalog.pm $(POSTGRES_BKI_SRCS) $(POSTGRES_BKI_DATA) $(top_srcdir)/configure.in
- $(PERL) -I $(catalogdir) $< --set-version=$(MAJORVERSION) $(POSTGRES_BKI_SRCS)
+ $(PERL) -I $(catalogdir) $< \
+ -I $(top_srcdir)/src/include/ --set-version=$(MAJORVERSION) \
+ $(POSTGRES_BKI_SRCS)
touch $@
# The generated headers must all be symlinked into builddir/src/include/,
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index edc8ea9f533..8e2a2480be6 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -22,6 +22,7 @@ use warnings;
my @input_files;
my $output_path = '';
my $major_version;
+my $include_path;
# Process command line switches.
while (@ARGV)
@@ -31,6 +32,10 @@ while (@ARGV)
{
push @input_files, $arg;
}
+ elsif ($arg =~ /^-I/)
+ {
+ $include_path = length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
+ }
elsif ($arg =~ /^-o/)
{
$output_path = length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
@@ -50,6 +55,7 @@ while (@ARGV)
# Sanity check arguments.
die "No input files.\n" if !@input_files;
die "--set-version must be specified.\n" if !defined $major_version;
+die "-I, the header include path, must be specified.\n" if !$include_path;
# Make sure output_path ends in a slash.
if ($output_path ne '' && substr($output_path, -1) ne '/')
@@ -133,17 +139,9 @@ foreach my $header (@input_files)
# While duplicate OIDs would only cause a failure if they appear in
# the same catalog, our project policy is that manually assigned OIDs
# should be globally unique, to avoid confusion.
-#
-# Also use the loop to determine the maximum explicitly assigned oid
-# found in the data file, we'll use that for default oid assignments.
my $found = 0;
-my $maxoid = 0;
foreach my $oid (keys %oidcounts)
{
- if ($oid > $maxoid)
- {
- $maxoid = $oid;
- }
next unless $oidcounts{$oid} > 1;
print STDERR "Duplicate OIDs detected:\n" if !$found;
print STDERR "$oid\n";
@@ -151,6 +149,15 @@ foreach my $oid (keys %oidcounts)
}
die "found $found duplicate OID(s) in catalog data\n" if $found;
+
+# Oids not specified in the input files are automatically assigned,
+# starting at FirstGenbkiObjectId.
+my $FirstGenbkiObjectId =
+ Catalog::FindDefinedSymbol('access/transam.h', $include_path,
+ 'FirstGenbkiObjectId');
+my $GenbkiNextOid = $FirstGenbkiObjectId;
+
+
# Fetch some special data that we will substitute into the output file.
# CAUTION: be wary about what symbols you substitute into the .bki file here!
# It's okay to substitute things that are expected to be really constant
@@ -418,8 +425,8 @@ EOM
# Assign oid if oid column exists and no explicit assignment in row
if ($attname eq "oid" and not defined $bki_values{$attname})
{
- $bki_values{$attname} = $maxoid;
- $maxoid++;
+ $bki_values{$attname} = $GenbkiNextOid;
+ $GenbkiNextOid++;
}
# Substitute constant values we acquired above.
@@ -858,6 +865,7 @@ sub usage
Usage: genbki.pl [options] header...
Options:
+ -I include path
-o output path
--set-version PostgreSQL version number for initdb cross-check
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index ca282913552..ed16737a6a1 100644
--- a/src/backend/utils/Gen_fmgrtab.pl
+++ b/src/backend/utils/Gen_fmgrtab.pl
@@ -79,9 +79,9 @@ foreach my $datfile (@input_files)
}
# Fetch some values for later.
-my $FirstBootstrapObjectId =
+my $FirstGenbkiObjectId =
Catalog::FindDefinedSymbol('access/transam.h', $include_path,
- 'FirstBootstrapObjectId');
+ 'FirstGenbkiObjectId');
my $INTERNALlanguageId =
Catalog::FindDefinedSymbolFromData($catalog_data{pg_language},
'INTERNALlanguageId');
@@ -252,13 +252,13 @@ const int fmgr_nbuiltins = (sizeof(fmgr_builtins) / sizeof(FmgrBuiltin));
# Create fmgr_builtins_oid_index table.
#
-# Note that the array has to be filled up to FirstBootstrapObjectId,
+# Note that the array has to be filled up to FirstGenbkiObjectId,
# as we can't rely on zero initialization as 0 is a valid mapping.
print $tfh qq|
-const uint16 fmgr_builtin_oid_index[FirstBootstrapObjectId] = {
+const uint16 fmgr_builtin_oid_index[FirstGenbkiObjectId] = {
|;
-for (my $i = 0; $i < $FirstBootstrapObjectId; $i++)
+for (my $i = 0; $i < $FirstGenbkiObjectId; $i++)
{
my $oid = $fmgr_builtin_oid_index[$i];
@@ -269,7 +269,7 @@ for (my $i = 0; $i < $FirstBootstrapObjectId; $i++)
$oid = 'InvalidOidBuiltinMapping';
}
- if ($i + 1 == $FirstBootstrapObjectId)
+ if ($i + 1 == $FirstGenbkiObjectId)
{
print $tfh " $oid\n";
}
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 73ff48c1963..231bf885989 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -75,7 +75,7 @@ fmgr_isbuiltin(Oid id)
uint16 index;
/* fast lookup only possible if original oid still assigned */
- if (id >= FirstBootstrapObjectId)
+ if (id >= FirstGenbkiObjectId)
return NULL;
/*
diff --git a/src/include/access/transam.h b/src/include/access/transam.h
index 83ec3f19797..f979a065f9f 100644
--- a/src/include/access/transam.h
+++ b/src/include/access/transam.h
@@ -71,18 +71,21 @@
/* ----------
* Object ID (OID) zero is InvalidOid.
*
- * OIDs 1-9999 are reserved for manual assignment (see the files
- * in src/include/catalog/).
+ * OIDs 1-9999 are reserved for manual assignment (see .dat files in
+ * src/include/catalog/), with 9000-9999 tentatively reserved for forks.
*
- * OIDS 10000-16383 are reserved for assignment during initdb
- * using the OID generator. (We start the generator at 10000.)
+ * OIDs 10000-12000 are reserved for assignment by genbki.pl, when the
+ * .dat files in src/include/catalog/ do not specify oids.
+ *
+ * OIDS 12000-16383 are reserved for assignment during initdb
+ * using the OID generator. (We start the generator at 12000.)
*
* OIDs beginning at 16384 are assigned from the OID generator
* during normal multiuser operation. (We force the generator up to
* 16384 as soon as we are in normal operation.)
*
- * The choices of 10000 and 16384 are completely arbitrary, and can be moved
- * if we run low on OIDs in either category. Changing the macros below
+ * The choices of 10000, 12000 and 16384 are completely arbitrary, and can be
+ * moved if we run low on OIDs in either category. Changing the macros below
* should be sufficient to do this.
*
* NOTE: if the OID generator wraps around, we skip over OIDs 0-16383
@@ -90,7 +93,8 @@
* reassigning OIDs that might have been assigned during initdb.
* ----------
*/
-#define FirstBootstrapObjectId 10000
+#define FirstGenbkiObjectId 10000
+#define FirstBootstrapObjectId 12000
#define FirstNormalObjectId 16384
/*
diff --git a/src/include/catalog/unused_oids b/src/include/catalog/unused_oids
index c5fc378ae34..b5cafe0f3d3 100755
--- a/src/include/catalog/unused_oids
+++ b/src/include/catalog/unused_oids
@@ -32,11 +32,11 @@ my @input_files = (glob("pg_*.h"), qw(indexing.h toasting.h));
my $oids = Catalog::FindAllOidsFromHeaders(@input_files);
-# Also push FirstBootstrapObjectId to serve as a terminator for the last gap.
-my $FirstBootstrapObjectId =
+# Also push FirstGenbkiObjectId to serve as a terminator for the last gap.
+my $FirstGenbkiObjectId =
Catalog::FindDefinedSymbol('access/transam.h', '..',
- 'FirstBootstrapObjectId');
-push @{$oids}, $FirstBootstrapObjectId;
+ 'FirstGenbkiObjectId');
+push @{$oids}, $FirstGenbkiObjectId;
my $prev_oid = 0;
foreach my $oid (sort { $a <=> $b } @{$oids})
diff --git a/src/include/utils/fmgrtab.h b/src/include/utils/fmgrtab.h
index 6122ed3e978..72671c38296 100644
--- a/src/include/utils/fmgrtab.h
+++ b/src/include/utils/fmgrtab.h
@@ -41,6 +41,6 @@ extern const int fmgr_nbuiltins; /* number of entries in table */
* array.
*/
#define InvalidOidBuiltinMapping PG_UINT16_MAX
-extern const uint16 fmgr_builtin_oid_index[FirstBootstrapObjectId];
+extern const uint16 fmgr_builtin_oid_index[FirstGenbkiObjectId];
#endif /* FMGRTAB_H */
