Re: GIN pending list clean up exposure to SQL

On Wed, Jan 20, 2016 at 6:17 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Sat, Jan 16, 2016 at 7:42 AM, Julien Rouhaud
> <julien.rouhaud@dalibo.com> wrote:
>> On 15/01/2016 22:59, Jeff Janes wrote:
>>> On Sun, Jan 10, 2016 at 4:24 AM, Julien Rouhaud
>>> <julien.rouhaud@dalibo.com> wrote:
>>
>> All looks fine to me, I flag it as ready for committer.
>
> When I compiled the PostgreSQL with the patch, I got the following error.
> ISTM that the inclusion of pg_am.h header file is missing in ginfast.c.

Thanks.  Fixed.

> gin_clean_pending_list() must check whether the server is in recovery or not.
> If it's in recovery, the function must exit with an error. That is, IMO,
> something like the following check must be added.
>
>          if (RecoveryInProgress())
>                  ereport(ERROR,
>
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>                                   errmsg("recovery is in progress"),
>                                   errhint("GIN pending list cannot be
> cleaned up during recovery.")));
>
> It's better to make gin_clean_pending_list() check whether the target index
> is temporary index of other sessions or not, like pgstatginindex() does.

I've added both of these checks.  Sorry I overlooked your early email
in this thread about those.

>
> +    Relation    indexRel = index_open(indexoid, AccessShareLock);
>
> ISTM that AccessShareLock is not safe when updating the pending list and
> GIN index main structure. Probaby we should use RowExclusiveLock?

Other callers of the ginInsertCleanup function also do so while
holding only the AccessShareLock on the index.  It turns out that
there is a bug around this, as discussed in "Potential GIN vacuum bug"
(http://www.postgresql.org/message-id/flat/CAMkU=1xALfLhUUohFP5v33RzedLVb5aknNUjcEuM9KNBKrB6-Q@mail.gmail.com)

But, that bug has to be solved at a deeper level than this patch.

I've also cleaned up some other conflicts, and chose a more suitable
OID for the new catalog function.

The number of new header includes needed to implement this makes me
wonder if I put this code in the correct file, but I don't see a
better location for it.

New version attached.

Thanks,

Jeff