On Fri, Sep 20, 2019 at 11:59:13AM +0530, Kuntal Ghosh wrote:
> On Fri, Sep 20, 2019 at 7:08 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Hence attached is a patch set to address those issues:
>> - 0001 makes sure that any existing module creating a custom reloption
>> has the lock mode set to AccessExclusiveMode, which would be a sane
>> default anyway. I think that we should just back-patch that and close
>> any holes.
>
> Looks good to me. The patch solves the issue and passes with
> regression tests. IMHO, it should be back-patched to all the branches.
That's the plan but...
>> - 0002 is a patch which we could use to extend the existing reloption
>> APIs to set the lock mode used. I am aware of the recent work done by
>> Nikolay in CC to rework this API set, but I am unsure where we are
>> going there, and the resulting patch is actually very short, being
>> 20-line long with the current infrastructure. That could go into
>> HEAD. Table AMs have been added in v12 so custom reloptions could
>> gain more in popularity, but as we are very close to the release it
>> would not be cool to break those APIs. The patch simplicity could
>> also be a reason sufficient for a back-patch, and I don't think that
>> there are many users of them yet.
>>
>
> I think this is good approach for now and can be committed on the
> HEAD only.
Let's wait a couple of days to see if others have any objections to
offer on the matter. My plan would be to revisit this patch set after
RC1 is tagged next week to at least fix the bug. I don't predict any
strong objections to the patch for HEAD, but who knows..
> One small thing:
>
> add_int_reloption(bl_relopt_kind, buf,
> "Number of bits generated for each index column",
> - DEFAULT_BLOOM_BITS, 1, MAX_BLOOM_BITS);
> + DEFAULT_BLOOM_BITS, 1, MAX_BLOOM_BITS,
> + AccessExclusiveLock);
> Do we need a comment to explain why we're using AccessExclusiveLock in
> this case?
Because that's the safest default to use here? That seemed obvious to
me.
--
Michael