Re: [RFC] Removing "magic" oids

Hi All, Noah,

Noah, see one question to you below.

On 2018-09-29 20:48:10 -0700, Andres Freund wrote:
> The attached *PROTOTYPE* *WIP* patch removes WITH OIDs support, converts
> catalog table oids to explicit oid columns, and makes enough
> infrastructure changes to make plain pg_regress pass (after the
> necessary changes of course). Only superficial psql and no pg_dump
> changes.
>
> There's plenty of issues with the prototype, but overall I'm pretty
> pleased.

Here's an attached version of the prototype. Main changes besides lots
of conflict resolutions are that now all the regression tests pass. I've
also excised pg_dump support, and removed docs for WITH OIDs.

Questions:
- Currently the patch allows WITHOUT OIDS and WITH (oids = false), as
those are "harmless". Should we keep that?
- pg_dump will atm emit a warning if oids are contained in dumped table,
but not fail. Does that seem reasonable?
- pg_restore does the same, just when reading the archive
- pg_dump still puts the hasoids field into the archive. Thus the format
didn't change. That seems the most reasonable to me.
- one pgbench test tested concurrent insertions into a table with
oids, as some sort of stress test for lwlocks and spinlocks. I *think*
this doesn't really have to be a system oid column, and this was just
because that's how we triggered a bug on some machine. Noah, do I get
this right?

My reading from the discussion is that:
- we'll not have some magic compat thing in pg_dump that rewrites < 12 oids
into oid columns
- pg_upgrade will error out when it encounters an oid
- oids will remain visible

Does that seem like a fair read?

> 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.

> 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.

Greetings,

Andres Freund