Re: Bug: mdunlinkfiletag unlinks mainfork seg.0 instead of indicated fork+segment - Mailing list pgsql-hackers

From surya poondla
Subject Re: Bug: mdunlinkfiletag unlinks mainfork seg.0 instead of indicated fork+segment
Date
Msg-id CAOVWO5p-+dVcyMzpgpB3bWxhVxkd_gqQGrm-jGBT=7RvpdM7yA@mail.gmail.com
Whole thread
List pgsql-hackers
Hi Matthias,

Thank you for reporting this issue and working on it.

I reviewed the whole email chain and v2 patch.

I really liked Thomas's suggestion on renaming register_unlink_segment() to register_unlink_tombstone().

The old signature with forknum and segno parameters genuinely looked like it could unlink arbitrary fork segments but mdunlinkfiletag() was silently ignoring both fields and always going to main fork seg 0 regardless. This could be very confusing.
The assert is a good belt-and-suspenders addition on top of that. If anyone ever manages to push a non-tombstone tag through this path, a cassert build will catch it immediately instead of silently deleting the wrong file.

One thing I'd flag for future work, Thomas mentioned in the thread that there's a reliability gap here where a crash between the checkpoint record being written and the unlink queue being processed will leak the tombstone. This patch doesn't fix that and it doesn't need to, but it'd be good to see someone pick that up eventually.

Overall the patch is in a good shape.

Regards, 
Surya Poondla

pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)
Next
From: "David G. Johnston"
Date:
Subject: Re: pg_publication_tables: return NULL attnames when no column list is specified