Horton

Thread

Commits

  1. Allow definition of lock mode for custom reloptions

  2. Fix failure with lock mode used for custom relation options

  1. Custom reloptions and lock modes

    Michael Paquier <michael@paquier.xyz> — 2019-09-20T01:38:31Z

    Hi all,

This is a new thread related to the bug analyzed here:
https://www.postgresql.org/message-id/20190919083203.GC21144@paquier.xyz

And in short, if you attempt to do an ALTER TABLE with a custom
reloptions the command burns itself, like that for example this
sequence:
create extension bloom;
create table aa (a int);
create index aa_bloom ON aa USING bloom (a);
alter index aa_bloom set (length = 20);

Which results in the following error:
ERROR:  XX000: unrecognized lock mode: 2139062143
LOCATION:  LockAcquireExtended, lock.c:756

The root of the problem is that the set of relation options loaded
finds properly the custom options set when looking for the lock mode
to use in AlterTableGetRelOptionsLockLevel(), but we never set the
lock mode this option should use when allocating it, resulting in a
failure.  The current set of APIs does not allow either to set the
lock mode associated with a custom reloption.

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.
- 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.

My take would be to use 0001 on all branches (or I am missing
something related to custom relopts manipulation?), and consider 0002
on HEAD.

Thoughts?
--
Michael

  2. Re: Custom reloptions and lock modes

    Michael Paquier <michael@paquier.xyz> — 2019-09-20T01:44:45Z

    On Fri, Sep 20, 2019 at 10:38:31AM +0900, Michael Paquier wrote:
> Hi all,
> 
> This is a new thread related to the bug analyzed here:
> https://www.postgresql.org/message-id/20190919083203.GC21144@paquier.xyz
> 
> And in short, if you attempt to do an ALTER TABLE with a custom
> reloptions the command burns itself, like that for example this
> sequence:
> create extension bloom;
> create table aa (a int);
> create index aa_bloom ON aa USING bloom (a);
> alter index aa_bloom set (length = 20);
> 
> Which results in the following error:
> - 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 mean a back-patch down to v12 for this part, but not further down.
Sorry for the possible confusion.
--
Michael

  3. Re: Custom reloptions and lock modes

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2019-09-20T06:29:13Z

    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.

> - 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.

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?

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  4. Re: Custom reloptions and lock modes

    Michael Paquier <michael@paquier.xyz> — 2019-09-20T07:08:52Z

    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

  5. Re: Custom reloptions and lock modes

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2019-09-20T07:10:51Z

    On Fri, Sep 20, 2019 at 12:38 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> > 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.
Okay. Sounds good.



-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  6. Re: Custom reloptions and lock modes

    Michael Paquier <michael@paquier.xyz> — 2019-09-24T02:33:35Z

    On Fri, Sep 20, 2019 at 12:40:51PM +0530, Kuntal Ghosh wrote:
> Okay. Sounds good.

Thanks for the review.  Attached is the patch set I am planning to
commit.  I'll wait after the tag of this week as the first patch needs
to go down to 9.6, the origin of the bug being 47167b7.  The second
patch would go only to HEAD, as discussed.
--
Michael

  7. Re: Custom reloptions and lock modes

    Michael Paquier <michael@paquier.xyz> — 2019-09-25T01:23:43Z

    On Tue, Sep 24, 2019 at 11:33:35AM +0900, Michael Paquier wrote:
> Thanks for the review.  Attached is the patch set I am planning to
> commit.  I'll wait after the tag of this week as the first patch needs
> to go down to 9.6, the origin of the bug being 47167b7.  The second
> patch would go only to HEAD, as discussed.

And applied both.
--
Michael