Обсуждение: Connections hang indefinitely while taking a gin index's LWLockbuffer_content lock

On Tue, Nov 6, 2018 at 10:05 AM chenhj <chjischj@163.com> wrote:
> I  analyzed the btree block where lwlock deadlock occurred, as follows:

Thank you for doing this important work.

You're using Postgres 10.2. While that version isn't current with all
GIN bug fixes, it does have this important one:

"Ensure that vacuum will always clean up the pending-insertions list
of a GIN index (Masahiko Sawada)"

Later GIN fixes seem unlikely to be relevant to your issue. I think
that this is probably a genuine, new bug.

> The ginInsertValue() function above gets the lwlock in the order described in the README.

> However, ginScanToDelete() depth-first scans the btree and gets the EXCLUSIVE lock, which creates a deadlock.
> Is the above statement correct? If so, deadlocks should easily happen.

I have been suspicious of deadlock hazards in the code for some time
-- particularly around pending list cleanup. I go into a lot of detail
on my suspicions here:


https://www.postgresql.org/message-id/flat/CAH2-WzmfUpRjWcUq3%2B9ijyum4AJ%2Bk-meGT8_HnxBMpKz1aNS-g%40mail.gmail.com#ea5af1088adfacb3d0ba88313be1a5e3

I note that your first deadlock involve the following kinds of backends:

* ginInsertCleanup() calls from a regular backend, which will have a
backend do things that VACUUM generally only gets to do, like call
RecordFreeIndexPage().

* (auto)VACUUM processes.

Your second/recovery deadlock involves:

* Regular read-only queries.

* Recovery code.

Quite a lot of stuff is involved here!

The code in this area is way too complicated, and I haven't thought
about it in about a year, so it's hard for me to be sure of anything
at the moment. My guess is that there is confusion about the type of
page expected within one or more blocks (e.g. entry tree vs. pending
list), due to a race condition in block deletion and/or recycling --
again, I've suspected something like this could happen for some time.
The fact that you get a distinct deadlock during recovery is
consistent with that theory.

It's safe to say that promoting the asserts on gin page type into
"can't happen" elog errors in places like scanPendingInsert() and
ginInsertCleanup() would be a good start. Note that we already did
similar Assert-elog(ERROR) promotion this for posting tree code, when
similar bugs appeared way back in 2013.

-- 
Peter Geoghegan

On Mon, Oct 29, 2018 at 12:04 PM chenhj <chjischj@163.com> wrote:
>     #4  0x00000000007095ac in LWLockAcquire (lock=0x2aaab4009564, mode=LW_EXCLUSIVE) at lwlock.c:1233
>     #5  0x0000000000490a32 in ginStepRight (buffer=6713826, index=<optimized out>, lockmode=lockmode@entry=2) at
ginbtree.c:174
>     #6  0x0000000000490c1c in ginFinishSplit (btree=btree@entry=0x7ffd81e4f950, stack=0x28eebf8,
freestack=freestack@entry=1'\001', buildStats=buildStats@entry=0x0) at ginbtree.c:701
 
>     #7  0x0000000000491396 in ginInsertValue (btree=btree@entry=0x7ffd81e4f950, stack=<optimized out>,
insertdata=insertdata@entry=0x7ffd81e4f940,buildStats=buildStats@entry=0x0)
 
>         at ginbtree.c:773
>     #8  0x000000000048fb42 in ginInsertItemPointers (index=<optimized out>, rootBlkno=rootBlkno@entry=644,
items=items@entry=0x2916598,nitem=nitem@entry=353, buildStats=buildStats@entry=0x0)
 
>         at gindatapage.c:1907
>     #9  0x000000000048a9ea in ginEntryInsert (ginstate=ginstate@entry=0x28e7ef8, attnum=<optimized out>,
key=42920440,category=<optimized out>, items=0x2916598, nitem=353,
 
>         buildStats=buildStats@entry=0x0) at gininsert.c:214
>     #10 0x0000000000496d94 in ginInsertCleanup (ginstate=ginstate@entry=0x28e7ef8, full_clean=full_clean@entry=0
'\000',fill_fsm=fill_fsm@entry=1 '\001',
 
>         forceCleanup=forceCleanup@entry=0 '\000', stats=stats@entry=0x0) at ginfast.c:883
>     #11 0x0000000000497727 in ginHeapTupleFastInsert (ginstate=ginstate@entry=0x28e7ef8,
collector=collector@entry=0x7ffd81e4fe60)at ginfast.c:448
 
>     #12 0x000000000048b209 in gininsert (index=<optimized out>, values=0x7ffd81e4ff40, isnull=0x7ffd81e50040 "",
ht_ctid=0x280d98c,heapRel=<optimized out>, checkUnique=<optimized out>,
 
>         indexInfo=0x28b5aa8) at gininsert.c:522

I want to point something out about the above inserter represented by
this stack trace. It deadlocks against VACUUM from within the
following ginEntryInsert() call:

            /*
             * While we left the page unlocked, more stuff might have gotten
             * added to it.  If so, process those entries immediately.  There
             * shouldn't be very many, so we don't worry about the fact that
             * we're doing this with exclusive lock. Insertion algorithm
             * guarantees that inserted row(s) will not continue on next page.
             * NOTE: intentionally no vacuum_delay_point in this loop.
             */
            if (PageGetMaxOffsetNumber(page) != maxoff)
            {
                ginInitBA(&accum);
                processPendingPage(&accum, &datums, page, maxoff + 1);

                ginBeginBAScan(&accum);
                while ((list = ginGetBAEntry(&accum,
                                             &attnum, &key, &category,
&nlist)) != NULL)
                    ginEntryInsert(ginstate, attnum, key, category, //
<--- deadlocks
                                   list, nlist, NULL);
            }

Clearly this particular call to ginEntryInsert() from within
ginInsertCleanup() is generally supposed to be a rare occurrence. It's
not surprising that it's hard for you to hit this issue.

BTW, I strongly doubt that disabling hugepages has fixed anything,
though it may have accidentally masked the problem. This is an issue
around the basic correctness of the locking protocols used by GIN.

-- 
Peter Geoghegan

On Mon, Oct 29, 2018 at 12:04 PM chenhj <chjischj@163.com> wrote:
> ## stack of autovacuum:Acquire lock 0x2aaab670dfe4 and hold 0x2aaab4009564
> --------------------------------------
>     (gdb) bt
>     #0  0x00007fe11552379b in do_futex_wait.constprop.1 () from /lib64/libpthread.so.0
>     #1  0x00007fe11552382f in __new_sem_wait_slow.constprop.0 () from /lib64/libpthread.so.0
>     #2  0x00007fe1155238cb in sem_wait@@GLIBC_2.2.5 () from /lib64/libpthread.so.0
>     #3  0x000000000069d362 in PGSemaphoreLock (sema=0x2aaaaac07eb8) at pg_sema.c:310
>     #4  0x00000000007095ac in LWLockAcquire (lock=0x2aaab670dfe4, mode=LW_EXCLUSIVE) at lwlock.c:1233
>     #5  0x00000000004947ef in ginScanToDelete (gvs=gvs@entry=0x7ffd81e4d0f0, blkno=701, isRoot=isRoot@entry=0 '\000',
parent=parent@entry=0x7ffd81e4c790,myoff=myoff@entry=37) at ginvacuum.c:262
 
>     #6  0x0000000000494874 in ginScanToDelete (gvs=gvs@entry=0x7ffd81e4d0f0, blkno=blkno@entry=9954,
isRoot=isRoot@entry=1'\001', parent=parent@entry=0x7ffd81e4c790, myoff=myoff@entry=0)
 
>         at ginvacuum.c:277
>     #7  0x0000000000494ed1 in ginVacuumPostingTreeLeaves (gvs=gvs@entry=0x7ffd81e4d0f0, blkno=9954,
isRoot=isRoot@entry=0'\000') at ginvacuum.c:404
 
>     #8  0x0000000000494e21 in ginVacuumPostingTreeLeaves (gvs=gvs@entry=0x7ffd81e4d0f0, blkno=644,
isRoot=isRoot@entry=1'\001') at ginvacuum.c:372
 
>     #9  0x0000000000495540 in ginVacuumPostingTree (rootBlkno=<optimized out>, gvs=0x7ffd81e4d0f0) at
ginvacuum.c:426
>     #10 ginbulkdelete (info=0x7ffd81e4f720, stats=<optimized out>, callback=<optimized out>,
callback_state=<optimizedout>) at ginvacuum.c:649
 
>     #11 0x00000000005e1194 in lazy_vacuum_index (indrel=0x3146e28, stats=stats@entry=0x28ec200,
vacrelstats=vacrelstats@entry=0x28ebc28)at vacuumlazy.c:1621
 
>     #12 0x00000000005e214d in lazy_scan_heap (aggressive=<optimized out>, nindexes=<optimized out>, Irel=<optimized
out>,vacrelstats=<optimized out>, options=16, onerel=0x28ec1f8)
 
>         at vacuumlazy.c:1311
>     #13 lazy_vacuum_rel (onerel=onerel@entry=0x3144fa8, options=options@entry=99, params=params@entry=0x289f270,
bstrategy=<optimizedout>) at vacuumlazy.c:258
 

Actually, the bigger problem is on this side of the deadlock, within
VACUUM. ginInsertCleanup() (the first/other side of the deadlock) may
have problems, but this seems worse. Commit 218f51584d5 appears to be
at fault here.

First things first: ginScanToDelete() *maintains* buffer locks on
multiple levels of a posting tree, meaning that there may be cases
where quite a few exclusive buffer locks may be held all at once (one
per level). MAX_SIMUL_LWLOCKS is 200 these days, and in practice a
B-Tree can never get that tall, but having the number of buffer locks
acquired be determined by the height of the tree is not okay, on
general principle. The nbtree code's page deletion goes to a lot of
effort to keep the number of buffer locks fixed, but nothing like that
is is attempted for GIN posting trees.

Chen's original analysis of the problem seems to be more or less
accurate: the order that ginScanToDelete() acquires buffer locks as it
descends the tree (following commit 218f51584d5) is not compatible
with the order within ginFinishSplit(). The faulty code within
ginScanToDelete() crabs/couples buffer locks while descending the
tree, whereas the code within ginFinishSplit() crabs them as it
ascends the same tree.

Teodor: Do you think that the issue is fixable? It looks like there
are serious issues with the design of 218f51584d5 to me. I don't think
the general "there can't be any inserters at this subtree" thing works
given that we have to couple buffer locks when moving right for other
reasons. We call ginStepRight() within ginFinishSplit(), for reasons
that date back to bug fix commit ac4ab97e from 2013 -- that detail is
probably important, because it seems to be what breaks the subtree
design (we don't delete in two phases or anything in GIN).

I think that you have to be doing a multi-level delete for a deadlock
to take place, which isn't particularly likely to coincide with a
concurrent insertion in general. That may explain why it's taken a
year to get a high-quality bug report.

-- 
Peter Geoghegan

On Wed, Nov 7, 2018 at 5:46 PM Peter Geoghegan <pg@bowt.ie> wrote:
> I think that you have to be doing a multi-level delete for a deadlock
> to take place, which isn't particularly likely to coincide with a
> concurrent insertion in general. That may explain why it's taken a
> year to get a high-quality bug report.

BTW, it's notable that Chen's query uses ON CONFLICT DO UPDATE.
Speculative insertion might cause just the wrong kind of churn,
involving continual recycling of heap TIDs.

-- 
Peter Geoghegan

On Wed, Nov 7, 2018 at 5:46 PM Peter Geoghegan <pg@bowt.ie> wrote:
> Teodor: Do you think that the issue is fixable? It looks like there
> are serious issues with the design of 218f51584d5 to me. I don't think
> the general "there can't be any inserters at this subtree" thing works
> given that we have to couple buffer locks when moving right for other
> reasons. We call ginStepRight() within ginFinishSplit(), for reasons
> that date back to bug fix commit ac4ab97e from 2013 -- that detail is
> probably important, because it seems to be what breaks the subtree
> design (we don't delete in two phases or anything in GIN).

Ping?

This is a thorny problem, and I'd like to get your input soon. I
suspect that reverting 218f51584d5 may be the ultimate outcome, even
though it's a v10 feature.

-- 
Peter Geoghegan

Hi!

Mail.App somehow borken my reponse with <div class="ApplePlainTextBody">, which is far beyond of my understanding of
whatis plain text, that's why I'll quote all my previous message here. Hope this mail client is OK.
 

14.11.2018, 23:13, "Andrey Borodin" <x4mmm@yandex-team.ru>:
> Hi everyone!
>
> I didn't noticed this thread for too long somehow, sorry.
>
>> 8 нояб. 2018 г., в 6:46, Peter Geoghegan <pg@bowt.ie> написал(а):
>>
>> I don't think
>> the general "there can't be any inserters at this subtree" thing works
>> given that we have to couple buffer locks when moving right for other
>> reasons. We call ginStepRight() within ginFinishSplit(), for reasons
>> that date back to bug fix commit ac4ab97e from 2013 -- that detail is
>> probably important, because it seems to be what breaks the subtree
>> design (we don't delete in two phases or anything in GIN).
>
> ginVacuumPostingTreeLeaves() holds LockBufferForCleanup() on subtree root b.
> Thus there may be no GinBtreeStack's holding pin on b at the moment.
> When you ginStepRight(b) to the parent in ginFinishSplit(), you always get to the buffer from your stack.
> Hence you can never have ginFinishSplit() deadlock with cleanup of subtree whose root is LockBufferForCleanup()'d.
>
> Is this correct or did I miss something?
>
> But we have a deadlock at hand, I'll think more about it. Something with locking protocol is clearly wrong.
>
>> 11 нояб. 2018 г., в 22:33, chenhj <chjischj@163.com> написал(а):
>>
>> The order of get lwlock in ginRedoDeletePage() may should be change from "dbuffer->pbuffer->lbuffer" to
"lbuffer->dbuffer->pbuffer". Is this right?
 
>
> This looks correct to me.
>
> Best regards, Andrey Borodin.

I've reread Chen's reports and understood his findings.

> When you ginStepRight(b) to the parent in ginFinishSplit(), you always get to the buffer from your stack.

This statement does not hold.
When you have a GinBtreeStack S, one of it's internal pages may split into new page P. If in this moment you start a
multi-leveldelete from P and cascade split from S, P is not in S and we may deadlock.
 

Correct solution seems to replace lock-coupling descent by usual B-tree searches for parent pages as in B-tree.
I think I can compose patch for consideration.

Best regards, Andrey Borodin.

Hi!

> 15 нояб. 2018 г., в 17:30, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
>
> I think I can compose patch for consideration.

I found no practical way to fix concept of "subtree-lock". Pre-locking all left siblings for cleanup would suffice, but
doesnot look very practical. 
GIN Posting tree has no all useful B-tree inventory like left links and high keys for concurrent deletes without
"subtree-lock".

Let's consider concurrent deletion.
As far as I understand, absence of high-key makes unsafe concurrent deletion of highest keys from internal pages. Thus,
internalpages cannot be deleted at all. 
PFA patch with most simple deletion algorithm:
1. Descend to leftmost leaf
2. Scan by rightlinks until rightmost page
3. Delete empty leaf pages, if they are not highest keys in corresponding parent page

Parent page is found by rightlink scan for leftmost parent with cached previous result.
Deleted page is stamped by ReadNewTransactionId()'s xid, page cannot be reused until deletion xid is beyond
RecentGlobalDataXmin.
DeleteXid is WAL-logged.
I've refactored btree->getLeftMostChild() and btree->findChildPtr() to reuse their code (removed unnecessary
parameter).

Please note, that attached patch do not fix 2nd bug found by Chen in page delete redo.

I understand that reverting commit 218f51584d5 and returning to long but finite root locks is safest solution. In this
descriptionof proposed patch I've tried to put concepts in a minimum of words. Please ping me if some concepts sound
crypticand require more clarification. 

Best regards, Andrey Borodin.

On Fri, Nov 23, 2018 at 2:18 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> I found no practical way to fix concept of "subtree-lock". Pre-locking all left siblings for cleanup would suffice,
butdoes not look very practical.
 
> GIN Posting tree has no all useful B-tree inventory like left links and high keys for concurrent deletes without
"subtree-lock".

Significantly changing the design of GIN vacuuming in back branches
for a release that's more than a year old is absolutely out of the
question. I think that your patch should be evaluated as a new v12
feature, following the revert of 218f51584d5.

> Please note, that attached patch do not fix 2nd bug found by Chen in page delete redo.
>
> I understand that reverting commit 218f51584d5 and returning to long but finite root locks is safest solution

Whatever happens with the master branch, I think that reverting commit
218f51584d5 is the only option on the table for the back branches. Its
design is based on ideas on locking protocols that are fundamentally
incorrect and unworkable. I don't have a lot of faith in our ability
to retrofit a design that fixes the issue without causing problems
elsewhere.

-- 
Peter Geoghegan

Hi,

On 2018-11-10 17:42:16 -0800, Peter Geoghegan wrote:
> On Wed, Nov 7, 2018 at 5:46 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > Teodor: Do you think that the issue is fixable? It looks like there
> > are serious issues with the design of 218f51584d5 to me. I don't think
> > the general "there can't be any inserters at this subtree" thing works
> > given that we have to couple buffer locks when moving right for other
> > reasons. We call ginStepRight() within ginFinishSplit(), for reasons
> > that date back to bug fix commit ac4ab97e from 2013 -- that detail is
> > probably important, because it seems to be what breaks the subtree
> > design (we don't delete in two phases or anything in GIN).
> 
> Ping?
> 
> This is a thorny problem, and I'd like to get your input soon. I
> suspect that reverting 218f51584d5 may be the ultimate outcome, even
> though it's a v10 feature.

Teodor, any chance for a response here?  It's not OK to commit something
and then just not respond to bug-reports, especially when they're well
diagnose and clearly point towards issues in a commit of yours.

Alexander, CCing you, perhaps you could point the thread out to Teodor?

Greetings,

Andres Freund

On Tue, Dec 4, 2018 at 8:01 PM Andres Freund <andres@anarazel.de> wrote:
> On 2018-11-10 17:42:16 -0800, Peter Geoghegan wrote:
> > On Wed, Nov 7, 2018 at 5:46 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > > Teodor: Do you think that the issue is fixable? It looks like there
> > > are serious issues with the design of 218f51584d5 to me. I don't think
> > > the general "there can't be any inserters at this subtree" thing works
> > > given that we have to couple buffer locks when moving right for other
> > > reasons. We call ginStepRight() within ginFinishSplit(), for reasons
> > > that date back to bug fix commit ac4ab97e from 2013 -- that detail is
> > > probably important, because it seems to be what breaks the subtree
> > > design (we don't delete in two phases or anything in GIN).
> >
> > Ping?
> >
> > This is a thorny problem, and I'd like to get your input soon. I
> > suspect that reverting 218f51584d5 may be the ultimate outcome, even
> > though it's a v10 feature.
>
> Teodor, any chance for a response here?  It's not OK to commit something
> and then just not respond to bug-reports, especially when they're well
> diagnose and clearly point towards issues in a commit of yours.

Unfortunately, Teodor is unreachable at least till next week.

> Alexander, CCing you, perhaps you could point the thread out to Teodor?

I'll take a look at this issue.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Dec 6, 2018 at 10:56 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Tue, Dec 4, 2018 at 8:01 PM Andres Freund <andres@anarazel.de> wrote:
> > On 2018-11-10 17:42:16 -0800, Peter Geoghegan wrote:
> > > On Wed, Nov 7, 2018 at 5:46 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > > > Teodor: Do you think that the issue is fixable? It looks like there
> > > > are serious issues with the design of 218f51584d5 to me. I don't think
> > > > the general "there can't be any inserters at this subtree" thing works
> > > > given that we have to couple buffer locks when moving right for other
> > > > reasons. We call ginStepRight() within ginFinishSplit(), for reasons
> > > > that date back to bug fix commit ac4ab97e from 2013 -- that detail is
> > > > probably important, because it seems to be what breaks the subtree
> > > > design (we don't delete in two phases or anything in GIN).
> > >
> > > Ping?
> > >
> > > This is a thorny problem, and I'd like to get your input soon. I
> > > suspect that reverting 218f51584d5 may be the ultimate outcome, even
> > > though it's a v10 feature.
> >
> > Teodor, any chance for a response here?  It's not OK to commit something
> > and then just not respond to bug-reports, especially when they're well
> > diagnose and clearly point towards issues in a commit of yours.
>
> Unfortunately, Teodor is unreachable at least till next week.
>
> > Alexander, CCing you, perhaps you could point the thread out to Teodor?
>
> I'll take a look at this issue.

I've run through the thread.

So, algorithm introduced by 218f51584d5 is broken.  It tries to
guarantee that there are no inserters in the subtree by placing
cleanup lock to subtree root, assuming that inserter holds pins on the
path from root to leaf.  But due to concurrent splits of internal
pages the pins held can be not relevant to actual path.  I don't see
the way to fix this easily.  So, I think we should revert it from back
branches and try to reimplement that in master.

However, I'd like to note that 218f51584d5 introduces two changes:
1) Cleanup locking only if there pages to delete
2) Cleanup locking only subtree root
The 2nd one is broken.  But the 1st one seems still good for me and
useful, because in vast majority of cases vacuum doesn't delete any
index pages.  So, I propose to revert 218f51584d5, but leave there
logic, which locks root for cleanup only once there are pages to
delete.  Any thoughts?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Dec 6, 2018 at 12:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> So, algorithm introduced by 218f51584d5 is broken.  It tries to
> guarantee that there are no inserters in the subtree by placing
> cleanup lock to subtree root, assuming that inserter holds pins on the
> path from root to leaf.  But due to concurrent splits of internal
> pages the pins held can be not relevant to actual path.  I don't see
> the way to fix this easily.  So, I think we should revert it from back
> branches and try to reimplement that in master.
>
> However, I'd like to note that 218f51584d5 introduces two changes:
> 1) Cleanup locking only if there pages to delete
> 2) Cleanup locking only subtree root
> The 2nd one is broken.  But the 1st one seems still good for me and
> useful, because in vast majority of cases vacuum doesn't delete any
> index pages.  So, I propose to revert 218f51584d5, but leave there
> logic, which locks root for cleanup only once there are pages to
> delete.  Any thoughts?

Can you post a patch that just removes the 2nd part, leaving the
still-correct first part?

Your proposal sounds reasonable, but I'd like to see what you have in
mind in detail before commenting.

Thanks
-- 
Peter Geoghegan

Hi!

> 7 дек. 2018 г., в 2:50, Peter Geoghegan <pg@bowt.ie> написал(а):
>
> On Thu, Dec 6, 2018 at 12:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>>
>> However, I'd like to note that 218f51584d5 introduces two changes:
>> 1) Cleanup locking only if there pages to delete
>> 2) Cleanup locking only subtree root
>> The 2nd one is broken.  But the 1st one seems still good for me and
>> useful, because in vast majority of cases vacuum doesn't delete any
>> index pages.  So, I propose to revert 218f51584d5, but leave there
>> logic, which locks root for cleanup only once there are pages to
>> delete.  Any thoughts?
>
> Can you post a patch that just removes the 2nd part, leaving the
> still-correct first part?

I like the idea of keeping cleanup lock only if there are pages to delete. It will still solve the original problem
thatcaused proposals for GIN VACUUM enhancements. 

But I must note that there is one more problem: ginVacuumPostingTreeLeaves() do not ensure that all splitted pages are
visited.It copies downlink block numbers to a temp array and then unlocks parent. It is not correct way to scan posting
treefor cleanup. 

PFA diff with following changes:
1. Always take root cleanup lock before deleting pages
2. Check for concurrent splits after scanning page

Please note, that neither applying this diff nor reverting 218f51584d5 will solve bug of page delete redo lock on
standby.

Best regards, Andrey Borodin.

On Fri, Dec 7, 2018 at 12:14 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > 7 дек. 2018 г., в 2:50, Peter Geoghegan <pg@bowt.ie> написал(а):
> > On Thu, Dec 6, 2018 at 12:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >> However, I'd like to note that 218f51584d5 introduces two changes:
> >> 1) Cleanup locking only if there pages to delete
> >> 2) Cleanup locking only subtree root
> >> The 2nd one is broken.  But the 1st one seems still good for me and
> >> useful, because in vast majority of cases vacuum doesn't delete any
> >> index pages.  So, I propose to revert 218f51584d5, but leave there
> >> logic, which locks root for cleanup only once there are pages to
> >> delete.  Any thoughts?
> >
> > Can you post a patch that just removes the 2nd part, leaving the
> > still-correct first part?
>
> I like the idea of keeping cleanup lock only if there are pages to delete. It will still solve the original problem
thatcaused proposals for GIN VACUUM enhancements. 
>
> But I must note that there is one more problem: ginVacuumPostingTreeLeaves() do not ensure that all splitted pages
arevisited. It copies downlink block numbers to a temp array and then unlocks parent. It is not correct way to scan
postingtree for cleanup. 

Hmm... In ginVacuumPostingTreeLeaves() we visit pages from parent to
children.  This is why splitted pages might be unvisited.  Should we
visit leafpages by rightlinks instead?  Then, if we have candidates
for delete, ginScanToDelete() can work the same as before 218f51584d5.

> PFA diff with following changes:
> 1. Always take root cleanup lock before deleting pages
> 2. Check for concurrent splits after scanning page
>
> Please note, that neither applying this diff nor reverting 218f51584d5 will solve bug of page delete redo lock on
standby.

ginRedoDeletePage() visits pages in following order: right(to be
deleted), parent, left.  I didn't find any description of why it has
been done so.  Any guesses don't we visit parent page first here?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Fri, Dec 7, 2018 at 12:50 AM Peter Geoghegan <pg@bowt.ie> wrote:
> On Thu, Dec 6, 2018 at 12:51 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > So, algorithm introduced by 218f51584d5 is broken.  It tries to
> > guarantee that there are no inserters in the subtree by placing
> > cleanup lock to subtree root, assuming that inserter holds pins on the
> > path from root to leaf.  But due to concurrent splits of internal
> > pages the pins held can be not relevant to actual path.  I don't see
> > the way to fix this easily.  So, I think we should revert it from back
> > branches and try to reimplement that in master.
> >
> > However, I'd like to note that 218f51584d5 introduces two changes:
> > 1) Cleanup locking only if there pages to delete
> > 2) Cleanup locking only subtree root
> > The 2nd one is broken.  But the 1st one seems still good for me and
> > useful, because in vast majority of cases vacuum doesn't delete any
> > index pages.  So, I propose to revert 218f51584d5, but leave there
> > logic, which locks root for cleanup only once there are pages to
> > delete.  Any thoughts?
>
> Can you post a patch that just removes the 2nd part, leaving the
> still-correct first part?
>
> Your proposal sounds reasonable, but I'd like to see what you have in
> mind in detail before commenting.

Yep, please find attached draft patch.

BTW, it seems that I've another bug in GIN.  README says that

"However, posting trees are only
fully searched from left to right, starting from the leftmost leaf. (The
tree-structure is only needed by insertions, to quickly find the correct
insert location). So as long as we don't delete the leftmost page on each
level, a search can never follow a downlink to page that's about to be
deleted."

But that's not really true once we teach GIN to skip parts of posting
trees in PostgreSQL 9.4.  So, there might be a risk to visit page to
be deleted using downlink.  But in order to get real problem, vacuum
should past cleanup stage and someone else should reuse this page,
before we traverse downlink.  Thus, the risk of real problem is very
low.  But it still should be addressed.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi!

Thanks, Alexander!
> 8 дек. 2018 г., в 6:54, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
>
> Yep, please find attached draft patch.

Patch seems good to me, I'll check it in more detail.
The patch gets posting item at FirstOffsetNumber instead of btree->getLeftMostChild(). This seem OK, since
dataGetLeftMostPage()is doing just the same, but with few Assert()s. 

> BTW, it seems that I've another bug in GIN.  README says that
>
> "However, posting trees are only
> fully searched from left to right, starting from the leftmost leaf. (The
> tree-structure is only needed by insertions, to quickly find the correct
> insert location). So as long as we don't delete the leftmost page on each
> level, a search can never follow a downlink to page that's about to be
> deleted."
>
> But that's not really true once we teach GIN to skip parts of posting
> trees in PostgreSQL 9.4.  So, there might be a risk to visit page to
> be deleted using downlink.  But in order to get real problem, vacuum
> should past cleanup stage and someone else should reuse this page,
> before we traverse downlink.  Thus, the risk of real problem is very
> low.  But it still should be addressed.

There's a patch above in this thread 0001-Use-correct-locking-protocol-during-GIN-posting-tree.patch where I propose
stampingevery deleted page with GinPageSetDeleteXid(page, ReadNewTransactionId()); and avoid reusing the page before
TransactionIdPrecedes(GinPageGetDeleteXid(page),RecentGlobalDataXmin). 
Should we leave alone this bug for future fixes to keep current fix noninvasive?

Best regards, Andrey Borodin.

On Sat, Dec 8, 2018 at 12:48 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > 8 дек. 2018 г., в 6:54, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
> >
> > Yep, please find attached draft patch.
>
> Patch seems good to me, I'll check it in more detail.
> The patch gets posting item at FirstOffsetNumber instead of btree->getLeftMostChild(). This seem OK, since
dataGetLeftMostPage()is doing just the same, but with few Assert()s. 

I'd like to evade creating GinBtree for just calling
getLeftMostChild().  Also, few more places in ginvacuum.c do the same.
We have the same amount of Assert()s in ginVacuumPostingTreeLeaves().
So, let's keep it uniform.

I would also like Peter Geoghegan to take a look at this patch before
committing it.

> > BTW, it seems that I've another bug in GIN.  README says that
> >
> > "However, posting trees are only
> > fully searched from left to right, starting from the leftmost leaf. (The
> > tree-structure is only needed by insertions, to quickly find the correct
> > insert location). So as long as we don't delete the leftmost page on each
> > level, a search can never follow a downlink to page that's about to be
> > deleted."
> >
> > But that's not really true once we teach GIN to skip parts of posting
> > trees in PostgreSQL 9.4.  So, there might be a risk to visit page to
> > be deleted using downlink.  But in order to get real problem, vacuum
> > should past cleanup stage and someone else should reuse this page,
> > before we traverse downlink.  Thus, the risk of real problem is very
> > low.  But it still should be addressed.
>
> There's a patch above in this thread 0001-Use-correct-locking-protocol-during-GIN-posting-tree.patch where I propose
stampingevery deleted page with GinPageSetDeleteXid(page, ReadNewTransactionId()); and avoid reusing the page before
TransactionIdPrecedes(GinPageGetDeleteXid(page),RecentGlobalDataXmin). 
> Should we leave alone this bug for future fixes to keep current fix noninvasive?

I think since is separate bug introduced in PostgreSQL 9.4, which
should be backpatched with separate commit.  Could you please extract
patch dealing with GinPageSetDeleteXid() and GinPageGetDeleteXid().
The rest of work made in your patch should be considered for master.

BTW, what do you think about locking order in ginRedoDeletePage()?  Do
you have any explanation of current behavior?  I'll try to reach
Teodor tomorrow with this question.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi!

> 10 дек. 2018 г., в 0:25, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
>>
>> There's a patch above in this thread 0001-Use-correct-locking-protocol-during-GIN-posting-tree.patch where I propose
stampingevery deleted page with GinPageSetDeleteXid(page, ReadNewTransactionId()); and avoid reusing the page before
TransactionIdPrecedes(GinPageGetDeleteXid(page),RecentGlobalDataXmin). 
>> Should we leave alone this bug for future fixes to keep current fix noninvasive?
>
> I think since is separate bug introduced in PostgreSQL 9.4, which
> should be backpatched with separate commit.  Could you please extract
> patch dealing with GinPageSetDeleteXid() and GinPageGetDeleteXid().
> The rest of work made in your patch should be considered for master.

PFA this part. In thread about GiST vacuum Heikki was not very happy with reusing PageHeader->pd_prune_xid for this.
Butwe somewhat concluded that this might be OK. 
Anyway, there's whole page, we can store those bits anywhere.

> BTW, what do you think about locking order in ginRedoDeletePage()?  Do
> you have any explanation of current behavior?
It seems to me that locking order must be left->deleted->parent.

Best regards, Andrey Borodin.

> 10 дек. 2018 г., в 18:56, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
>
> PFA this part. In thread about GiST vacuum Heikki was not very happy with reusing PageHeader->pd_prune_xid for this.
Butwe somewhat concluded that this might be OK. 
> Anyway, there's whole page, we can store those bits anywhere.

Errrmm... I'm very sorry for the noise. I've forgot the actual patch.

Best regards, Andrey Borodin.

On Mon, Dec 10, 2018 at 4:58 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > 10 дек. 2018 г., в 18:56, Andrey Borodin <x4mmm@yandex-team.ru> написал(а):
> >
> > PFA this part. In thread about GiST vacuum Heikki was not very happy with reusing PageHeader->pd_prune_xid for
this.But we somewhat concluded that this might be OK. 
> > Anyway, there's whole page, we can store those bits anywhere.

We might allocate special field in opaque to store stat xid like
B-tree or SP-GiST.  But we have to be binary compatible.  And this
change shouldn't require upgrade procedure, since we're going to
backpatch that.  We could allocate space elsewhere on the page, but
that would be a bit cumbersome.  So, let's use pd_prune_xid once it's
available.

> Errrmm... I'm very sorry for the noise. I've forgot the actual patch.

Attached patch appears to be incomplete.  GinPageSetDeleteXid() is
called only in ginRedoDeletePage(), so only in recovery, while it
should be set during normal work too.  deleteXid field of
ginxlogDeletePage is never set.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sun, Dec 9, 2018 at 10:25 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Sat, Dec 8, 2018 at 12:48 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > > 8 дек. 2018 г., в 6:54, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
> > >
> > > Yep, please find attached draft patch.
> >
> > Patch seems good to me, I'll check it in more detail.
> > The patch gets posting item at FirstOffsetNumber instead of btree->getLeftMostChild(). This seem OK, since
dataGetLeftMostPage()is doing just the same, but with few Assert()s. 
>
> I'd like to evade creating GinBtree for just calling
> getLeftMostChild().  Also, few more places in ginvacuum.c do the same.
> We have the same amount of Assert()s in ginVacuumPostingTreeLeaves().
> So, let's keep it uniform.
>
> I would also like Peter Geoghegan to take a look at this patch before
> committing it.

I've slightly adjusted commit message.  I'm going to commit this fix
if no objections.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi!

> 11 дек. 2018 г., в 3:43, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
>
> Attached patch appears to be incomplete.  GinPageSetDeleteXid() is
> called only in ginRedoDeletePage(), so only in recovery, while it
> should be set during normal work too.  deleteXid field of
> ginxlogDeletePage is never set.

Sorry, I've messed it again. Forgot to include ginvacuum.c changes. Here it is.

Best regards, Andrey Borodin.

On Tue, Dec 11, 2018 at 10:14 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > 11 дек. 2018 г., в 3:43, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
> >
> > Attached patch appears to be incomplete.  GinPageSetDeleteXid() is
> > called only in ginRedoDeletePage(), so only in recovery, while it
> > should be set during normal work too.  deleteXid field of
> > ginxlogDeletePage is never set.
>
> Sorry, I've messed it again. Forgot to include ginvacuum.c changes. Here it is.

BTW, I still can't see you're setting deleteXid field of
ginxlogDeletePage struct anywhere.

Also, I note that protection against re-usage of recently deleted
pages is implemented differently than it is in B-tree.
1) You check TransactionIdPrecedes(GinPageGetDeleteXid(page),
RecentGlobalDataXmin)), while B-tree checks
TransactionIdPrecedes(opaque->btpo.xact, RecentGlobalXmin).  Could you
clarify why do we use RecentGlobalDataXmin instead of RecentGlobalXmin
here?
2) B-tree checks this condition both before putting page into FSM and
after getting page from FSM.  You're checking only after getting page
from FSM.  Approach of B-tree looks better for me.  It's seems more
consistent when FSM pages are really free for usage.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Tue, Dec 11, 2018 at 1:50 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Sun, Dec 9, 2018 at 10:25 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Sat, Dec 8, 2018 at 12:48 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > > > 8 дек. 2018 г., в 6:54, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
> > > >
> > > > Yep, please find attached draft patch.
> > >
> > > Patch seems good to me, I'll check it in more detail.
> > > The patch gets posting item at FirstOffsetNumber instead of btree->getLeftMostChild(). This seem OK, since
dataGetLeftMostPage()is doing just the same, but with few Assert()s. 
> >
> > I'd like to evade creating GinBtree for just calling
> > getLeftMostChild().  Also, few more places in ginvacuum.c do the same.
> > We have the same amount of Assert()s in ginVacuumPostingTreeLeaves().
> > So, let's keep it uniform.
> >
> > I would also like Peter Geoghegan to take a look at this patch before
> > committing it.
>
> I've slightly adjusted commit message.  I'm going to commit this fix
> if no objections.

Please also find patch changing lock order in ginRedoDeletePage()
preventing deadlock on standby.  I'm going to commit it too.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi!

> 12 дек. 2018 г., в 3:26, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
>
> BTW, I still can't see you're setting deleteXid field of
> ginxlogDeletePage struct anywhere.
Oops. Fixed.
>
> Also, I note that protection against re-usage of recently deleted
> pages is implemented differently than it is in B-tree.
> 1) You check TransactionIdPrecedes(GinPageGetDeleteXid(page),
> RecentGlobalDataXmin)), while B-tree checks
> TransactionIdPrecedes(opaque->btpo.xact, RecentGlobalXmin).  Could you
> clarify why do we use RecentGlobalDataXmin instead of RecentGlobalXmin
> here?
As far as I understand RecentGlobalDataXmin may be slightly farther than RecentGlobalXmin in case when
replication_slot_catalog_xminis holding RecentGlobalXmin. And GIN is never used by catalog tables. But let's use
RecentGlobalXminlike in B-tree. 

> 2) B-tree checks this condition both before putting page into FSM and
> after getting page from FSM.  You're checking only after getting page
> from FSM.  Approach of B-tree looks better for me.  It's seems more
> consistent when FSM pages are really free for usage.
Fixed.


Best regards, Andrey Borodin.

On Wed, Dec 12, 2018 at 4:08 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > 12 дек. 2018 г., в 3:26, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
> >
> > BTW, I still can't see you're setting deleteXid field of
> > ginxlogDeletePage struct anywhere.
> Oops. Fixed.
> >
> > Also, I note that protection against re-usage of recently deleted
> > pages is implemented differently than it is in B-tree.
> > 1) You check TransactionIdPrecedes(GinPageGetDeleteXid(page),
> > RecentGlobalDataXmin)), while B-tree checks
> > TransactionIdPrecedes(opaque->btpo.xact, RecentGlobalXmin).  Could you
> > clarify why do we use RecentGlobalDataXmin instead of RecentGlobalXmin
> > here?
> As far as I understand RecentGlobalDataXmin may be slightly farther than RecentGlobalXmin in case when
replication_slot_catalog_xminis holding RecentGlobalXmin. And GIN is never used by catalog tables. But let's use
RecentGlobalXminlike in B-tree. 
>
> > 2) B-tree checks this condition both before putting page into FSM and
> > after getting page from FSM.  You're checking only after getting page
> > from FSM.  Approach of B-tree looks better for me.  It's seems more
> > consistent when FSM pages are really free for usage.
> Fixed.

Thank you.  I've revised your patch and pushed it.  As long as two
other patches in this thread.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Dec 13, 2018 at 03:03:54PM +0300, Alexander Korotkov wrote:
> On Wed, Dec 12, 2018 at 4:08 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > > 12 дек. 2018 г., в 3:26, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
> > >
> > > BTW, I still can't see you're setting deleteXid field of
> > > ginxlogDeletePage struct anywhere.
> > Oops. Fixed.
> > >
> > > Also, I note that protection against re-usage of recently deleted
> > > pages is implemented differently than it is in B-tree.
> > > 1) You check TransactionIdPrecedes(GinPageGetDeleteXid(page),
> > > RecentGlobalDataXmin)), while B-tree checks
> > > TransactionIdPrecedes(opaque->btpo.xact, RecentGlobalXmin).  Could you
> > > clarify why do we use RecentGlobalDataXmin instead of RecentGlobalXmin
> > > here?
> > As far as I understand RecentGlobalDataXmin may be slightly farther than RecentGlobalXmin in case when
replication_slot_catalog_xminis holding RecentGlobalXmin. And GIN is never used by catalog tables. But let's use
RecentGlobalXminlike in B-tree.
 
> >
> > > 2) B-tree checks this condition both before putting page into FSM and
> > > after getting page from FSM.  You're checking only after getting page
> > > from FSM.  Approach of B-tree looks better for me.  It's seems more
> > > consistent when FSM pages are really free for usage.
> > Fixed.
> 
> Thank you.  I've revised your patch and pushed it.  As long as two
> other patches in this thread.

I am seeing this warning in the 9.4 branch:

    ginxlog.c:756:5: warning: ‘lbuffer’ may be used uninitialized
    in this function [-Wmaybe-uninitialized]

from this commit:

    commit 19cf52e6cc33b9e126775f28269ccccb6ddf7e30
    Author: Alexander Korotkov <akorotkov@postgresql.org>
    Date:   Thu Dec 13 06:12:25 2018 +0300

    Prevent deadlock in ginRedoDeletePage()

    On standby ginRedoDeletePage() can work concurrently with read-only queries.
    Those queries can traverse posting tree in two ways.
    1) Using rightlinks by ginStepRight(), which locks the next page before
       unlocking its left sibling.
    2) Using downlinks by ginFindLeafPage(), which locks at most one page at time.

    Original lock order was: page, parent, left sibling.  That lock order can
    deadlock with ginStepRight().  In order to prevent deadlock this commit changes
    lock order to: left sibling, page, parent.  Note, that position of parent in
    locking order seems insignificant, because we only lock one page at time while
    traversing downlinks.

    Reported-by: Chen Huajun
    Diagnosed-by: Chen Huajun, Peter Geoghegan, Andrey Borodin
    Discussion: https://postgr.es/m/31a702a.14dd.166c1366ac1.Coremail.chjischj%40163.com
    Author: Alexander Korotkov
    Backpatch-through: 9.4

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Bruce Momjian <bruce@momjian.us> writes:
> I am seeing this warning in the 9.4 branch:
>     ginxlog.c:756:5: warning: ‘lbuffer’ may be used uninitialized
>     in this function [-Wmaybe-uninitialized]

I'm also getting that, just in 9.4, but at a different line number:

ginxlog.c: In function 'ginRedoDeletePage':
ginxlog.c:693: warning: 'lbuffer' may be used uninitialized in this function

That's with gcc version 4.4.7 20120313 (Red Hat 4.4.7-23)

            regards, tom lane

Hi!


> 13 дек. 2018 г., в 17:03, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
>
> Thank you.  I've revised your patch and pushed it.  As long as two
> other patches in this thread.

That's great! Thanks!

> 13 дек. 2018 г., в 20:12, chjischj@163.com написал(а):
>
>
> hi
> I Have a question. Why the order of unlocking is not adjusted in this patch? like this:
>
> if (BufferIsValid(lbuffer))
> UnlockReleaseBuffer(lbuffer);
> if (BufferIsValid(pbuffer))
> UnlockReleaseBuffer(pbuffer);
> if (BufferIsValid(dbuffer))
> UnlockReleaseBuffer(dbuffer);
> ==>
> if (BufferIsValid(pbuffer))
> UnlockReleaseBuffer(pbuffer);
> if (BufferIsValid(dbuffer))
> UnlockReleaseBuffer(dbuffer);
> if (BufferIsValid(lbuffer))
> UnlockReleaseBuffer(lbuffer);

I think that unlock order does not matter. But I may be wrong. May be just for uniformity?


> 13 дек. 2018 г., в 21:48, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>
> Bruce Momjian <bruce@momjian.us> writes:
>> I am seeing this warning in the 9.4 branch:
>>     ginxlog.c:756:5: warning: ‘lbuffer’ may be used uninitialized
>>     in this function [-Wmaybe-uninitialized]
>
> I'm also getting that, just in 9.4, but at a different line number:
>
> ginxlog.c: In function 'ginRedoDeletePage':
> ginxlog.c:693: warning: 'lbuffer' may be used uninitialized in this function
>
> That's with gcc version 4.4.7 20120313 (Red Hat 4.4.7-23)


That's the same variable, one place is definition while other is potential misuse.
Seems like these 2 lines [0]

+    if (BufferIsValid(lbuffer))
+        UnlockReleaseBuffer(lbuffer);

are superfluous: lbuffer is UnlockReleased earlier.



Best regards, Andrey Borodin.

[0]
https://github.com/postgres/postgres/commit/19cf52e6cc33b9e126775f28269ccccb6ddf7e30#diff-ed6446a8993c76d2884ec6413e49a6b6R757

On Thu, Dec 13, 2018 at 8:06 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > if (BufferIsValid(lbuffer))
> > UnlockReleaseBuffer(lbuffer);
> > if (BufferIsValid(pbuffer))
> > UnlockReleaseBuffer(pbuffer);
> > if (BufferIsValid(dbuffer))
> > UnlockReleaseBuffer(dbuffer);
> > ==>
> > if (BufferIsValid(pbuffer))
> > UnlockReleaseBuffer(pbuffer);
> > if (BufferIsValid(dbuffer))
> > UnlockReleaseBuffer(dbuffer);
> > if (BufferIsValid(lbuffer))
> > UnlockReleaseBuffer(lbuffer);
>
> I think that unlock order does not matter. But I may be wrong. May be just for uniformity?

It doesn't mater, because we release all locks on every buffer at one
time.  The unlock order can have effect on what waiter will acquire
the lock next after ginRedoDeletePage().  However, I don't see why one
unlock order is better than another.  Thus, I just used the rule of
thumb to not change code when it's not necessary for bug fix.

> > 13 дек. 2018 г., в 21:48, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
> >
> > Bruce Momjian <bruce@momjian.us> writes:
> >> I am seeing this warning in the 9.4 branch:
> >>      ginxlog.c:756:5: warning: ‘lbuffer’ may be used uninitialized
> >>      in this function [-Wmaybe-uninitialized]
> >
> > I'm also getting that, just in 9.4, but at a different line number:
> >
> > ginxlog.c: In function 'ginRedoDeletePage':
> > ginxlog.c:693: warning: 'lbuffer' may be used uninitialized in this function
> >
> > That's with gcc version 4.4.7 20120313 (Red Hat 4.4.7-23)
>
>
> That's the same variable, one place is definition while other is potential misuse.
> Seems like these 2 lines [0]
>
> +       if (BufferIsValid(lbuffer))
> +               UnlockReleaseBuffer(lbuffer);
>
> are superfluous: lbuffer is UnlockReleased earlier.

Thanks to everybody for noticing!  Speaking more generally backpatch
to 9.4 was completely wrong: there were multiple errors.  It's my
oversight, I forget how much better our xlog system became since 9.4
:)

I've pushed bf0e5a73be fixing that.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi,

On 2018-12-13 22:40:59 +0300, Alexander Korotkov wrote:
> It doesn't mater, because we release all locks on every buffer at one
> time.  The unlock order can have effect on what waiter will acquire
> the lock next after ginRedoDeletePage().  However, I don't see why one
> unlock order is better than another.  Thus, I just used the rule of
> thumb to not change code when it's not necessary for bug fix.

I think it's right to not change unlock order at the same time as a
bugfix here.  More generally I think it can often be useful to default
to release locks in the inverse order they've been acquired - if there's
any likelihood that somebody will acquire them in the same order, that
ensures that such a party would only need to wait for a lock once,
instead of being woken up for one lock, and then immediately having to
wait for the next one.

Greetings,

Andres Freund

On Thu, Dec 13, 2018 at 10:46 PM Andres Freund <andres@anarazel.de> wrote:
> On 2018-12-13 22:40:59 +0300, Alexander Korotkov wrote:
> > It doesn't mater, because we release all locks on every buffer at one
> > time.  The unlock order can have effect on what waiter will acquire
> > the lock next after ginRedoDeletePage().  However, I don't see why one
> > unlock order is better than another.  Thus, I just used the rule of
> > thumb to not change code when it's not necessary for bug fix.
>
> I think it's right to not change unlock order at the same time as a
> bugfix here.  More generally I think it can often be useful to default
> to release locks in the inverse order they've been acquired - if there's
> any likelihood that somebody will acquire them in the same order, that
> ensures that such a party would only need to wait for a lock once,
> instead of being woken up for one lock, and then immediately having to
> wait for the next one.

Good point, thank you!

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Dec 13, 2018 at 10:40:59PM +0300, Alexander Korotkov wrote:
> On Thu, Dec 13, 2018 at 8:06 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> > That's the same variable, one place is definition while other is potential misuse.
> > Seems like these 2 lines [0]
> >
> > +       if (BufferIsValid(lbuffer))
> > +               UnlockReleaseBuffer(lbuffer);
> >
> > are superfluous: lbuffer is UnlockReleased earlier.
> 
> Thanks to everybody for noticing!  Speaking more generally backpatch
> to 9.4 was completely wrong: there were multiple errors.  It's my
> oversight, I forget how much better our xlog system became since 9.4
> :)
> 
> I've pushed bf0e5a73be fixing that.

I can confirm the compiler warning is now gone.  Thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Thu, Mar 21, 2019 at 9:26 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> On Thu, 13 Dec 2018 at 14:48, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>> On Thu, Dec 13, 2018 at 10:46 PM Andres Freund <andres@anarazel.de> wrote:
>> > On 2018-12-13 22:40:59 +0300, Alexander Korotkov wrote:
>> > > It doesn't mater, because we release all locks on every buffer at one
>> > > time.  The unlock order can have effect on what waiter will acquire
>> > > the lock next after ginRedoDeletePage().  However, I don't see why one
>> > > unlock order is better than another.  Thus, I just used the rule of
>> > > thumb to not change code when it's not necessary for bug fix.
>> >
>> > I think it's right to not change unlock order at the same time as a
>> > bugfix here.  More generally I think it can often be useful to default
>> > to release locks in the inverse order they've been acquired - if there's
>> > any likelihood that somebody will acquire them in the same order, that
>> > ensures that such a party would only need to wait for a lock once,
>> > instead of being woken up for one lock, and then immediately having to
>> > wait for the next one.
>>
>> Good point, thank you!
>
>
> It's been pointed out to me that 52ac6cd2d0cd70e01291e0ac4ee6d068b69bc478
> introduced a WAL incompatibility that has not been flagged.
>
> In ginRedoDeletePage() we use the struct directly to read the WAL record, so if a WAL record was written prior to
52ac6cd2d0cd70e01291e0ac4ee6d068b69bc478,yet read by code at 52ac6cd2d0cd70e01291e0ac4ee6d068b69bc478 or later then we
willhave problems, since deleteXid will not be set correctly. 
>
> It seems this should not have been backpatched.
>
> Please give your assessment.

Oh, right.  This is my fault.

However, I think this still can be backpatched correctly.  We can
determine whether xlog record data contains deleteXid by its size.
See the attached patch.  I haven't test this yet.  I'm going to test
it.  If OK, then push.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2019-Mar-21, Alexander Korotkov wrote:

> However, I think this still can be backpatched correctly.  We can
> determine whether xlog record data contains deleteXid by its size.
> See the attached patch.  I haven't test this yet.  I'm going to test
> it.  If OK, then push.

Wow, this is so magical that I think it merits a long comment explaining
what is going on.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Mar 22, 2019 at 12:06 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> On 2019-Mar-21, Alexander Korotkov wrote:
>
> > However, I think this still can be backpatched correctly.  We can
> > determine whether xlog record data contains deleteXid by its size.
> > See the attached patch.  I haven't test this yet.  I'm going to test
> > it.  If OK, then push.
>
> Wow, this is so magical that I think it merits a long comment explaining
> what is going on.

Yeah, have to fo magic to fix such weird things :)
Please, find next revision of patch attached.  It uses static
assertion to check that data structure size has changed.  Also,
assertion checks that record data length match on of structures
length.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Fri, Mar 22, 2019 at 11:05 AM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
> On Fri, Mar 22, 2019 at 12:06 AM Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > On 2019-Mar-21, Alexander Korotkov wrote:
> >
> > > However, I think this still can be backpatched correctly.  We can
> > > determine whether xlog record data contains deleteXid by its size.
> > > See the attached patch.  I haven't test this yet.  I'm going to test
> > > it.  If OK, then push.
> >
> > Wow, this is so magical that I think it merits a long comment explaining
> > what is going on.
>
> Yeah, have to fo magic to fix such weird things :)
> Please, find next revision of patch attached.  It uses static
> assertion to check that data structure size has changed.  Also,
> assertion checks that record data length match on of structures
> length.

I'm going to backpatch this on no objections.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sat, Mar 23, 2019 at 11:43 AM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
> On Fri, Mar 22, 2019 at 11:05 AM Alexander Korotkov
> <aekorotkov@gmail.com> wrote:
> > On Fri, Mar 22, 2019 at 12:06 AM Alvaro Herrera
> > <alvherre@2ndquadrant.com> wrote:
> > > On 2019-Mar-21, Alexander Korotkov wrote:
> > >
> > > > However, I think this still can be backpatched correctly.  We can
> > > > determine whether xlog record data contains deleteXid by its size.
> > > > See the attached patch.  I haven't test this yet.  I'm going to test
> > > > it.  If OK, then push.
> > >
> > > Wow, this is so magical that I think it merits a long comment explaining
> > > what is going on.
> >
> > Yeah, have to fo magic to fix such weird things :)
> > Please, find next revision of patch attached.  It uses static
> > assertion to check that data structure size has changed.  Also,
> > assertion checks that record data length match on of structures
> > length.
>
> I'm going to backpatch this on no objections.

So, pushed.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company