Re: automatically assigning catalog toast oids

Hi,

On 2018-12-09 18:43:14 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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