Subject: a litter question about mdunlinkfiletag function

From: px shi <[email-masked]>
Date: Mon, 30 Sep 2024 10:43:17 +0800

*Hi, hackers*

*When calculating the path, *forknum* is hardcoded as *MAIN_FORKNUM*:*
/* Compute the path. */
p = relpathperm(ftag->rnode, MAIN_FORKNUM);

*But since the *ftag* structure already contains *forknum*:*
typedef struct FileTag
{
int16 handler; /* SyncRequestHandler value, saving space */
int16 forknum; /* ForkNumber, saving space */
RelFileNode rnode;
uint32 segno;
} FileTag;

*Wouldn’t it be more flexible to use the value from the *ftag* structure
directly?*

*Best regards, *

*Pixian Shi*

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 14 Oct 2024 16:59:37 -0400

On Mon, Sep 30, 2024 at 10:43:17AM +0800, px shi wrote:
> Hi, hackers
> 
> When calculating the path, forknum is hardcoded as MAIN_FORKNUM:
> 
> /* Compute the path. */
> But since the ftag structure already contains forknum:
> 
> typedef struct FileTag
> {
> Wouldn’t it be more flexible to use the value from the ftag structure directly?

You will find other places where relpathperm() is called without having
a FileTag structure available, e.g. ReorderBufferProcessTXN().

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"

---

From: px shi <[email-masked]>
Date: Tue, 15 Oct 2024 10:49:46 +0800

> a FileTag structure available, e.g. ReorderBufferProcessTXN().

I apologize for the confusion. What I meant to say is that in the
mdunlinkfiletag() function, the forknum is currently hardcoded as
MAIN_FORKNUM when calling relpathperm(). While mdunlinkfiletag currently
only handles MAIN_FORKNUM, wouldn’t it be more flexible to retrieve the
forknum from the ftag structure instead?

---

From: Matthias van de Meent <[email-masked]>
Date: Sat, 21 Dec 2024 02:29:31 +0100

On Tue, 15 Oct 2024 at 04:50, px shi <[email-masked]> wrote:
> I apologize for the confusion. What I meant to say is that in the mdunlinkfiletag() function, the forknum is currently hardcoded as MAIN_FORKNUM when calling relpathperm(). While mdunlinkfiletag currently only handles MAIN_FORKNUM, wouldn’t it be more flexible to retrieve the forknum from the ftag structure instead?

I just noticed this mail thread as I was searching the archives for
other mentions of `mdunlinkfiletag` when doing some more digging on
uses of that function, to back my own bug report of what looks like
the same issue. See [0].

As was explained to me by Thomas, the reason why MAIN_FORKNUM is
hardcoded here (and why ftag.segno is also ignored) is that this code
is only ever reached for FileTag values with forknum=MAIN_FORKNUM (and
segno is also always 0) with the code in Postgres' repository. The
patch proposed in [0] is supposed to make that more clear to
developers.

I suspect the code will be further updated to include the correct fork
number and segment number when there is a need to unlink
non-MAIN_FORKNUM or non-segno=0 files in mdunlinkfiletag.

Kind regards,

Matthias van de Meent

[0] https://www.postgresql.org/message-id/flat/CAEze2WiWt%2B9%2BOnqW1g9rKz0gqxymmt%3Doe6pKAEDrutdfpDMpTw%40mail.gmail.com