Обсуждение: Custom reloptions and lock modes
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
Вложения
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
Вложения
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
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
Вложения
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
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
Вложения
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