On Mon, Oct 24, 2022 at 02:22:16PM +0200, Alvaro Herrera wrote: > I suppose it's a matter of whether any bugs are possible outside of > Neon. If yes, then definitely this should be backpatched. Offhand, I > don't see any. On the other hand, even if no bugs are known, then it's > still valuable to have all code paths do WAL insertion in the same way, > rather than having a single place that is alone in doing it in a > different way. But if we don't know of any bugs, then backpatching > might be more risk than not doing so.
I have been putting my mind on that once again, and I don't see how this would cause an issue in vanilla PG code. XLogBeginInsert() does its checks, meaning that we'd get a PANIC instead of an ERROR now that these calls are within a critical section but that should not matter because we know that recovery has ended or we would not be able to do GIN insertions like that. Then, it only switches to begininsert_called to true, that we use for sanity checks in the various WAL insert paths. As Matthias has outlined, Neon relies on begininsert_called more than we do currently. FWIW, I think that we're still fine not backpatching that, even considering the consistency of the code with stable branches. Now, if there is a strong trend in favor of a backpatch, I'd be fine with this conclusion as well. > I confess I don't understand why is it important that XLogBeginInsert is > called inside the critical section. It seems to me that that part is > only a side-effect of having to acquire the buffer locks in the critical > section. Right? Yeah, you are right that it would not matter for XLogBeginInsert(), though I'd like to think that this is a good practice on consistency grounds with anywhere else, and we respect what's documented in the README. > I noticed that line 427 logs the GIN metapage with flag REGBUF_STANDARD; > is the GIN metapage really honoring pd_upper? I see only pg_lower being > set. Hmm. Not sure. -- Michael
signature.asc
Description: PGP signature
