Re: automatically assigning catalog toast oids

On 2018-12-11 15:08:02 -0800, Andres Freund wrote:
> Hi,
> 
> On 2018-12-09 18:43:14 -0500, Tom Lane wrote:
> > Andres Freund <andres@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.

I've pushed it like that, after blindly attempting to satisfy
Solution.pm...

- Andres