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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
block-create-index-from-a-trigger.patchtext/x-diff; charset=us-ascii; name=block-create-index-from-a-trigger.patchDownload+18-0
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers