Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT
Tom Lane wrote: >> Andres Freund writes: >>> Hm, strategically sprinkled CheckTableNotInUse() might do the trick? > > Attached is a proposed patch that closes off this problem. I've tested > it to the extent that it blocks Albe's example and passes check-world. I tested it, and it works fine. Thanks! > I'm unsure whether to back-patch or not; the main argument for not doing > so is that if any extensions are calling DefineIndex() directly, this > would be an API break for them. Given what a weird case this is, I'm not > sure it's worth that. > > A possible end-run around the API objection would be to not add an extra > argument to DefineIndex() in the back branches, but to use !is_alter_table > as the control condition. That would work for the core callers, although > we might need a special case for bootstrap mode. On the other hand, > thinking again about hypothetical third-party callers, it's possible that > that's not the right answer for them, in which case they'd be really in > trouble. So I'm not that much in love with that answer. It causes a slight bellyache to leave an unfixed data corruption bug in the back branches (if only index corruption), but I agree that it is such a weird case to create an index in a BEFORE trigger that we probably can live without a back-patch. Yours, Laurenz Albe -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT
Michael Paquier writes: > The patch looks good to me, could you add a regression test? Done, thanks for the review. I stuck the test into triggers.sql, which is not completely on point since there are other ways to get to this error. But if we're thinking of it as a framework for testing other CheckTableNotInUse cases then adding it to e.g. create_index.sql doesn't seem right either. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT
On 2017-06-03 18:23:33 -0400, Tom Lane wrote: > Attached is a proposed patch that closes off this problem. I've tested > it to the extent that it blocks Albe's example and passes check-world. Cool. > I'm unsure whether to back-patch or not; the main argument for not doing > so is that if any extensions are calling DefineIndex() directly, this > would be an API break for them. Given what a weird case this is, I'm not > sure it's worth that. I slightly lean against backpatching, it has taken long to be reported, and it's pretty easy to work around... - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT
On Sun, Jun 4, 2017 at 7:23 AM, Tom Lane wrote: > I'm unsure whether to back-patch or not; the main argument for not doing > so is that if any extensions are calling DefineIndex() directly, this > would be an API break for them. Given what a weird case this is, I'm not > sure it's worth that. Knowing that it has taken 20 years to get a report for this problem, It seems to me that one report does not win against the risk of destabilizing extensions that would surprisingly break after a minor update. > A possible end-run around the API objection would be to not add an extra > argument to DefineIndex() in the back branches, but to use !is_alter_table > as the control condition. That would work for the core callers, although > we might need a special case for bootstrap mode. On the other hand, > thinking again about hypothetical third-party callers, it's possible that > that's not the right answer for them, in which case they'd be really in > trouble. So I'm not that much in love with that answer. Yes, that's not worth the trouble I think for back-branches. The patch looks good to me, could you add a regression test? This could be used later on as a basis for other DDL commands if somebody looks at this problem for the other ones. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT
I wrote: > Andres Freund writes: >> Hm, strategically sprinkled CheckTableNotInUse() might do the trick? > +1. We can't reasonably make it work: the outer query already has its > list of indexes that need to be inserted into. Also, if you try to > make the index via ALTER TABLE ADD CONSTRAINT in the trigger, that will > give you "cannot ALTER TABLE "mytable" because it is being used by active > queries in this session" because of the check in AlterTable(). Attached is a proposed patch that closes off this problem. I've tested it to the extent that it blocks Albe's example and passes check-world. I'm unsure whether to back-patch or not; the main argument for not doing so is that if any extensions are calling DefineIndex() directly, this would be an API break for them. Given what a weird case this is, I'm not sure it's worth that. A possible end-run around the API objection would be to not add an extra argument to DefineIndex() in the back branches, but to use !is_alter_table as the control condition. That would work for the core callers, although we might need a special case for bootstrap mode. On the other hand, thinking again about hypothetical third-party callers, it's possible that that's not the right answer for them, in which case they'd be really in trouble. So I'm not that much in love with that answer. > It doesn't seem terribly hard to fix the CREATE INDEX case to behave > likewise, but I wonder how many other cases we've missed? That remains an open question :-( regards, tom lane diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y index de3695c..2e1fef0 100644 *** a/src/backend/bootstrap/bootparse.y --- b/src/backend/bootstrap/bootparse.y *** Boot_DeclareIndexStmt: *** 323,328 --- 323,329 $4, false, false, + false, true, /* skip_build */ false); do_end(); *** Boot_DeclareUniqueIndexStmt: *** 366,371 --- 367,373 $5, false, false, + false, true, /* skip_build */ false); do_end(); diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 4861799..c090279 100644 *** a/src/backend/commands/indexcmds.c --- b/src/backend/commands/indexcmds.c *** CheckIndexCompatible(Oid oldId, *** 295,300 --- 295,303 * 'is_alter_table': this is due to an ALTER rather than a CREATE operation. * 'check_rights': check for CREATE rights in namespace and tablespace. (This * should be true except when ALTER is deleting/recreating an index.) + * 'check_not_in_use': check for table not already in use in current session. + * This should be true unless caller is holding the table open, in which + * case the caller had better have checked it earlier. * 'skip_build': make the catalog entries but leave the index file empty; * it will be filled later. * 'quiet': suppress the NOTICE chatter ordinarily provided for constraints. *** DefineIndex(Oid relationId, *** 307,312 --- 310,316 Oid indexRelationId, bool is_alter_table, bool check_rights, + bool check_not_in_use, bool skip_build, bool quiet) { *** DefineIndex(Oid relationId, *** 405,410 --- 409,423 errmsg("cannot create indexes on temporary tables of other sessions"))); /* + * Unless our caller vouches for having checked this already, insist that + * the table not be in use by our own session, either. Otherwise we might + * fail to make entries in the new index (for instance, if an INSERT or + * UPDATE is in progress and has already made its list of target indexes). + */ + if (check_not_in_use) + CheckTableNotInUse(rel, "CREATE INDEX"); + + /* * Verify we (still) have CREATE rights in the rel's namespace. * (Presumably we did when the rel was created, but maybe not anymore.) * Skip check if caller doesn't want it. Also skip check if diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7959120..b61fda9 100644 *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** ATExecAddIndex(AlteredTableInfo *tab, Re *** 6679,6684 --- 6679,6685 InvalidOid, /* no predefined OID */ true, /* is_alter_table */ check_rights, + false, /* check_not_in_use - we did it already */ skip_build, quiet); diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 1e941fb..a22fd53 100644 *** a/src/backend/tcop/utility.c --- b/src/backend/tcop/utility.c *** ProcessUtilitySlow(ParseState *pstate, *** 1329,1334 --- 1329,1335 InvalidOid, /* no predefined OID */ false, /* is_alter_table */ true, /* check_rights */ + true, /* check_not_in_use */ false, /
Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT
Andres Freund writes: > On 2017-05-24 08:26:24 -0400, Robert Haas wrote: >> I'm willing to bet that nobody ever thought about that case very hard. >> It seems like we should either make it work or prohibit it, but I >> can't actually see quite how to do either off-hand. > Hm, strategically sprinkled CheckTableNotInUse() might do the trick? +1. We can't reasonably make it work: the outer query already has its list of indexes that need to be inserted into. Also, if you try to make the index via ALTER TABLE ADD CONSTRAINT in the trigger, that will give you "cannot ALTER TABLE "mytable" because it is being used by active queries in this session" because of the check in AlterTable(). It doesn't seem terribly hard to fix the CREATE INDEX case to behave likewise, but I wonder how many other cases we've missed? One small issue is that naive use of CheckTableNotInUse would produce an error reading "cannot CREATE INDEX "mytable" because ...", which is not exactly on point. It'd be better for it to say "cannot create an index on "mytable" because ...". However, given that it took twenty years for anybody to notice this, maybe it's close enough. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT
On 2017-05-24 08:26:24 -0400, Robert Haas wrote: > On Mon, May 22, 2017 at 7:05 AM, Albe Laurenz wrote: > > Not that it is a useful use case, but I believe that this is > > a bug that causes index corruption: > > > > CREATE TABLE mytable( > >id integer PRIMARY KEY, > >id2 integer NOT NULL > > ); > > > > CREATE FUNCTION makeindex() RETURNS trigger > >LANGUAGE plpgsql AS > > $$BEGIN > >CREATE INDEX ON mytable(id2); > >RETURN NEW; > > END;$$; > > I'm willing to bet that nobody ever thought about that case very hard. > It seems like we should either make it work or prohibit it, but I > can't actually see quite how to do either off-hand. Hm, strategically sprinkled CheckTableNotInUse() might do the trick? I've neither tried nor thought this through fully, but I can't think of a case where pre-existing relcache references to tables are ok... - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT
On Mon, May 22, 2017 at 7:05 AM, Albe Laurenz wrote: > Not that it is a useful use case, but I believe that this is > a bug that causes index corruption: > > CREATE TABLE mytable( >id integer PRIMARY KEY, >id2 integer NOT NULL > ); > > CREATE FUNCTION makeindex() RETURNS trigger >LANGUAGE plpgsql AS > $$BEGIN >CREATE INDEX ON mytable(id2); >RETURN NEW; > END;$$; I'm willing to bet that nobody ever thought about that case very hard. It seems like we should either make it work or prohibit it, but I can't actually see quite how to do either off-hand. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers