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
