Re: [Patch] remove duplicated smgrclose

On Sat, Mar 8, 2025 at 1:37 AM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> 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.

Right. Also, I'm concerned that even if we could remove these
smgrclose() calls the benefit of removing these calls here would be
very small compared to the risk of changing the code.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com