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

On Fri, Sep 24, 2021 at 12:44 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Wed, Sep 22, 2021 at 3:07 PM Shruthi Gowda <gowdashru(at)gmail(dot)com> wrote:
> > > - The comment in binary_upgrade_set_pg_class_oids() is still not
> > > accurate. You removed the sentence which says "Indexes cannot have
> > > toast tables, so we need not make this probe in the index code path"
> > > but the immediately preceding sentence is still inaccurate in at least
> > > two ways. First, it only talks about tables, but the code now applies
> > > to indexes. Second, it only talks about OIDs, but now also deals with
> > > refilenodes. It's really important to fully update every comment that
> > > might be affected by your changes!
> >
> > The comment is updated.
>
> Looks good.
>
> > The SQL query will not result in duplicate rows because the first join
> > filters the duplicate rows if any with the on clause ' i.indisvalid'
> > on it. The result of the first join is further left joined with pg_class and
> > pg_class will not have duplicate rows for a given oid.
>
> Oh, you're right. My mistake.
>
> > As per your suggestion, reloid and relfilenode are absorbed in the same place.
> > An additional parameter called 'suppress_storage' is passed to heap_create()
> > which indicates whether or not to create the storage when the caller
> > passed a valid relfilenode.
>
> I find it confusing to have both suppress_storage and create_storage
> with one basically as the negation of the other. To avoid that sort of
> thing I generally have a policy that variables and options should say
> whether something should happen, rather than whether it should be
> prevented from happening. Sometimes there are good reasons - such as
> strong existing precedent - to deviate from this practice but I think
> it's good to follow when possible. So my proposal is to always have
> create_storage and never suppress_storage, and if some function needs
> to adjust the value of create_storage that was passed to it then OK.

Sure, I agree. In the latest patch, only 'create_storage' is used.

> > I did not make the changes to set the oid and relfilenode in the same call.
> > I feel the uniformity w.r.t the other function signatures in
> > pg_upgrade_support.c will be lost because currently each function sets
> > only one attribute.
> > Also, renaming the applicable function names to represent that they
> > set both oid and relfilenode will make the function name even longer.
> > We may opt to not include the relfilenode in the function name instead
> > use a generic name like binary_upgrade_set_next_xxx_pg_class_oid() but
> > then
> > we will end up with some functions that set two attributes and some
> > functions that set one attribute.
>
> OK.
>
> > I have also attached the patch to preserve the DB oid. As discussed,
> > template0 will be created with a fixed oid during initdb. I am using
> > OID 2 for template0. Even though oid 2 is already in use for the
> > 'pg_am' catalog I see no harm in using it for template0 DB because oid
> > doesn’t have to be unique across the database - it has to be unique
> > for the particular catalog table. Kindly let me know if I am missing
> > something?
> > Apparently, if we did decide to pick an unused oid for template0 then
> > I see a challenge in removing that oid from the unused oid list. I
> > could not come up with a feasible solution for handling it.
>
> You are correct that there is no intrinsic reason why the same OID
> can't be used in various different catalogs. We have a policy of not
> doing that, though; I'm not clear on the reason. Maybe it'd be OK to
> deviate from that policy here, but another option would be to simply
> change the unused_oids script (and maybe some of the others). It
> already has:
>
> my $FirstGenbkiObjectId =
> Catalog::FindDefinedSymbol('access/transam.h', '..', 'FirstGenbkiObjectId');
> push @{$oids}, $FirstGenbkiObjectId;
>
> Presumably it could be easily adapted to push the value of some other
> defined symbol into @{$oids} also, thus making that OID in effect
> used.

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.

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