Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)

On Mon, Dec 13, 2021 at 9:40 AM Shruthi Gowda <gowdashru(at)gmail(dot)com> wrote:
> > I am reviewing another patch
> > "v5-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg" as well
> > and will provide the comments soon if any...

I spent much of today reviewing 0001. Here's an updated version, so
far only lightly tested. Please check whether I've broken anything.
Here are the changes:

- I adjusted the function header comment for heap_create. Your
proposed comment seemed like it was pretty detailed but not 100%
correct. It also made one of the lines kind of long because you didn't
wrap the text in the surrounding style. I decided to make it simpler
and shorter instead of longer still and 100% correct.

- I removed a one-line comment that said /* Override the toast
relfilenode */ because it preceded an error check, not a line of code
that would have done what the comment claimed.

- I removed a one-line comment that said /* Override the relfilenode
*/ because the following code would only sometimes override the
relfilenode. The code didn't seem complex enough to justify a a longer
and more accurate comment, so I just took it out.

- I changed a test for (relkind == RELKIND_RELATION || relkind ==
RELKIND_SEQUENCE || relkind == RELKIND_MATVIEW) to use
RELKIND_HAS_STORAGE(). It's true that not all of the storage types
that RELKIND_HAS_STORAGE() tests are possible here, but that's not a
reason to avoiding using the macro. If somebody adds a new relkind
with storage in the future, they might miss the need to manually
update this place, but they will not likely miss the need to update
RELKIND_HAS_STORAGE() since, if they did, their code probably wouldn't
work at all.

- I changed the way that you were passing create_storage down to
heap_create. I think I said before that you should EITHER fix it so
that we set create_storage = true only when the relation actually has
storage OR ELSE have heap_create() itself override the value to false
when there is no storage. You did both. There are times when it's
reasonable to ensure the same thing in multiple places, but this
doesn't seem to be one of them. So I took that out. I chose to retain
the code in heap_create() that overrides the value to false, added a
comment explaining that it does that, and then adjusted the callers to
ignore the storage type. I then added comments, and in one place an
assertion, to make it clearer what is happening.

- In pg_dump.c, I adjusted the comment that says "Not every relation
has storage." and the test that immediately follows, to ignore the
relfilenode when relkind says it's a partitioned table. Really,
partitioned tables should never have had relfilenodes, but as it turns
out, they used to have them.

Let me know your thoughts.

--
Robert Haas
EDB: http://www.enterprisedb.com