Em seg., 22 de jun. de 2020 às 16:33, Robert Haas <[email protected]>
escreveu:
> Ranier,
>
> This topic is largely unrelated to the current thread. Also...
>
Weel, I was trying to improve the patch for the current thread.
Or perhaps, you are referring to something else, which I may not have
understood.
>
> On Mon, Jun 22, 2020 at 12:47 PM Ranier Vilela <[email protected]>
> wrote:
> > Questions:
> > 1. Why acquire and release lock in retry: loop.
>
> This is a super-bad idea. Note the coding rule mentioned in spin.h.
> There are many discussion on this mailing list about the importance of
> keeping the critical section for a spinlock to a few instructions.
> Calling another function that *itself acquires an LWLock* is
> definitely not OK.
>
Perhaps, I was not clear and it is another misunderstanding.
I am not suggesting a function to acquire the lock.
By the way, I did the tests with this change and it worked perfectly.
But, as it is someone else's patch, I asked why to learn.
By the way, my suggestion is with less instructions than the patch.
The only change I asked is why to acquire and release the lock repeatedly
within the goto retry, when you already have it.
If I can acquire the lock before retry: and release it only at the end when
I leave table_block_parallelscan_startblock_init,
why not do it.
I will attach the suggested excerpt so that I have no doubts about what I
am asking.
regards,
Ranier Vilela
void
table_block_parallelscan_startblock_init(Relation rel,
ParallelBlockTableScanWork workspace,
ParallelBlockTableScanDesc pbscan)
{
BlockNumber sync_startpage = InvalidBlockNumber;
/* Reset the state we use for controlling allocation size. */
memset(workspace, 0, sizeof(*workspace));
StaticAssertStmt(MaxBlockNumber <= 0xFFFFFFFE,
"pg_nextpower2_32 may be too small for
non-standard BlockNumber width");
workspace->phsw_chunk_size = pg_nextpower2_32(Max(pbscan->phs_nblocks /
PARALLEL_SEQSCAN_NCHUNKS, 1));
/* Ensure we don't go over the maximum size with larger rels */
workspace->phsw_chunk_size = Min(workspace->phsw_chunk_size,
PARALLEL_SEQSCAN_MAX_CHUNK_SIZE);
/* Grab the spinlock. */
SpinLockAcquire(&pbscan->phs_mutex);
retry:
/*
* If the scan's startblock has not yet been initialized, we must do so
* now. If this is not a synchronized scan, we just start at block 0,
but
* if it is a synchronized scan, we must get the starting position from
* the synchronized scan machinery. We can't hold the spinlock while
* doing that, though, so release the spinlock, get the information we
* need, and retry. If nobody else has initialized the scan in the
* meantime, we'll fill in the value we fetched on the second time
* through.
*/
if (pbscan->phs_startblock == InvalidBlockNumber)
{
if (!pbscan->base.phs_syncscan)
pbscan->phs_startblock = 0;
else if (sync_startpage != InvalidBlockNumber)
pbscan->phs_startblock = sync_startpage;
else
{
sync_startpage = ss_get_location(rel,
pbscan->phs_nblocks);
goto retry;
}
}
SpinLockRelease(&pbscan->phs_mutex);
}
