Re: Re: Re: Cache relation sizes?

On Wed, Dec 30, 2020 at 4:13 AM 陈佳昕(步真) <buzhen(dot)cjx(at)alibaba-inc(dot)com> wrote:
> I found some other problems which I want to share my change with you to make you confirm.
> <1> I changed the code in smgr_alloc_sr to avoid dead lock.
>
> LWLockAcquire(mapping_lock, LW_EXCLUSIVE);
> flags = smgr_lock_sr(sr);
> Assert(flags & SR_SYNCING(forknum));
> + flags &= ~SR_SYNCING(forknum);
> if (flags & SR_JUST_DIRTIED(forknum))
> {
> /*
> * Someone else dirtied it while we were syncing, so we can't mark
> * it clean. Let's give up on this SR and go around again.
> */
> smgr_unlock_sr(sr, flags);
> LWLockRelease(mapping_lock);
> goto retry;
> }
> /* This fork is clean! */
> - flags &= ~SR_SYNCING(forknum);
> flags &= ~SR_DIRTY(forknum);
> }
>
> In smgr_drop_sr, if the sr is SR_SYNCING, it will retry until the sr is not SR_SYNCING. But in smgr_alloc_sr, if the sr is SR_JUST_DIRTIED, it will retry to get another sr, although it has been synced by smgrimmedsync, the flag SR_SYNCING doesn't changed. This might cause dead lock. So I changed your patch as above.

Right. Thanks!

I also added smgrdropdb() to handle DROP DATABASE (the problem you
reported in your previous email).

While tweaking that code, I fixed it so that it uses a condition
variable to wait (instead of the silly sleep loop) when it needs to
drop an SR that is being sync'd. Also, it now releases the mapping
lock while it's doing that, and requires it on retry.

> <2> I changed the code in smgr_drop_sr to avoid some corner cases
> /* Mark it invalid and drop the mapping. */
> smgr_unlock_sr(sr, ~SR_VALID);
> + for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
> + sr->nblocks[forknum] = InvalidBlockNumber;
> hash_search_with_hash_value(sr_mapping_table,
> &reln->smgr_rnode,
> hash,
> HASH_REMOVE,
> NULL);
>
> smgr_drop_sr just removes the hash entry from sr_mapping_table, but doesn't remove the sr_pool. But in smgrnblocks_fast, it just get sr from sr_pool, so I add some codes as above to avoid some corner cases get an unexpected result from smgrnblocks_fast. Is it necessary, I also want some advice from you.

Hmm. I think it might be better to increment sr->generation. That
was already done in the "eviction" code path, where other processes
might still have references to the SR object, and I don't think it's
possible for anyone to access a dropped relation, but I suppose it's
more consistent to do that anyway. Fixed.

Thanks for the review!