pgsql: Fix page modification outside of critical section in GIN

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