Horton

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

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Shruthi Gowda <gowdashru@gmail.com>
Cc: Stephen Frost <sfrost@snowman.net>, Bruce Momjian <bruce@momjian.us>, Andres Freund <andres@anarazel.de>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Masahiko Sawada <sawada.mshk@gmail.com>, Tom Kincaid <tomjohnkincaid@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, PostgreSQL Development <pgsql-hackers@postgresql.org>, Masahiko Sawada <masahiko.sawada@2ndquadrant.com>, Tom Lane <tgl@sss.pgh.pa.us>
Date: 2021-10-06T20:35:22Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Rethink method for assigning OIDs to the template0 and postgres DBs.

  2. pg_upgrade: Preserve database OIDs.

  3. pg_upgrade: Preserve relfilenodes and tablespace OIDs.

  4. Fix for new Boolean node

  5. Improve error handling of HMAC computations

  6. Add macro RelationIsPermanent() to report relation permanence

  7. Enhance nbtree index tuple deletion.

On Mon, Oct 4, 2021 at 12:44 PM Shruthi Gowda <gowdashru@gmail.com> wrote:
> Thanks for the inputs, Robert. In the v4 patch, an unused OID (i.e, 4)
> is fixed for the template0 and the same is removed from unused oid
> list.
>
> In addition to the review comment fixes, I have removed some code that
> is no longer needed/doesn't make sense since we preserve the OIDs.

This is not a full review, but I'm wondering about this bit of code:

-       if (!RELKIND_HAS_STORAGE(relkind) || OidIsValid(relfilenode))
+       if (!RELKIND_HAS_STORAGE(relkind) || (OidIsValid(relfilenode)
&& !create_storage))
                create_storage = false;
        else
        {
                create_storage = true;
-               relfilenode = relid;
+
+               /*
+                * Create the storage with oid same as relid if relfilenode is
+                * unspecified by the caller
+                */
+               if (!OidIsValid(relfilenode))
+                       relfilenode = relid;
        }

This seems hard to understand, and I wonder if perhaps it can be
simplified. If !RELKIND_HAS_STORAGE(relkind), then we're going to set
create_storage to false if it was previously true, and otherwise just
do nothing. Otherwise, if !create_storage, we'll enter the
create_storage = false branch which effectively does nothing.
Otherwise, if !OidIsValid(relfilenode), we'll set relfilenode = relid.
So couldn't we express that like this?

if (!RELKIND_HAS_STORAGE(relkind))
    create_storage = false;
else if (create_storage && !OidIsValid(relfilenode))
    relfilenode = relid;

If so, that seems more clear.

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