[Patch] remove duplicated smgrclose

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com> wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall() and smgrclose().
But both functions would close SMgrRelation object, it's dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit message.

--
Regards
Junwang Zhao

Hi, Junwang,

Thank you for the review and excellent summary in commit message!

This is my first contribution to community, and not so familiar with the
overall process.
After reading the process again, it looks like that I'm not qualified to
submit the patch to commitfest as I never had reviewed others' work. :(
If so, could you please help to submit it to commitfest?

Best Regards,
Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月1日周四 20:32写道：

Hi Steven,

On Fri, Aug 2, 2024 at 12:12 PM Steven Niu <niushiji@gmail.com> wrote:

Hi, Junwang,

Thank you for the review and excellent summary in commit message!

This is my first contribution to community, and not so familiar with the overall process.
After reading the process again, it looks like that I'm not qualified to submit the patch to commitfest as I never had reviewed others' work. :(
If so, could you please help to submit it to commitfest?

https://commitfest.postgresql.org/49/5149/

I can not find your profile on commitfest so I left the author as empty,
have you ever registered? If you have a account, you can put your
name in the Authors list.

Best Regards,
Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月1日周四 20:32写道：

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com> wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall() and smgrclose().
But both functions would close SMgrRelation object, it's dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit message.

--
Regards
Junwang Zhao

--
Regards
Junwang Zhao

Thanks, I have set my name in the Authors column of CF.

Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月2日周五 13:22写道：

On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku@gmail.com> wrote:

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com> wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall() and smgrclose().
But both functions would close SMgrRelation object, it's dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit message.

--
Regards
Junwang Zhao

Hi all!
This change looks good to me. However, i have an objection to these
lines from v2:

/* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);

Why do we do this? This seems to be an unrelated change given thread
$subj. This is just a pure refactoring job, which deserves a separate
patch. There is similar coding in
smgrdestroy function:

```
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
smgrsw[reln->smgr_which].smgr_close(reln, forknum);
```

So, I feel like these two places should be either changed together or
not be altered at all. And is it definitely a separate change.

--
Best regards,
Kirill Reshke

On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku@gmail.com> wrote:

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com> wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall() and smgrclose().
But both functions would close SMgrRelation object, it's dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit message.

--
Regards
Junwang Zhao

Hi all!
This change looks good to me. However, i have an objection to these
lines from v2:

/* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);

Why do we do this? This seems to be an unrelated change given thread
$subj. This is just a pure refactoring job, which deserves a separate
patch. There is similar coding in
smgrdestroy function:

```
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
smgrsw[reln->smgr_which].smgr_close(reln, forknum);
```

So, I feel like these two places should be either changed together or
not be altered at all. And is it definitely a separate change.

Yeah, I tend to agree with you, maybe we should split the patch
into two.

Steven, could you do this?

--
Best regards,
Kirill Reshke

--
Regards
Junwang Zhao

Kirill,

Good catch!
I will split the patch into two to cover both cases.

Thanks,
Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月9日周五 18:19写道：

Junwang, Kirill,

The split work has been done. I created a new patch for removing
redundant smgrclose() function as attached.
Please help review it.

Thanks,
Steven

Steven Niu <niushiji@gmail.com> 于2024年8月12日周一 18:11写道：

On Wed, Aug 14, 2024 at 2:35 PM Steven Niu <niushiji@gmail.com> wrote:

Junwang, Kirill,

The split work has been done. I created a new patch for removing redundant smgrclose() function as attached.
Please help review it.

Patch looks good, actually you can make the refactoring code as v3-0002-xxx
by using:

git format-patch -2 -v 3

Another kind reminder: Do not top-post when you reply ;)

Thanks,
Steven

Steven Niu <niushiji@gmail.com> 于2024年8月12日周一 18:11写道：

Kirill,

Good catch!
I will split the patch into two to cover both cases.

Thanks,
Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月9日周五 18:19写道：

On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku@gmail.com> wrote:

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com> wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall() and smgrclose().
But both functions would close SMgrRelation object, it's dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit message.

--
Regards
Junwang Zhao

Hi all!
This change looks good to me. However, i have an objection to these
lines from v2:

/* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);

Why do we do this? This seems to be an unrelated change given thread
$subj. This is just a pure refactoring job, which deserves a separate
patch. There is similar coding in
smgrdestroy function:

```
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
smgrsw[reln->smgr_which].smgr_close(reln, forknum);
```

So, I feel like these two places should be either changed together or
not be altered at all. And is it definitely a separate change.

Yeah, I tend to agree with you, maybe we should split the patch
into two.

Steven, could you do this?

--
Best regards,
Kirill Reshke

--
Regards
Junwang Zhao

--
Regards
Junwang Zhao

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月15日周四 18:03写道：

On Wed, Aug 14, 2024 at 2:35 PM Steven Niu <niushiji@gmail.com> wrote:

Junwang, Kirill,

The split work has been done. I created a new patch for removing

redundant smgrclose() function as attached.

Please help review it.

Patch looks good, actually you can make the refactoring code as v3-0002-xxx
by using:

git format-patch -2 -v 3

Another kind reminder: Do not top-post when you reply ;)

Thanks,
Steven

Steven Niu <niushiji@gmail.com> 于2024年8月12日周一 18:11写道：

Kirill,

Good catch!
I will split the patch into two to cover both cases.

Thanks,
Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月9日周五 18:19写道：

On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <reshkekirill@gmail.com>

wrote:

On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku@gmail.com> wrote:

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com>

wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall() and

smgrclose().

But both functions would close SMgrRelation object, it's

dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit

message.

--
Regards
Junwang Zhao

Hi all!
This change looks good to me. However, i have an objection to these
lines from v2:

/* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);

Why do we do this? This seems to be an unrelated change given thread
$subj. This is just a pure refactoring job, which deserves a separate
patch. There is similar coding in
smgrdestroy function:

```
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
smgrsw[reln->smgr_which].smgr_close(reln, forknum);
```

So, I feel like these two places should be either changed together or
not be altered at all. And is it definitely a separate change.

Yeah, I tend to agree with you, maybe we should split the patch
into two.

Steven, could you do this?

--
Best regards,
Kirill Reshke

--
Regards
Junwang Zhao

--
Regards
Junwang Zhao

OK， thanks for your kind help.

Steven

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Hi

the patch looks good to me as well. Calling smgrclose() right after calling smgrdounlinkall() does seem
unnecessary as it is already done inside smgrdounlinkall() as you mentioned. I checked the commit logs
and seems like the code has been like this for over 10 years. One difference is that smgrdounlinkall() does
not reset smgr_cached_nblocks and smgr_targblock but that does not seem to matter as it is about to
remove the physical files.

While leaving them like this does no harm because smgrclose() simply does nothing if the relation has already
been closed, it does look weird that the code tries to close the relation after smgrdounlinkall(), because the
physical files have just been deleted when smgrdounlinkall() completes, and we try to close something that
has been deleted ?!

---------------------------
Cary Huang
Highgo software Canada
www.highgo.ca

On Wed, 14 Aug 2024 at 11:35, Steven Niu <niushiji@gmail.com> wrote:

Junwang, Kirill,

The split work has been done. I created a new patch for removing redundant smgrclose() function as attached.
Please help review it.

Thanks,
Steven

Steven Niu <niushiji@gmail.com> 于2024年8月12日周一 18:11写道：

Kirill,

Good catch!
I will split the patch into two to cover both cases.

Thanks,
Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月9日周五 18:19写道：

On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku@gmail.com> wrote:

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com> wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall() and smgrclose().
But both functions would close SMgrRelation object, it's dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit message.

--
Regards
Junwang Zhao

Hi all!
This change looks good to me. However, i have an objection to these
lines from v2:

/* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);

Why do we do this? This seems to be an unrelated change given thread
$subj. This is just a pure refactoring job, which deserves a separate
patch. There is similar coding in
smgrdestroy function:

```
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
smgrsw[reln->smgr_which].smgr_close(reln, forknum);
```

So, I feel like these two places should be either changed together or
not be altered at all. And is it definitely a separate change.

Yeah, I tend to agree with you, maybe we should split the patch
into two.

Steven, could you do this?

--
Best regards,
Kirill Reshke

--
Regards
Junwang Zhao

Hi!
Looks like discussion on the subject is completed, and no open items
left, so I will try to mark commitfest [1]https://commitfest.postgresql.org/50/5149/ -- Best regards, Kirill Reshke entry as Ready For
Committer.

[1]: https://commitfest.postgresql.org/50/5149/ -- Best regards, Kirill Reshke
--
Best regards,
Kirill Reshke

Hi,

On Sun, Oct 27, 2024 at 12:05 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

On Wed, 14 Aug 2024 at 11:35, Steven Niu <niushiji@gmail.com> wrote:

Junwang, Kirill,

The split work has been done. I created a new patch for removing redundant smgrclose() function as attached.
Please help review it.

Thanks,
Steven

Steven Niu <niushiji@gmail.com> 于2024年8月12日周一 18:11写道：

Kirill,

Good catch!
I will split the patch into two to cover both cases.

Thanks,
Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月9日周五 18:19写道：

On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku@gmail.com> wrote:

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com> wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall() and smgrclose().
But both functions would close SMgrRelation object, it's dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit message.

--
Regards
Junwang Zhao

Hi all!
This change looks good to me. However, i have an objection to these
lines from v2:

/* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);

Why do we do this? This seems to be an unrelated change given thread
$subj. This is just a pure refactoring job, which deserves a separate
patch. There is similar coding in
smgrdestroy function:

```
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
smgrsw[reln->smgr_which].smgr_close(reln, forknum);
```

So, I feel like these two places should be either changed together or
not be altered at all. And is it definitely a separate change.

Yeah, I tend to agree with you, maybe we should split the patch
into two.

Steven, could you do this?

--
Best regards,
Kirill Reshke

--
Regards
Junwang Zhao

Hi!
Looks like discussion on the subject is completed, and no open items
left, so I will try to mark commitfest [1] entry as Ready For
Committer.

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?

Regards,

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

On Sat, Mar 8, 2025 at 12:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi,

On Sun, Oct 27, 2024 at 12:05 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

On Wed, 14 Aug 2024 at 11:35, Steven Niu <niushiji@gmail.com> wrote:

Junwang, Kirill,

The split work has been done. I created a new patch for removing redundant smgrclose() function as attached.
Please help review it.

Thanks,
Steven

Steven Niu <niushiji@gmail.com> 于2024年8月12日周一 18:11写道：

Kirill,

Good catch!
I will split the patch into two to cover both cases.

Thanks,
Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月9日周五 18:19写道：

On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku@gmail.com> wrote:

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com> wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall() and smgrclose().
But both functions would close SMgrRelation object, it's dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit message.

--
Regards
Junwang Zhao

Hi all!
This change looks good to me. However, i have an objection to these
lines from v2:

/* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);

Why do we do this? This seems to be an unrelated change given thread
$subj. This is just a pure refactoring job, which deserves a separate
patch. There is similar coding in
smgrdestroy function:

```
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
smgrsw[reln->smgr_which].smgr_close(reln, forknum);
```

So, I feel like these two places should be either changed together or
not be altered at all. And is it definitely a separate change.

Yeah, I tend to agree with you, maybe we should split the patch
into two.

Steven, could you do this?

--
Best regards,
Kirill Reshke

--
Regards
Junwang Zhao

Hi!
Looks like discussion on the subject is completed, and no open items
left, so I will try to mark commitfest [1] entry as Ready For
Committer.

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.

Regards,

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

--
Regards
Junwang Zhao

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

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

Masahiko Sawada <sawada.mshk@gmail.com> 于2025年3月8日周六 12:04写道：

Hi,

On Sun, Oct 27, 2024 at 12:05 PM Kirill Reshke <reshkekirill@gmail.com>
wrote:

On Wed, 14 Aug 2024 at 11:35, Steven Niu <niushiji@gmail.com> wrote:

Junwang, Kirill,

The split work has been done. I created a new patch for removing

redundant smgrclose() function as attached.

Please help review it.

Thanks,
Steven

Steven Niu <niushiji@gmail.com> 于2024年8月12日周一 18:11写道：

Kirill,

Good catch!
I will split the patch into two to cover both cases.

Thanks,
Steven

Junwang Zhao <zhjwpku@gmail.com> 于2024年8月9日周五 18:19写道：

On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <reshkekirill@gmail.com>

wrote:

On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku@gmail.com>

wrote:

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@gmail.com>

wrote:

Hello, hackers,

I think there may be some duplicated codes.
Function smgrDoPendingDeletes() calls both smgrdounlinkall()

and smgrclose().

But both functions would close SMgrRelation object, it's

dupliacted behavior?

So I make this patch. Could someone take a look at it?

Thanks for your help,
Steven

From Highgo.com

You change LGTM, but the patch seems not to be applied to HEAD,
I generate the attached v2 using `git format` with some commit

message.

--
Regards
Junwang Zhao

Hi all!
This change looks good to me. However, i have an objection to these
lines from v2:

/* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);

Why do we do this? This seems to be an unrelated change given

thread

$subj. This is just a pure refactoring job, which deserves a

separate

patch. There is similar coding in
smgrdestroy function:

```
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
smgrsw[reln->smgr_which].smgr_close(reln, forknum);
```

So, I feel like these two places should be either changed together

or

not be altered at all. And is it definitely a separate change.

Yeah, I tend to agree with you, maybe we should split the patch
into two.

Steven, could you do this?

--
Best regards,
Kirill Reshke

--
Regards
Junwang Zhao

Hi!
Looks like discussion on the subject is completed, and no open items
left, so I will try to mark commitfest [1] entry as Ready For
Committer.

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?

Regards,

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

Hi, Masahiko

Thanks for your comments! I understand your concern as you stated.
However, my initial patch was split into two parts as Kirill suggested.
This thread is about the first part. Another part is here:
https://commitfest.postgresql.org/patch/5149/
Or you can take a look at the v2-0001-remove-duplicated-smgrclose.patch
</messages/by-id/attachment/163268/v2-0001-remove-duplicated-smgrclose.patch&gt;
in
this thread for the complete change.

I think either we review the v2-patch, or review the both 5149 and 5196
CFs, for my complete change.
There should be no missing operations.

Please let me know if you have more comments.

Best Regards,
Steven

Hi, Masahiko

Thanks for your comments! I understand your concern as you stated.
However, my initial patch was split into two parts as Kirill suggested.
This thread is about the first part. Another part is here: https://commitfest.postgresql.org/patch/5149/
Or you can take a look at the v2-0001-remove-duplicated-smgrclose.patch in this thread for the complete change.

I think either we review the v2-patch, or review the both 5149 and 5196 CFs, for my complete change.
There should be no missing operations.

@@ -482,13 +482,11 @@ smgrdounlinkall(SMgrRelation *rels, int nrels,
bool isRedo)
for (i = 0; i < nrels; i++)
{
RelFileLocatorBackend rlocator = rels[i]->smgr_rlocator;
- int which = rels[i]->smgr_which;

rlocators[i] = rlocator;

  /* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);
  }

Yeah, you are adjusting the behavior by moving the `smgrclose` operation
after the `smgrdounlinkall` to the `smgrdounlinkall` function itself.

Seems no missing operations in v2-patch. Thanks.

Please let me know if you have more comments.

Best Regards,
Steven

--
Regards
Junwang Zhao

On Mon, Mar 10, 2025 at 3:08 AM Steven Niu <niushiji@gmail.com> wrote:

Hi, Masahiko

Thanks for your comments! I understand your concern as you stated.
However, my initial patch was split into two parts as Kirill suggested.
This thread is about the first part. Another part is here: https://commitfest.postgresql.org/patch/5149/
Or you can take a look at the v2-0001-remove-duplicated-smgrclose.patch in this thread for the complete change.

I think either we review the v2-patch, or review the both 5149 and 5196 CFs, for my complete change.
There should be no missing operations.

Please let me know if you have more comments.

What is the main point of removing these duplication? While I can see
that in smgrDoPendingDeletes(), we do
'smgrsw[reln->smgr_which].smgr_close(reln, forknum);' twice for each
relation: in smgrdounlinkall() and in smgrrelease(), I'm not sure what
this change benefits to us. Particularly, we quick return from the
second mdclose() call as all segment files are already closed. Have
you experienced a case like where the system got stuck or got very
slower due to these duplicated calls?

Also, we might need to pay attention that with the patch
smgrdounlinkall() came to depend on smgrclose(). For example, the more
works we add in smgrclose() in the future, the more works
smgrdounlinkall() will do, which doesn't happen with the current code
as smgrdounlinkall() depends on mdclose().

Given that the patched codes doesn't do exactly the same things as
before (e.g, smgrdounlinkall() would end up resetting
reln->smgr_cached_nblocks[forknum] too), I think we need some reasons
for legitimating this change.

Regards,

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