Re: [RFC] Removing "magic" oids - Mailing list pgsql-hackers

Hi,

On 2018-11-14 00:01:52 -0800, Andres Freund wrote:
> > 1) There's a few places dealing with system tables that don't deal with
> >    a hardcoded system table. Since there's no notion of "table has oid"
> >    and "which column is the oid column) anymore, we need some way to
> >    deal with that.  So far I've just extended existing objectaddress.c
> >    code to deal with that, but it's not that pretty.
> 
> I think that can reverted, if we take care of 2), now that I cleaned up
> some of the uglier hacks.  Right now the consequence of this is that
> it's possible to insert into system catalogs without specifying the oid
> only for catalogs with objectaddress.c support.

> > 2) We need to be able to manually insert into catalog tables. Both
> >    initdb and emergency surgery.  My current hack is doing so directly
> >    in nodeModifyTable.c but that's beyond ugly.  I think we should add
> >    an explicit DEFAULT clause to those columns with something like
> >    nextoid('tablename', 'name_of_index') or such.
> 
> It'd be nextoid('tablename', 'oid', 'name_of_index'), I think.  There's
> a bit of a sequencing issue about when to add those column defaults, so
> initdb can properly insert, but it doesn't sound too hard.

I had contemplated adding the above function as the default on system
columns - but I think that'd add significant complications to
relcache.c. As it's not actually a common occurance to manually insert
into catalog tables, my revised plan is to provide a pg_nextoid()
function, but *not* set it as the default.  The only case in postgres
where that matters is the manual insertions into pg_depend during
initdb, but that query can trivially be adapted.

Currently I'm not planning to document the use pg_nextoid(), it's not
something users really should do.  What I wonder about however is where
it should best live - I've put into catalog.c for now, but I'm not
entirely happy with that.

I've thus taken out the nodeModifyTable.c hack that assigned oids at
INSERT. Thus insertions without oid specified now raise a NOT NULL
violation (which can be fixed by using the pg_nextoid() function).

Comments?


> > 3) The quickest way to deal with the bootstrap code was to just assign
> >    all oids for oid carrying tables that don't yet have any assigned.
> >    That doesn't generally seem terrible, although it's certainly badly
> >    implemented right now.  That'd mean we'd have three ranges of oids
> >    probably, unless we somehow have the bootstrap code advance the
> >    in-database oid counter to a right state before we start with
> >    post-bootstrap work.  I like the idea of all bootstrap time oids
> >    being determined by genbki.pl (I think that'll allow to remove some
> >    code too).
> 
> I've not done anything about this so far.

I've now revised this slightly. genbki.pl now computes the maximum oid
explicitly assigned in .dat files, and assignes oids to all 'oid'
columns without a value.  It does so starting directly at the maximum
value.  I personally don't see need to have implicit .bki oids be in a
different range, and having them assigned more densely is good for some
things (e.g. the fmgr builtins table, even though we currently assign
all proc oids manually).

Differing opinions about this?


Attached is revised version of the patch.  I've tried to find all
references to oids and revise them accordingly. The docs changes
probably need a polish.

I quite like the diffstat:
b0e170f2c8c57b683c74e6cccfb7e46356dbacea (HEAD -> oidnix) Remove WITH OIDs support.
 333 files changed, 1949 insertions(+), 4030 deletions(-)


While clearly not ready yet, I don't think it's that far off.

Missing:
- docs polish
- pg_upgrade early error
- discussion of the pg_dump/restore behaviour when encountering tables
  or archives with oids. It currently warns.  If we want to keep it that
  way - which I think is reasonable - a bit more code can be excised.

Greetings,

Andres Freund