Re: In-placre persistance change of a relation - Mailing list pgsql-hackers
From | Michael Paquier |
---|---|
Subject | Re: In-placre persistance change of a relation |
Date | |
Msg-id | Zl5bKIdOWiHHb-9N@paquier.xyz Whole thread Raw |
In response to | Re: In-placre persistance change of a relation (Jelte Fennema-Nio <postgres@jeltef.nl>) |
Responses |
Re: In-placre persistance change of a relation
|
List | pgsql-hackers |
On Tue, May 28, 2024 at 04:49:45PM -0700, Jelte Fennema-Nio wrote: > Two notes after looking at this quickly during the advanced patch > feedback session: > > 1. I would maybe split 0003 into two separate patches. One to make SET > UNLOGGED fast, which seems quite easy to do because no WAL is needed. > And then a follow up to make SET LOGGED fast, which does all the > XLOG_FPI stuff. Yeah, that would make sense. The LOGGED->UNLOGGED part is straight-forward because we only care about the init fork. The UNLOGGED->LOGGED case bugs me, though, a lot. > 2. When wal_level = minitam, still some WAL logging is needed. The > pages that were changed since the last still need to be made available > for crash recovery. More notes from me, as I was part of this session. + * XXXX: Some access methods don't support in-place persistence + * changes. GiST uses page LSNs to figure out whether a block has been [...] + if (r->rd_rel->relkind == RELKIND_INDEX && + /* GiST is excluded */ + r->rd_rel->relam != BTREE_AM_OID && + r->rd_rel->relam != HASH_AM_OID && + r->rd_rel->relam != GIN_AM_OID && + r->rd_rel->relam != SPGIST_AM_OID && + r->rd_rel->relam != BRIN_AM_OID) This knowledge should not be encapsulated in the backend code. The index AMs should be able to tell, instead, if they are able to support this code path so as any out-of-core index AM can decide things on its own. This ought to be split in its own patch, simple enough as of a boolean or a routine telling how this backend path should behave. + for (fork = 0; fork < INIT_FORKNUM; fork++) + { + if (smgrexists(RelationGetSmgr(r), fork)) + log_newpage_range(r, fork, 0, + smgrnblocks(RelationGetSmgr(r), fork), + false); + } A simple copy of the blocks means that we keep anything bloated in them, while a rewrite in ALTER TABLE means that we would start afresh by deforming the tuples from the origin before giving them to the target, without any bloat. The compression of the FPWs and the removal of the holes in the pages would surely limit the impact, but this has not been discussed on this thread, and this is a nice property of the existing implementation that would get silently removed by this patch set. Another point that Nathan has made is that it may be more appealling to study how this is better than an integration with the multi-INSERT APIs into AMs, so as it is possible to group the inserts in batches rather than process them one-at-a-time, see [1]. I am ready to accept that what this patch does is more efficient as long as everything is block-based in some cases. Still there is a risk-vs-gain argument here, and I am not sure whether what we have here is a good tradeoff compared to the potential risk of breaking things. The amount of new infrastructure is large for this code path. Grouping the inserts in large batches may finish by being more efficient than a WAL stream full of FPWs, as well, even if toast values are deformed? So perhaps there is an argument for making that optional at query level, instead. As a hole, I can say that grouping the INSERTs will be always more efficient, while what we have here can be less efficient in some cases. I'm OK to be outvoted, but the level of complications created by this block-based copy and WAL-logging concerns me when it comes to tweaking the relpersistence like that. [1]: https://commitfest.postgresql.org/48/4777/ -- Michael
Attachment
pgsql-hackers by date: