Re: GIN pending list clean up exposure to SQL

On Sat, Jan 16, 2016 at 7:42 AM, Julien Rouhaud
<julien(dot)rouhaud(at)dalibo(dot)com> wrote:
> On 15/01/2016 22:59, Jeff Janes wrote:
>> On Sun, Jan 10, 2016 at 4:24 AM, Julien Rouhaud
>> <julien(dot)rouhaud(at)dalibo(dot)com> wrote:
>>> On 29/12/2015 00:30, Jeff Janes wrote:
>>>> On Wed, Nov 25, 2015 at 9:29 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>>>>>
>>>>> I'll prepare a patch for core for the January commitfest, and see if
>>>>> it flies. If not, there is always the extension to fall back to.
>>>>
>>>> Here is an updated patch. I've added type and permission checks,
>>>> docs, and some regression tests.
>>>>
>>>
>>> I just reviewed it. Patch applies cleanly, everything works as intended,
>>> including regression tests.
>>>
>>> I think the function should be declared as strict.
>>
>> OK. I see that brin_summarize_new_values, which I modeled this on,
>> was recently changed to be strict. So I've changed this the same way.
>>
>>>
>>> Also, there are some trailing whitespaces in the documentation diff.
>>
>> Fixed.
>
> Thanks!
>
>> I also added the DESC to the pg_proc entry, which I somehow
>> missed before.
>>
>
> Good catch, I also missed it.
>
> 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.

ginfast.c: In function 'gin_clean_pending_list':
ginfast.c:980: error: 'GIN_AM_OID' undeclared (first use in this function)
ginfast.c:980: error: (Each undeclared identifier is reported only once
ginfast.c:980: error: for each function it appears in.)

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.

+ 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?

Regards,

--
Fujii Masao