Обсуждение: [HACKERS] Index created in BEFORE trigger not updated during INSERT
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;$$; CREATE TRIGGER makeindex BEFORE INSERT ON mytable FOR EACH ROW EXECUTE PROCEDURE makeindex(); INSERT INTO mytable VALUES (1, 42); SELECT * FROM mytable; ┌────┬─────┐ │ id │ id2 │ ├────┼─────┤ │ 1 │ 42 │ └────┴─────┘ (1 row) But 42 is not present in the index: SET enable_seqscan = off; SELECT * FROM mytable WHERE id2 = 42; ┌────┬─────┐ │ id │ id2 │ ├────┼─────┤ └────┴─────┘ (0 rows)
On Mon, May 22, 2017 at 7:05 AM, Albe Laurenz <laurenz.albe@wien.gv.at> 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
On 2017-05-24 08:26:24 -0400, Robert Haas wrote: > On Mon, May 22, 2017 at 7:05 AM, Albe Laurenz <laurenz.albe@wien.gv.at> 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
Andres Freund <andres@anarazel.de> 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
I wrote:
> Andres Freund <andres@anarazel.de> 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, /* skip_build */
false); /* quiet */
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 79f3be3..5dd14b4 100644
*** a/src/include/commands/defrem.h
--- b/src/include/commands/defrem.h
*************** extern ObjectAddress DefineIndex(Oid rel
*** 27,32 ****
--- 27,33 ----
Oid indexRelationId,
bool is_alter_table,
bool check_rights,
+ bool check_not_in_use,
bool skip_build,
bool quiet);
extern Oid ReindexIndex(RangeVar *indexRelation, int options);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Jun 4, 2017 at 7:23 AM, Tom Lane <tgl@sss.pgh.pa.us> 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
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
Michael Paquier <michael.paquier@gmail.com> 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
Tom Lane wrote: >> Andres Freund <andres@anarazel.de> 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