Horton

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

Shruthi Gowda <gowdashru@gmail.com>

View thread
From: Shruthi Gowda <gowdashru@gmail.com>
To: Robert Haas <robertmhaas@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-07T07:24:34Z
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 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