[HACKERS] ginInsertCleanup called from vacuum could still miss tuples to be deleted - Mailing list pgsql-hackers

From Masahiko Sawada
Subject [HACKERS] ginInsertCleanup called from vacuum could still miss tuples to be deleted
Date
Msg-id CAD21AoBLUSyiYKnTYtSAbC+F=XDjiaBrOUEGK+zUXdQ8owfPKw@mail.gmail.com
Whole thread Raw
Responses Re: [HACKERS] ginInsertCleanup called from vacuum could still misstuples to be deleted  (Peter Geoghegan <pg@bowt.ie>)
Re: [HACKERS] ginInsertCleanup called from vacuum could still misstuples to be deleted  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers
Hi,

Commit e2c79e14 prevented multiple cleanup process for pending list in
GIN index. But I think that there is still possibility that vacuum
could miss tuples to be deleted if someone else is cleaning up the
pending list.

In ginInsertCleanup(), we lock the GIN meta page by LockPage and could
wait for the concurrent cleaning up process if stats == NULL. And the
source code comment says that this happen is when ginINsertCleanup is
called by [auto]vacuum/analyze or gin_clean_pending_list(). I agree
with this behavior. However, looking at the callers the stats is NULL
only either if pending list exceeds to threshold during insertions or
if only analyzing is performed by an autovacum worker or ANALYZE
command. So I think we should inVacuum = (stats != NULL) instead.
Also, we might want autoanalyze and ANALYZE command to wait for
concurrent process as well. Attached patch fixes these two issue. If
this is a bug we should back-patch to 9.6.

void
ginInsertCleanup(GinState *ginstate, bool full_clean,
                 bool fill_fsm, IndexBulkDeleteResult *stats)
{

(snip)

    bool        inVacuum = (stats == NULL);

    /*
     * We would like to prevent concurrent cleanup process. For that we will
     * lock metapage in exclusive mode using LockPage() call. Nobody other
     * will use that lock for metapage, so we keep possibility of concurrent
     * insertion into pending list
     */

    if (inVacuum)
    {
        /*
         * We are called from [auto]vacuum/analyze or gin_clean_pending_list()
         * and we would like to wait concurrent cleanup to finish.
         */
        LockPage(index, GIN_METAPAGE_BLKNO, ExclusiveLock);
        workMemory =
            (IsAutoVacuumWorkerProcess() && autovacuum_work_mem != -1) ?
            autovacuum_work_mem : maintenance_work_mem;
    }
    else
    {
        /*
         * We are called from regular insert and if we see concurrent cleanup
         * just exit in hope that concurrent process will clean up pending
         * list.
         */
        if (!ConditionalLockPage(index, GIN_METAPAGE_BLKNO, ExclusiveLock))
            return;
        workMemory = work_mem;
    }

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

pgsql-hackers by date:

Previous
From: Noah Misch
Date:
Subject: Re: [HACKERS] Building PL/Perl with ActiveState Perl 5.22 and MSVC
Next
From: Alexander Kuzmenkov
Date:
Subject: Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans