Message-ID: <8acb1f83-2945-42cf-8795-780c94bbed36@oss.nttdata.com>
Date: Sun, 21 Jul 2024 13:51:56 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Provide a pg_truncate_freespacemap function
To: Ronan Dunklau <ronan.dunklau@aiven.io>, Stephen Frost <sfrost@snowman.net>
Cc: pgsql-hackers@lists.postgresql.org
References: <5446938.Sb9uPGUboI@aivenlaptop>
 <ZejD7P+i2ZPDw8O/@tamriel.snowman.net> <4331161.ejJDZkT8p0@aivenlaptop>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <4331161.ejJDZkT8p0@aivenlaptop>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/8acb1f83-2945-42cf-8795-780c94bbed36%40oss.nttdata.com>
Precedence: bulk


On 2024/03/07 16:59, Ronan Dunklau wrote:
> Le mercredi 6 mars 2024, 20:28:44 CET Stephen Frost a écrit :
>> I agree that this would generally be a useful thing to have.
> 
> Thanks !
> 
>>
>>> Does that seem correct ?
>>
>> Definitely needs to have a 'REVOKE ALL ON FUNCTION' at the end of the
>> upgrade script, similar to what you'll find at the bottom of
>> pg_visibility--1.1.sql in the tree today, otherwise anyone could run it.
>>
>> Beyond that, I'd suggest a function-level comment above the definition
>> of the function itself (which is where we tend to put those- not at the
>> point where we declare the function).
> 
> Thank you for the review. Here is an updated patch for both of those.

Here are my review comments:

The documentation for pg_freespace needs updating.


A regression test for pg_truncate_freespace_map() should be added.


+	/* Only some relkinds have a freespace map */
+	if (!RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind))
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("relation \"%s\" is of wrong relation kind",
+						RelationGetRelationName(rel)),
+				 errdetail_relkind_not_supported(rel->rd_rel->relkind)));

An index can have an FSM, but this code doesn't account for that.


+		smgrtruncate(RelationGetSmgr(rel), &fork, 1, &block);

Shouldn't truncation be performed after WAL-logging due to the WAL rule?
I'm not sure if the current order might actually cause significant issues
in FSM truncation case, though.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION