Thread: Indexes on partitioned tables and foreign partitions
Hi,
8b08f7d4 added propagation of indexes on partitioned tables to
partitions, which is very cool. However, index creation also recurses
down to foreign tables. I doubt this is intentional, as such indexes are
forbidden as not making much sense; attempt to create index on
partitioned table with foreign partition leads to an error
now. Attached lines fix this.
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
*************** DefineIndex(Oid relationId,
*** 915,920 ****
--- 915,926 ----
int maplen;
childrel = heap_open(childRelid, lockmode);
+ /* Foreign table doesn't need indexes */
+ if (childrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
+ {
+ heap_close(childrel, NoLock);
+ continue;
+ }
childidxs = RelationGetIndexList(childrel);
attmap =
convert_tuples_by_name_map(RelationGetDescr(childrel),
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** AttachPartitionEnsureIndexes(Relation re
*** 14352,14357 ****
--- 14352,14361 ----
MemoryContext cxt;
MemoryContext oldcxt;
+ /* Foreign table doesn't need indexes */
+ if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
+ return;
+
cxt = AllocSetContextCreate(CurrentMemoryContext,
"AttachPartitionEnsureIndexes",
ALLOCSET_DEFAULT_SIZES);
--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Wed, May 9, 2018 at 5:20 PM, Arseny Sher <a.sher@postgrespro.ru> wrote: > Hi, > > 8b08f7d4 added propagation of indexes on partitioned tables to > partitions, which is very cool. However, index creation also recurses > down to foreign tables. I doubt this is intentional, as such indexes are > forbidden as not making much sense; attempt to create index on > partitioned table with foreign partition leads to an error > now. Attached lines fix this. > The patch looks good, but I guess that we have to do some tricks with the validity of index on the partitioned table since not all partitions have a given index when one of those is a foreign partition. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On 9 May 2018 at 12:50, Arseny Sher <a.sher@postgrespro.ru> wrote: > Hi, > > 8b08f7d4 added propagation of indexes on partitioned tables to > partitions, which is very cool. However, index creation also recurses > down to foreign tables. I doubt this is intentional, as such indexes are > forbidden as not making much sense; attempt to create index on > partitioned table with foreign partition leads to an error > now. Attached lines fix this. "Fix"? How much sense is it to have a partitioned table with a mix of local and foreign tables? Shouldn't the fix be to allow creation of indexes on foreign tables? (Maybe they would be virtual or foreign indexes??) -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Simon Riggs <simon@2ndquadrant.com> writes: > How much sense is it to have a partitioned table with a mix of local > and foreign tables? Well, as much sense as fdw-based sharding has, for instance. It is arguable, but it exists. > Shouldn't the fix be to allow creation of indexes on foreign tables? > (Maybe they would be virtual or foreign indexes??) Similar ideas were discussed at [1]. There was no wide consensus of even what problems such feature would solve. Since currently indexes on foreign tables are just forbidden, it seems to me that the best what partitioning code can do today is just not creating them. [1] https://www.postgresql.org/message-id/flat/4F62FD69.2060007%40lab.ntt.co.jp#4F62FD69.2060007@lab.ntt.co.jp -- Arseny Sher Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On 9 May 2018 at 15:26, Arseny Sher <a.sher@postgrespro.ru> wrote: > > Simon Riggs <simon@2ndquadrant.com> writes: > >> How much sense is it to have a partitioned table with a mix of local >> and foreign tables? > > Well, as much sense as fdw-based sharding has, for instance. It is > arguable, but it exists. > >> Shouldn't the fix be to allow creation of indexes on foreign tables? >> (Maybe they would be virtual or foreign indexes??) > > Similar ideas were discussed at [1]. There was no wide consensus of even > what problems such feature would solve. Since currently indexes on > foreign tables are just forbidden, it seems to me that the best what > partitioning code can do today is just not creating them. > > [1] https://www.postgresql.org/message-id/flat/4F62FD69.2060007%40lab.ntt.co.jp#4F62FD69.2060007@lab.ntt.co.jp Indexes on foreign tables cause an ERROR, so yes, we already just don't create them. You're suggesting silently skipping the ERROR. I can't see a reason for that. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, May 9, 2018 at 9:08 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > How much sense is it to have a partitioned table with a mix of local > and foreign tables? Fair question, but we put some effort into making it work, so I think it should keep working. > Shouldn't the fix be to allow creation of indexes on foreign tables? > (Maybe they would be virtual or foreign indexes??) It might be useful to invent the concept of a foreign index, but not for v11 a month after feature freeze. For right now, I think the options are (1) throw an ERROR if we encounter a foreign table or (2) silently skip the foreign table. I think (2) is defensible for non-UNIQUE indexes, because the index is just a performance optimization. However, for UNIQUE indexes, at least, it seems like we'd better do (1), because a major point of such an index is to enforce a constraint; we can't allege that we have such a constraint if foreign tables are just silently skipped. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, May 9, 2018 at 9:08 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> Shouldn't the fix be to allow creation of indexes on foreign tables?
>> (Maybe they would be virtual or foreign indexes??)
> It might be useful to invent the concept of a foreign index, but not
> for v11 a month after feature freeze.
Yeah. That's a can of worms we can *not* open at this stage.
> For right now, I think the options are (1) throw an ERROR if we
> encounter a foreign table or (2) silently skip the foreign table. I
> think (2) is defensible for non-UNIQUE indexes, because the index is
> just a performance optimization. However, for UNIQUE indexes, at
> least, it seems like we'd better do (1), because a major point of such
> an index is to enforce a constraint; we can't allege that we have such
> a constraint if foreign tables are just silently skipped.
Agreed about unique indexes. I suggest that we throw an error for both
cases, because (1) having unique and non-unique indexes behave differently
for this purpose seems pretty weird; (2) throwing an error now preserves
our options to do something different later. Given where we are in the
release cycle, we should be taking the most conservative path we can.
regards, tom lane
On 9 May 2018 at 15:57, Robert Haas <robertmhaas@gmail.com> wrote: > For right now, I think the options are (1) throw an ERROR if we > encounter a foreign table or (2) silently skip the foreign table. I > think (2) is defensible for non-UNIQUE indexes, because the index is > just a performance optimization. However, for UNIQUE indexes, at > least, it seems like we'd better do (1), because a major point of such > an index is to enforce a constraint; we can't allege that we have such > a constraint if foreign tables are just silently skipped. If we can assume an index exists on a foreign table, why can we not just assume a unique index exists?? Why the difference? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 9 May 2018 at 16:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, May 9, 2018 at 9:08 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>> Shouldn't the fix be to allow creation of indexes on foreign tables? >>> (Maybe they would be virtual or foreign indexes??) > >> It might be useful to invent the concept of a foreign index, but not >> for v11 a month after feature freeze. > > Yeah. That's a can of worms we can *not* open at this stage. Lucky nobody suggested that then, eh? Robert's just making a joke. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, May 9, 2018 at 11:14 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 9 May 2018 at 16:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> On Wed, May 9, 2018 at 9:08 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>>> Shouldn't the fix be to allow creation of indexes on foreign tables? >>>> (Maybe they would be virtual or foreign indexes??) >> >>> It might be useful to invent the concept of a foreign index, but not >>> for v11 a month after feature freeze. >> >> Yeah. That's a can of worms we can *not* open at this stage. > > Lucky nobody suggested that then, eh? Robert's just making a joke. Someone did suggest that. It was you. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 9 May 2018 at 16:15, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, May 9, 2018 at 11:14 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> On 9 May 2018 at 16:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Robert Haas <robertmhaas@gmail.com> writes: >>>> On Wed, May 9, 2018 at 9:08 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>>>> Shouldn't the fix be to allow creation of indexes on foreign tables? >>>>> (Maybe they would be virtual or foreign indexes??) >>> >>>> It might be useful to invent the concept of a foreign index, but not >>>> for v11 a month after feature freeze. >>> >>> Yeah. That's a can of worms we can *not* open at this stage. >> >> Lucky nobody suggested that then, eh? Robert's just making a joke. > > Someone did suggest that. It was you. Oh, you weren't joking. I think we're having serious problems with people putting words in my mouth again then. Please show me where I suggested doing anything for v11? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, May 9, 2018 at 11:20 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 9 May 2018 at 16:15, Robert Haas <robertmhaas@gmail.com> wrote: >> On Wed, May 9, 2018 at 11:14 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>> On 9 May 2018 at 16:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> Robert Haas <robertmhaas@gmail.com> writes: >>>>> On Wed, May 9, 2018 at 9:08 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>>>>> Shouldn't the fix be to allow creation of indexes on foreign tables? >>>>>> (Maybe they would be virtual or foreign indexes??) >>>> >>>>> It might be useful to invent the concept of a foreign index, but not >>>>> for v11 a month after feature freeze. >>>> >>>> Yeah. That's a can of worms we can *not* open at this stage. >>> >>> Lucky nobody suggested that then, eh? Robert's just making a joke. >> >> Someone did suggest that. It was you. > > Oh, you weren't joking. I think we're having serious problems with > people putting words in my mouth again then. > > Please show me where I suggested doing anything for v11? Come on, Simon. It's in the quoted text. I realize you didn't say v11 specifically, but this is the context of a patch that is proposed a bug-fix for v11. If you meant that we should apply the patch as proposed now, or some other one, and do the other thing later, you could have said so. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, May 9, 2018 at 11:12 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > If we can assume an index exists on a foreign table, why can we not > just assume a unique index exists?? Why the difference? We can't assume either of those things, and I didn't say that we should. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Simon Riggs <simon@2ndquadrant.com> writes: > Indexes on foreign tables cause an ERROR, so yes, we already just > don't create them. > > You're suggesting silently skipping the ERROR. I can't see a reason for that. Truly, I was inaccurate. I mean that index propagation is a nice feature, and making it available for mix of foreign and local partitions skipping the foreign ones is useful thing for users who understand the consequences (of course there should be a warning in the docs -- my patch was just an illustration of the idea). As I said, FDW-based sharding often involves mix of foreign and local partitions, even in simple manual forms. Imagine that you have a table which is too big to fit into one server, so you partition it and move some partiitons to another machine; you still would like to access the whole table to avoid manual tuple routing. Or, you partition table by timestamps and keep recent data on fast primary server, gradually moving old partitions to another box. Again, it would be nice to access table as a whole to e.g. calculate some analytics. Anyway, given other opinions, I assume that the community has chosen more conservative approach. Indeed, we can't guarantee uniqueness this way; that's fully users responsiblity. -- Arseny Sher Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On Wed, May 09, 2018 at 11:10:43AM -0400, Tom Lane wrote:
> Agreed about unique indexes. I suggest that we throw an error for both
> cases, because (1) having unique and non-unique indexes behave differently
> for this purpose seems pretty weird; (2) throwing an error now preserves
> our options to do something different later. Given where we are in the
> release cycle, we should be taking the most conservative path we can.
Heartfully agreed to throw an error for all cases for now. Something
that I find confusing on HEAD though is that DefineIndex calls itself
around line 1006 and cascades through each children but there is no
context about the error.
For example if I have this partition layer:
CREATE TABLE measurement (
logdate date not null,
peaktemp int,
unitsales int
) PARTITION BY RANGE (logdate);
CREATE FOREIGN TABLE measurement_y2016m07
PARTITION OF measurement FOR VALUES FROM ('2016-07-01') TO
('2016-08-01')
SERVER postgres_server;
Then by creating an index on the parent I get that:
=# create index measurement_idx on measurement (peaktemp);
ERROR: 42809: cannot create index on foreign table "measurement_y2016m07"
LOCATION: DefineIndex, indexcmds.c:442
This may be confusing for the user as the DDL does not complain directly
about the relation worked on. Perhaps we could add an error context?
We surely don't want multiple contexts either when working on long
chains, but we could build a chained list of relation names in the error
message.
--
Michael
Attachment
On 2018/05/09 23:57, Robert Haas wrote: > For right now, I think the options are (1) throw an ERROR if we > encounter a foreign table or (2) silently skip the foreign table. I > think (2) is defensible for non-UNIQUE indexes, because the index is > just a performance optimization. Along with others, my +1 to option 1. > However, for UNIQUE indexes, at > least, it seems like we'd better do (1), because a major point of such > an index is to enforce a constraint; we can't allege that we have such > a constraint if foreign tables are just silently skipped. While I agree with this, let me point out that we do allow inherited check constraints on foreign tables that are not actually enforced locally. create table p (a int) partition by range (a); create table p1 partition of p for values from (minvalue) to (1); create table p2base (a int); create foreign table p2 partition of p for values from (1) to (maxvalue) server loopback options (table_name 'p2base'); alter table p add check (a between -1000 and 1000); -- routed to foreign partition, which doesn't enforce check constraints insert into p values (1001); INSERT 0 1 -- whereas, local partition does insert into p values (-1001); ERROR: new row for relation "p1" violates check constraint "p_a_check" DETAIL: Failing row contains (-1001). We have to do the following to prevent that. alter table p2base add check (a between -1000 and 1000); insert into p values (1001); ERROR: new row for relation "p2base" violates check constraint "p2base_a_check" DETAIL: Failing row contains (1001). CONTEXT: remote SQL command: INSERT INTO public.p2base(a) VALUES ($1) Thanks, Amit
On Thu, May 10, 2018 at 10:15:05AM +0900, Amit Langote wrote: > While I agree with this, let me point out that we do allow inherited check > constraints on foreign tables that are not actually enforced locally. > > create table p (a int) partition by range (a); > create table p1 partition of p for values from (minvalue) to (1); > create table p2base (a int); > create foreign table p2 partition of p for values from (1) to (maxvalue) > server loopback options (table_name 'p2base'); > > alter table p add check (a between -1000 and 1000); > > -- routed to foreign partition, which doesn't enforce check constraints > insert into p values (1001); > INSERT 0 1 That's not actually a surprise, right? Since foreign tables can be part of inheritance trees in 9.5, CHECK constraints on foreign tables are not enforced locally, but used as planner hints to guess how a query would work remotely. So getting partition children to work the same way is consistent. > We have to do the following to prevent that. > > alter table p2base add check (a between -1000 and 1000); > insert into p values (1001); > ERROR: new row for relation "p2base" violates check constraint > "p2base_a_check" > DETAIL: Failing row contains (1001). > CONTEXT: remote SQL command: INSERT INTO public.p2base(a) VALUES ($1) This bit looks natural to me as well. -- Michael
Attachment
On 2018/05/10 10:37, Michael Paquier wrote: > On Thu, May 10, 2018 at 10:15:05AM +0900, Amit Langote wrote: >> While I agree with this, let me point out that we do allow inherited check >> constraints on foreign tables that are not actually enforced locally. >> >> create table p (a int) partition by range (a); >> create table p1 partition of p for values from (minvalue) to (1); >> create table p2base (a int); >> create foreign table p2 partition of p for values from (1) to (maxvalue) >> server loopback options (table_name 'p2base'); >> >> alter table p add check (a between -1000 and 1000); >> >> -- routed to foreign partition, which doesn't enforce check constraints >> insert into p values (1001); >> INSERT 0 1 > > That's not actually a surprise, right? Since foreign tables can be part > of inheritance trees in 9.5, CHECK constraints on foreign tables are not > enforced locally, but used as planner hints to guess how a query would > work remotely. So getting partition children to work the same way is > consistent. > >> We have to do the following to prevent that. >> >> alter table p2base add check (a between -1000 and 1000); >> insert into p values (1001); >> ERROR: new row for relation "p2base" violates check constraint >> "p2base_a_check" >> DETAIL: Failing row contains (1001). >> CONTEXT: remote SQL command: INSERT INTO public.p2base(a) VALUES ($1) > > This bit looks natural to me as well. Yes, I know it is working as designed and documented. I was just trying to comment on this bit of Robert's email: "...because a major point of such an index is to enforce a constraint; we can't allege that we have such a constraint if foreign tables are just silently skipped." So if someday we go ahead and allow indexes to be created on partitioned tables even if there are foreign partitions, how would we choose to deal with a unique constraint? Will it be same as CHECK constraints such that we don't enforce it locally but only assume it to be enforced by the remote data source using whatever method? But it seems I've misinterpreted what he was saying. He doesn't seem to be saying anything about how or whether we enforce the unique constraint on foreign tables. Only that if someone creates a constraint index on the partitioned table, all partitions *including* foreign partitions, must get a copy. So for now, we give users an error if they try to create an index on a partitioned table with a mix of local and foreign partitions. Once we figure out how to allow creating indexes (constraint-enforcing or not) on foreign tables, we can then think of relaxing that restriction. Thanks, Amit
On 2018/05/10 10:02, Michael Paquier wrote:
> Something
> that I find confusing on HEAD though is that DefineIndex calls itself
> around line 1006 and cascades through each children but there is no
> context about the error.
>
> For example if I have this partition layer:
> CREATE TABLE measurement (
> logdate date not null,
> peaktemp int,
> unitsales int
> ) PARTITION BY RANGE (logdate);
> CREATE FOREIGN TABLE measurement_y2016m07
> PARTITION OF measurement FOR VALUES FROM ('2016-07-01') TO
> ('2016-08-01')
> SERVER postgres_server;
>
> Then by creating an index on the parent I get that:
> =# create index measurement_idx on measurement (peaktemp);
> ERROR: 42809: cannot create index on foreign table "measurement_y2016m07"
> LOCATION: DefineIndex, indexcmds.c:442
>
> This may be confusing for the user as the DDL does not complain directly
> about the relation worked on. Perhaps we could add an error context?
> We surely don't want multiple contexts either when working on long
> chains, but we could build a chained list of relation names in the error
> message.
How about we error out even *before* calling DefineIndex for the 1st time?
I see that ProcessUtilitySlow() gets a list of all partitions when
locking them for index creation before calling DefineIndex. Maybe, just
go through the list and error out if one of them is a partition that we
don't support creating an index on?
For example, with the attached patch doing the above, I get:
create table p (a int) partition by range (a);
create table p1 partition of p for values from (minvalue) to (1);
create foreign table p2 partition of p for values from (1) to (maxvalue)
server loopback options (table_name 'p2base');
create index on p (a);
ERROR: cannot create index on partitioned table containing foreign partitions
Thanks,
Amit
Attachment
On Thu, May 10, 2018 at 8:30 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > How about we error out even *before* calling DefineIndex for the 1st time? > I see that ProcessUtilitySlow() gets a list of all partitions when > locking them for index creation before calling DefineIndex. Maybe, just > go through the list and error out if one of them is a partition that we > don't support creating an index on? +1 for the idea. We don't want to spend efforts in DefineIndex for some relations only to throw away that work later. I quickly looked at the patch and I think it's on right track. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On Wed, May 9, 2018 at 10:20 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > But it seems I've misinterpreted what he was saying. He doesn't seem to > be saying anything about how or whether we enforce the unique constraint > on foreign tables. Only that if someone creates a constraint index on the > partitioned table, all partitions *including* foreign partitions, must get > a copy. Honestly, I hadn't quite gotten that far in my thinking. That's a really useful distinction, and I completely agree with it. > So for now, we give users an error if they try to create an index on a > partitioned table with a mix of local and foreign partitions. Once we > figure out how to allow creating indexes (constraint-enforcing or not) on > foreign tables, we can then think of relaxing that restriction. Yeah, that sounds exactly right. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 9 May 2018 at 17:33, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, May 9, 2018 at 11:20 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> On 9 May 2018 at 16:15, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Wed, May 9, 2018 at 11:14 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>>> On 9 May 2018 at 16:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>>> Robert Haas <robertmhaas@gmail.com> writes: >>>>>> On Wed, May 9, 2018 at 9:08 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>>>>>> Shouldn't the fix be to allow creation of indexes on foreign tables? >>>>>>> (Maybe they would be virtual or foreign indexes??) >>>>> >>>>>> It might be useful to invent the concept of a foreign index, but not >>>>>> for v11 a month after feature freeze. >>>>> >>>>> Yeah. That's a can of worms we can *not* open at this stage. >>>> >>>> Lucky nobody suggested that then, eh? Robert's just making a joke. >>> >>> Someone did suggest that. It was you. >> >> Oh, you weren't joking. I think we're having serious problems with >> people putting words in my mouth again then. >> >> Please show me where I suggested doing anything for v11? > > Come on, Simon. It's in the quoted text. No, its not. > I realize you didn't say > v11 specifically, but this is the context of a patch that is proposed > a bug-fix for v11. If you meant that we should apply the patch as > proposed now, or some other one, and do the other thing later, you > could have said so. I think its reasonable to expect you interpret my words sensibly, rather than in some more dramatic form where I seem to break rules with every phrase. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, May 11, 2018 at 8:52 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > I think its reasonable to expect you interpret my words sensibly, > rather than in some more dramatic form where I seem to break rules > with every phrase. Sure, I agree. I try to interpret the words of everyone here sensibly and without unnecessary drama. I don't really know why we're still talking about this. My principle concern here has never been to understand whether or not you were proposing foreign indexes for v11 -- although I have now got a very clear idea that you weren't -- but to resolve the actual problem -- which seems to be a bug in 8b08f7d4. I regret that we seem to have gotten sucked into a conversation about whether I was being unreasonable in thinking that you might have meant what I thought you might have meant for a period of 3 hours last Wednesday, rather than about the technical issue. I apologize to you and to the list for my contribution to that state of affairs. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2018-May-10, Amit Langote wrote: > How about we error out even *before* calling DefineIndex for the 1st time? > I see that ProcessUtilitySlow() gets a list of all partitions when > locking them for index creation before calling DefineIndex. Maybe, just > go through the list and error out if one of them is a partition that we > don't support creating an index on? The overwhelming consensus seems to be for this option, so I pushed your patch after some small tweaks -- mostly to simplify unnecessarily baroque code. (I must have been thinking that recursion would happen right in ProcessUtilitySlow. That doesn't match my memories, but I can't explain this code otherwise.) I added a very small test too. I think it'd be better to take this out of ProcessUtility also and into DefineInde, for cleanliness sake; maybe add a 'recursing' flag to DefineIndex. Not for pg11, though. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/05/15 2:29, Alvaro Herrera wrote: > On 2018-May-10, Amit Langote wrote: > >> How about we error out even *before* calling DefineIndex for the 1st time? >> I see that ProcessUtilitySlow() gets a list of all partitions when >> locking them for index creation before calling DefineIndex. Maybe, just >> go through the list and error out if one of them is a partition that we >> don't support creating an index on? > > The overwhelming consensus seems to be for this option, so I pushed your > patch after some small tweaks -- mostly to simplify unnecessarily > baroque code. (I must have been thinking that recursion would happen > right in ProcessUtilitySlow. That doesn't match my memories, but I > can't explain this code otherwise.) I added a very small test too. Thank you. > I think it'd be better to take this out of ProcessUtility also and into > DefineInde, for cleanliness sake; maybe add a 'recursing' flag to > DefineIndex. Not for pg11, though. Agreed. Most of the stuff in utility.c is for command dispatch and there is no reason for this partitioning-related bit to be sitting here. Moving it will require perhaps non-trivial adjustment of indexcmds.c code, so let's leave it for later as you say. Thanks, Amit