Re: Provide a pg_truncate_freespacemap function

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