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

On Thu, Oct 7, 2021 at 2:05 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> 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.

'create_storage' flag says whether or not to create the storage when a
valid relfilenode is passed. 'create_storage' flag alone cannot make
the storage creation decision in heap_create().
Only binary upgrade flow sets the 'create_storage' flag to true and
expects storage gets created with specified relfilenode. Every other
caller/flow passes false for 'create_storage' and we still need to
create storage in heap_create() if relkind has storage. That's why I
have explicitly set 'create_storage = true' in the else flow and
initialize relfilenode on need basis.

Regards,
Shruthi KC
EnterpriseDB: http://www.enterprisedb.com