On Mon, Oct 4, 2021 at 12:44 PM Shruthi Gowda <gowdas...@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
