Re: [Patch] remove duplicated smgrclose

Hi Masahiko,

> > I've looked at the patch and have some comments:
> >
> > The patch removes smgrclose() calls following smgrdounlinkall(), for example:
> >
> > --- a/src/backend/catalog/storage.c
> > +++ b/src/backend/catalog/storage.c
> > @@ -686,9 +686,6 @@ smgrDoPendingDeletes(bool isCommit)
> > {
> > smgrdounlinkall(srels, nrels, false);
> >
> > - for (int i = 0; i < nrels; i++)
> > - smgrclose(srels[i]);
> > -
> > pfree(srels);
> > }
> > }
> >
> > While smgrdounlinkall() close the relation at smgr level as follow:
> >
> > /* Close the forks at smgr level */
> > for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
> > smgrsw[which].smgr_close(rels[i], forknum);
> >
> > smgrrelease(), called by smgrclose(), also does the same thing but
> > does more things as follow:.
> >
> > void
> > smgrrelease(SMgrRelation reln)
> > {
> > for (ForkNumber forknum = 0; forknum <= MAX_FORKNUM; forknum++)
> > {
> > smgrsw[reln->smgr_which].smgr_close(reln, forknum);
> > reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber;
> > }
> > reln->smgr_targblock = InvalidBlockNumber;
> > }
> >
> > Therefore, if we move such smgrclose() calls, we would end up missing
> > some operations that are done in smgrrelease() but not in
> > smgrdounlinkall(), no?
>
> Yeah, we miss setting the smgr_targblock/smgr_cached_nblocks to
> InvalidBlockNumber,
> but in the smgrDoPendingDeletes case, SMgrRelation are freed after calling
> smgrdounlinkall, so I guess it's ok not setting the InvalidBlockNumber?
>
> I did a quick seach of smgrdounlinkall usage, SMgrRelation seems
> not needed after the calling of smgrdounlinkall.
>

After a second look, I realize I'm wrong, it's that the pointers to SMgrRelation
are freed, not the SMgrRelation itself.

So I agree with you that we would end up missing some operations with
this patch.

> >
> > Regards,
> >
> > --
> > Masahiko Sawada
> > Amazon Web Services: https://aws.amazon.com
>
>
>
> --
> Regards
> Junwang Zhao

--
Regards
Junwang Zhao