Re: [HACKERS] Disallow unique index on system columns
On Wed, Apr 20, 2016 at 9:24 PM Tom Lane wrote:
> > SELECT FROM table WHERE my_func('table', ctid) ==> 'index condition'
>
> Um, why's the ctid important here, or perhaps more directly, what is
> it you're really trying to do?
>
This function is defined as my_func(regclass, tid) and simply returns the
tid value passed in. The operator is defined as ==>(tid, text).
Behind the scenes, the AM is actually backed by Elasticsearch, and the
tuple's ctid value is used as the "_id" in ES.
When Postgres decides to plan a sequential scan (or filter) to answer WHERE
clause conditions that use the ==>(tid, text) operator the AM isn't
involved but I still need to use the remote Elasticsearch server to answer
that condition.
So I came up with this "creative" approach to provide enough context in the
query plan for me to figure out a) which table is being used and b) which
physical row is being evaluated in the seqscan or filter.
When the operator's procedure is called, I notice that it's the first time
I've seen the FuncExpr on the LHS, go query Elasticsearch with the text
query from the RHS, build a hashtable of the matching ctids and lookup the
LHS's value in the hashtable. If it exists, the row matches.
There just didn't seem to be enough context in the FunctionCallInfo of the
the operator's procedure to figure this out without something in the query
that's basically statically determined at parse time.
I suspect what I should be doing for this particular problem is taking
advantage of the Custom Scan API, but I'm trying to support PG 9.3.
We weren't planning to do that.
>
Great!
eric
Re: [HACKERS] Disallow unique index on system columns
Eric Ridge writes:
> I've got an extension that's actually a custom Access Method, and for
> reasons that are probably too boring to go into here, it requires that the
> first column in the index be a function that takes the ctid. Ie, something
> akin to:
>CREATE INDEX idx ON table (my_func('table', ctid), other_func(table));
That's ... creative.
> The AM implementation itself doesn't actually use the result of my_func(),
> but that construct is necessary so I can detect certain queries that look
> like:
> SELECT FROM table WHERE my_func('table', ctid) ==> 'index condition'
Um, why's the ctid important here, or perhaps more directly, what is
it you're really trying to do?
> I don't mind that you're changing this for 9.6... 9.6 is going to change so
> much other stuff around custom AMs that I'll deal with it when the time
> comes, but back-patching this into 9.3/4/5 would make life very difficult.
We weren't planning to do that.
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] Disallow unique index on system columns
On Sat, Apr 16, 2016 at 12:14 PM Tom Lane wrote:
>
> Pushed. I moved the check into DefineIndex, as that's where user-facing
> complaints about indexes generally ought to be.
>
If you're planning on back-patching this, please don't. :) It'll
literally ruin my life.
I've got an extension that's actually a custom Access Method, and for
reasons that are probably too boring to go into here, it requires that the
first column in the index be a function that takes the ctid. Ie, something
akin to:
CREATE INDEX idx ON table (my_func('table', ctid), other_func(table));
The AM implementation itself doesn't actually use the result of my_func(),
but that construct is necessary so I can detect certain queries that look
like:
SELECT FROM table WHERE my_func('table', ctid) ==> 'index condition'
I don't mind that you're changing this for 9.6... 9.6 is going to change so
much other stuff around custom AMs that I'll deal with it when the time
comes, but back-patching this into 9.3/4/5 would make life very difficult.
Thanks for listening!
eric
Re: [HACKERS] Disallow unique index on system columns
David Rowley writes: > On 15 April 2016 at 13:43, David Rowley wrote: >> The attached patch just disallows any index containing a system >> column, apart from OID. > Seems I only did half the job as I forgot to think to check for system > columns that are hidden away inside expressions or predicates. > The attached fixes that. Pushed. I moved the check into DefineIndex, as that's where user-facing complaints about indexes generally ought to be. 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] Disallow unique index on system columns
Andres Freund writes: > On 2016-04-15 11:49:27 -0400, Tom Lane wrote: >> I think what we should do with this is split it into two patches, one >> to fix ALTER REPLICA IDENTITY's crash (which we'd back-patch to 9.4) >> and one to forbid indexes on system columns (which IMO should be HEAD >> only). The first of those should be pretty uncontroversial, so I'll >> go ahead with pushing it --- anyone have a problem with the second? > Only in that I'm wondering whether we shouldn't be more aggressive about > #2. Well, if there *is* someone out there somehow making use of an index on one of these columns, I think we shouldn't break it in a minor release. A more likely scenario is that someone's created such an index but it's lying around unused. In that case, with the patch as proposed, they'd have to drop it before they could upgrade to 9.6. That doesn't bother me ... but again, possibly causing problems in a minor release does. 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] Disallow unique index on system columns
On 2016-04-15 11:49:27 -0400, Tom Lane wrote: > I think what we should do with this is split it into two patches, one > to fix ALTER REPLICA IDENTITY's crash (which we'd back-patch to 9.4) > and one to forbid indexes on system columns (which IMO should be HEAD > only). The first of those should be pretty uncontroversial, so I'll > go ahead with pushing it --- anyone have a problem with the second? Only in that I'm wondering whether we shouldn't be more aggressive about #2. 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] Disallow unique index on system columns
David Rowley writes: > On 15 April 2016 at 13:43, David Rowley wrote: >> The attached patch just disallows any index containing a system >> column, apart from OID. > Seems I only did half the job as I forgot to think to check for system > columns that are hidden away inside expressions or predicates. > The attached fixes that. I think what we should do with this is split it into two patches, one to fix ALTER REPLICA IDENTITY's crash (which we'd back-patch to 9.4) and one to forbid indexes on system columns (which IMO should be HEAD only). The first of those should be pretty uncontroversial, so I'll go ahead with pushing it --- anyone have a problem with the second? 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] Disallow unique index on system columns
On 15 April 2016 at 13:43, David Rowley wrote: > The attached patch just disallows any index containing a system > column, apart from OID. Seems I only did half the job as I forgot to think to check for system columns that are hidden away inside expressions or predicates. The attached fixes that. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services disallow_index_on_syscols_v2.patch Description: Binary data -- 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] Disallow unique index on system columns
On 15 April 2016 at 13:56, Tom Lane wrote: > David Rowley writes: >> On 15 April 2016 at 13:30, Andres Freund wrote: >>> What'd be the point of indexing ctid, and why would it be correct? >>> Wouldn't, hm, HOT break it? > >> I don't personally see the point. > > An index on ctid is useless by definition: if you know the ctid of > a tuple, you can just go get it, never mind the index. I'm not sure that's 100% accurate, and perhaps it's not worth arguing, as they're likely broken because of HOT anyway, but it does seem like you've totally disregarded the fact that a TIDscan does not support range scanning, where an index scan on ctid would. E.g; how many live tuples are on page 0? select count(*) from t where ctid between '(0,0)' and '(0,1)'; I'm not saying it's going to be a common case. I just want to ensure we've considered all semi realistic use cases before we go and turn this off. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] Disallow unique index on system columns
David Rowley writes: > On 15 April 2016 at 13:30, Andres Freund wrote: >> What'd be the point of indexing ctid, and why would it be correct? >> Wouldn't, hm, HOT break it? > I don't personally see the point. An index on ctid is useless by definition: if you know the ctid of a tuple, you can just go get it, never mind the index. > Is it worth making some changes to pg_dump to skip such indexes? No. 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] Disallow unique index on system columns
On 15 April 2016 at 13:30, Andres Freund wrote: > On 2016-04-15 13:26:35 +1200, David Rowley wrote: >> On 15 April 2016 at 13:02, Tom Lane wrote: >> > David Rowley writes: >> >> I proposed a fix over there, but it didn't go anywhere, probably >> >> because Tom and Andres discussed just disallowing unique indexes on >> >> system columns altogether. So, the attached patch does just that, and >> >> also fixes up the replica identity bugs too, as it's still possible >> >> that someone could create a unique index on a system column with an >> >> old version, upgrade, then try to set the replica identity to that >> >> index. We'd need to handle that correctly, so I fixed that too. >> > >> > AFAIR, what we were discussing was disallowing any index on a system >> > column (other than OID). I do not see why only unique indexes are >> > problematic for them; the semantic issues are independent of that. >> >> I have to admit that my thoughts only considered ctid, which I >> imagined would have been OK to have an index on. As for the other >> system columns (apart from OID), I agree. > > What'd be the point of indexing ctid, and why would it be correct? > Wouldn't, hm, HOT break it? I don't personally see the point. I was just concerned that if there's a slight chance that it's useful, then someone might have one somewhere in the wild, and they might have problems restoring pg_dump backups, once we disallow it. The attached patch just disallows any index containing a system column, apart from OID. Is it worth making some changes to pg_dump to skip such indexes? -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services disallow_index_on_syscols.patch Description: Binary data -- 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] Disallow unique index on system columns
On 2016-04-15 13:26:35 +1200, David Rowley wrote: > On 15 April 2016 at 13:02, Tom Lane wrote: > > David Rowley writes: > >> I proposed a fix over there, but it didn't go anywhere, probably > >> because Tom and Andres discussed just disallowing unique indexes on > >> system columns altogether. So, the attached patch does just that, and > >> also fixes up the replica identity bugs too, as it's still possible > >> that someone could create a unique index on a system column with an > >> old version, upgrade, then try to set the replica identity to that > >> index. We'd need to handle that correctly, so I fixed that too. > > > > AFAIR, what we were discussing was disallowing any index on a system > > column (other than OID). I do not see why only unique indexes are > > problematic for them; the semantic issues are independent of that. > > I have to admit that my thoughts only considered ctid, which I > imagined would have been OK to have an index on. As for the other > system columns (apart from OID), I agree. What'd be the point of indexing ctid, and why would it be correct? Wouldn't, hm, HOT break it? 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] Disallow unique index on system columns
On 15 April 2016 at 13:02, Tom Lane wrote: > David Rowley writes: >> I proposed a fix over there, but it didn't go anywhere, probably >> because Tom and Andres discussed just disallowing unique indexes on >> system columns altogether. So, the attached patch does just that, and >> also fixes up the replica identity bugs too, as it's still possible >> that someone could create a unique index on a system column with an >> old version, upgrade, then try to set the replica identity to that >> index. We'd need to handle that correctly, so I fixed that too. > > AFAIR, what we were discussing was disallowing any index on a system > column (other than OID). I do not see why only unique indexes are > problematic for them; the semantic issues are independent of that. I have to admit that my thoughts only considered ctid, which I imagined would have been OK to have an index on. As for the other system columns (apart from OID), I agree. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] Disallow unique index on system columns
On 2016-04-14 21:02:26 -0400, Tom Lane wrote: > David Rowley writes: > > I proposed a fix over there, but it didn't go anywhere, probably > > because Tom and Andres discussed just disallowing unique indexes on > > system columns altogether. So, the attached patch does just that, and > > also fixes up the replica identity bugs too, as it's still possible > > that someone could create a unique index on a system column with an > > old version, upgrade, then try to set the replica identity to that > > index. We'd need to handle that correctly, so I fixed that too. > > AFAIR, what we were discussing was disallowing any index on a system > column (other than OID). I do not see why only unique indexes are > problematic for them; the semantic issues are independent of that. +1 -- 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] Disallow unique index on system columns
David Rowley writes: > I proposed a fix over there, but it didn't go anywhere, probably > because Tom and Andres discussed just disallowing unique indexes on > system columns altogether. So, the attached patch does just that, and > also fixes up the replica identity bugs too, as it's still possible > that someone could create a unique index on a system column with an > old version, upgrade, then try to set the replica identity to that > index. We'd need to handle that correctly, so I fixed that too. AFAIR, what we were discussing was disallowing any index on a system column (other than OID). I do not see why only unique indexes are problematic for them; the semantic issues are independent of that. 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
[HACKERS] Disallow unique index on system columns
Hi, Over in [1], while I was aiming to fix some incorrect formatting in an error message, Tom noticed that the code in that area was much more broken than I had thought. Basically, if you do; postgres=# create table t (a int) with oids; CREATE TABLE postgres=# create unique index t_oid_idx on t(oid); CREATE INDEX postgres=# alter table t replica identity using index t_oid_idx; You get; server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. I proposed a fix over there, but it didn't go anywhere, probably because Tom and Andres discussed just disallowing unique indexes on system columns altogether. So, the attached patch does just that, and also fixes up the replica identity bugs too, as it's still possible that someone could create a unique index on a system column with an old version, upgrade, then try to set the replica identity to that index. We'd need to handle that correctly, so I fixed that too. I hope there's still time to get this backpacked before the releases. [1] http://www.postgresql.org/message-id/26659.1459485...@sss.pgh.pa.us -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services disallow_unique_index_on_syscols.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
