Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce) - Mailing list pgsql-hackers

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