Thread: pgsql: Fix page modification outside of critical section in GIN
Fix page modification outside of critical section in GIN By oversight 52ac6cd2d0 makes ginDeletePage() sets pd_prune_xid of page to be deleted before entering the critical section. It appears that only versions 11 and later were affected by this oversight. Backpatch-through: 11 Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/b107140804817cc30a4069b1bb5545aa3ea0ce6c Modified Files -------------- src/backend/access/gin/ginvacuum.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Alexander Korotkov <akorotkov@postgresql.org> writes:
> Fix page modification outside of critical section in GIN
> By oversight 52ac6cd2d0 makes ginDeletePage() sets pd_prune_xid of page to be
> deleted before entering the critical section. It appears that only versions 11
> and later were affected by this oversight.
I happened to notice, while reviewing stuff for the release notes,
that this patch does not do what the commit message says. The
previous code modified the pd_prune_xid of the "dBuffer" page,
but now it's modifying the pd_prune_xid of the "lBuffer" page.
Is that actually correct? If it isn't, maybe you need to move
the GinPageSetDeleteXid call down a bit further, next to the
GinPageSetDeleted call (by which time the "page" variable has
been restored to the right thing).
Another idea which would be safer if a bit more invasive is to stop
using the single "page" variable for two purposes.
regards, tom lane
I wrote:
> Alexander Korotkov <akorotkov@postgresql.org> writes:
>> Fix page modification outside of critical section in GIN
> I happened to notice, while reviewing stuff for the release notes,
> that this patch does not do what the commit message says. The
> previous code modified the pd_prune_xid of the "dBuffer" page,
> but now it's modifying the pd_prune_xid of the "lBuffer" page.
> Is that actually correct?
Since we're rapidly approaching the wrap deadline for this week's
releases, I took it upon myself to review this code more carefully,
and concluded that indeed it isn't correct. I pushed a fix that
moves the GinPageSetDeleteXid call again.
regards, tom lane
On Sun, Feb 9, 2020 at 8:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I wrote: > > Alexander Korotkov <akorotkov@postgresql.org> writes: > >> Fix page modification outside of critical section in GIN > > > I happened to notice, while reviewing stuff for the release notes, > > that this patch does not do what the commit message says. The > > previous code modified the pd_prune_xid of the "dBuffer" page, > > but now it's modifying the pd_prune_xid of the "lBuffer" page. > > Is that actually correct? > > Since we're rapidly approaching the wrap deadline for this week's > releases, I took it upon myself to review this code more carefully, > and concluded that indeed it isn't correct. I pushed a fix that > moves the GinPageSetDeleteXid call again. Sorry for delayed response. Yes, my fix for oversight contain oversight itself. I confirm the fix you've committed is correct. Thank you! ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> Sorry for delayed response. Yes, my fix for oversight contain
> oversight itself. I confirm the fix you've committed is correct.
> Thank you!
Thanks for checking!
regards, tom lane