Обсуждение: ATSimpleRecursion() and inheritance foreign parents
Hi, Following ALTER TABLE actions are applied recursively to inheritance descendents via ATSimpleRecursion() - ALTER COLUMN DEFAULT ALTER COLUMN DROP NOT NULL ALTER COLUMN SET NOT NULL ALTER COLUMN SET STATISTICS ALTER COLUMN SET STORAGE The code at the beginning of ATSimpleRecursion() looks like - /** Propagate to children if desired. Non-table relations never have* children, so no need to search in that case.*/if (recurse&& rel->rd_rel->relkind == RELKIND_RELATION) Not sure if it's great idea, but now that foreign tables can also have children, should above be changed to take that into account? Any inheritance related recursion performed without using ATSimpleRecursion() recurse as dictated by RangeVar.inhOpt; so even foreign inheritance parents expand for various ALTER TABLE actions like adding a column though that is not a meaningful operation on foreign tables anyway. An example, postgres=# alter foreign table fparent alter a type char; ALTER FOREIGN TABLE postgres=# select * from fparent; ERROR: attribute "a" of relation "fchild1" does not match parent's type Above error, AIUI, is hit much before it is determined that fparent is a foreign table, whereas the following is FDW-specific (waiting to happen) error, postgres=# alter foreign table fparent add b char; ALTER FOREIGN TABLE postgres=# SELECT * FROM fparent; ERROR: column "b" does not exist CONTEXT: Remote SQL command: SELECT a, b FROM public.parent Not sure if the first case could be considered s a bug as foreign tables are pretty lax in these regards anyway. But if ATSimpleRecursion() prevents errors that result from concepts independent of foreign tables being involved, maybe, we need to fix? Thanks, Amit
On 2015/04/28 15:17, Amit Langote wrote: > The code at the beginning of ATSimpleRecursion() looks like - > > /* > * Propagate to children if desired. Non-table relations never have > * children, so no need to search in that case. > */ > if (recurse && rel->rd_rel->relkind == RELKIND_RELATION) > > Not sure if it's great idea, but now that foreign tables can also have > children, should above be changed to take that into account? Yeah, I think we should now allow the recursion for inheritance parents that are foreign tables as well. Attached is a patch for that. > so even foreign inheritance parents expand for > various ALTER TABLE actions like adding a column though that is not a > meaningful operation on foreign tables anyway. > > An example, > postgres=# alter foreign table fparent alter a type char; > ALTER FOREIGN TABLE > > postgres=# select * from fparent; > ERROR: attribute "a" of relation "fchild1" does not match parent's type > > Above error, AIUI, is hit much before it is determined that fparent is a > foreign table, whereas the following is FDW-specific (waiting to happen) error, > > postgres=# alter foreign table fparent add b char; > ALTER FOREIGN TABLE > > postgres=# SELECT * FROM fparent; > ERROR: column "b" does not exist > CONTEXT: Remote SQL command: SELECT a, b FROM public.parent > > Not sure if the first case could be considered s a bug as foreign tables are > pretty lax in these regards anyway. I think the first case would be a bug caused by ATSimpleRecursion() that doesn't allow the recursion. But I don't think the second case is a bug. As described in the notes in the reference page on ALTER FOREIGN TABLE, I think it should be the user's responsibility to ensure that the foreign table definition matches the reality. Best regards, Etsuro Fujita
Вложения
On Tue, Apr 28, 2015 at 8:45 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote: > On 2015/04/28 15:17, Amit Langote wrote: > > Yeah, I think we should now allow the recursion for inheritance parents that > are foreign tables as well. Attached is a patch for that. > Thanks! >> An example, >> postgres=# alter foreign table fparent alter a type char; >> ALTER FOREIGN TABLE >> >> postgres=# select * from fparent; >> ERROR: attribute "a" of relation "fchild1" does not match parent's type >> >> Above error, AIUI, is hit much before it is determined that fparent is a >> foreign table, whereas the following is FDW-specific (waiting to happen) >> error, >> >> postgres=# alter foreign table fparent add b char; >> ALTER FOREIGN TABLE >> >> postgres=# SELECT * FROM fparent; >> ERROR: column "b" does not exist >> CONTEXT: Remote SQL command: SELECT a, b FROM public.parent >> >> Not sure if the first case could be considered s a bug as foreign tables >> are >> pretty lax in these regards anyway. > > > I think the first case would be a bug caused by ATSimpleRecursion() that > doesn't allow the recursion. But I don't think the second case is a bug. > As described in the notes in the reference page on ALTER FOREIGN TABLE, I > think it should be the user's responsibility to ensure that the foreign > table definition matches the reality. > Yeah, I was guessing the first one would be a bug but wasn't quite sure and the second one is a well-documented behavior for foreign tables. I just felt that cases covered under ATSimpleRecursion() may also under fall under that category (being documented); but I guess not. Thanks, Amit
On Tue, Apr 28, 2015 at 03:17:08PM +0900, Amit Langote wrote: > > Hi, > > Following ALTER TABLE actions are applied recursively to inheritance > descendents via ATSimpleRecursion() - > > ALTER COLUMN DEFAULT > ALTER COLUMN DROP NOT NULL > ALTER COLUMN SET NOT NULL > ALTER COLUMN SET STATISTICS > ALTER COLUMN SET STORAGE > > The code at the beginning of ATSimpleRecursion() looks like - > > /* > * Propagate to children if desired. Non-table relations never have > * children, so no need to search in that case. > */ > if (recurse && rel->rd_rel->relkind == RELKIND_RELATION) > > Not sure if it's great idea, but now that foreign tables can also have > children, should above be changed to take that into account? Any inheritance > related recursion performed without using ATSimpleRecursion() recurse as > dictated by RangeVar.inhOpt; so even foreign inheritance parents expand for > various ALTER TABLE actions like adding a column though that is not a > meaningful operation on foreign tables anyway. > > An example, > postgres=# alter foreign table fparent alter a type char; > ALTER FOREIGN TABLE > > postgres=# select * from fparent; > ERROR: attribute "a" of relation "fchild1" does not match parent's type > > Above error, AIUI, is hit much before it is determined that fparent is a > foreign table, whereas the following is FDW-specific (waiting to happen) error, > > postgres=# alter foreign table fparent add b char; > ALTER FOREIGN TABLE > > postgres=# SELECT * FROM fparent; > ERROR: column "b" does not exist > CONTEXT: Remote SQL command: SELECT a, b FROM public.parent I'm pretty sure this is a bug. The way I see it, foreign tables can either fully participate in table inheritance, or not at all, because any inconsistencies here will cause confusion at best. How big a deal would it be to fix it? Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Tue, Apr 28, 2015 at 9:28 PM, David Fetter <david@fetter.org> wrote: > On Tue, Apr 28, 2015 at 03:17:08PM +0900, Amit Langote wrote: >> >> An example, >> postgres=# alter foreign table fparent alter a type char; >> ALTER FOREIGN TABLE >> >> postgres=# select * from fparent; >> ERROR: attribute "a" of relation "fchild1" does not match parent's type >> >> Above error, AIUI, is hit much before it is determined that fparent is a >> foreign table, whereas the following is FDW-specific (waiting to happen) error, >> >> postgres=# alter foreign table fparent add b char; >> ALTER FOREIGN TABLE >> >> postgres=# SELECT * FROM fparent; >> ERROR: column "b" does not exist >> CONTEXT: Remote SQL command: SELECT a, b FROM public.parent > > I'm pretty sure this is a bug. The way I see it, foreign tables can > either fully participate in table inheritance, or not at all, because > any inconsistencies here will cause confusion at best. > As mentioned by Fujita-san, the first one is definitely a bug (he sent a patch) but the second one is not quite related to inheritance; that is, it can happen irrespective of foreign tables participating in inheritance and is documented. Thanks, Amit
On Tue, Apr 28, 2015 at 09:39:02PM +0900, Amit Langote wrote: > On Tue, Apr 28, 2015 at 9:28 PM, David Fetter <david@fetter.org> wrote: > > On Tue, Apr 28, 2015 at 03:17:08PM +0900, Amit Langote wrote: > >> > >> An example, > >> postgres=# alter foreign table fparent alter a type char; > >> ALTER FOREIGN TABLE > >> > >> postgres=# select * from fparent; > >> ERROR: attribute "a" of relation "fchild1" does not match parent's type > >> > >> Above error, AIUI, is hit much before it is determined that fparent is a > >> foreign table, whereas the following is FDW-specific (waiting to happen) error, > >> > >> postgres=# alter foreign table fparent add b char; > >> ALTER FOREIGN TABLE > >> > >> postgres=# SELECT * FROM fparent; > >> ERROR: column "b" does not exist > >> CONTEXT: Remote SQL command: SELECT a, b FROM public.parent > > > > I'm pretty sure this is a bug. The way I see it, foreign tables can > > either fully participate in table inheritance, or not at all, because > > any inconsistencies here will cause confusion at best. > > > > As mentioned by Fujita-san, the first one is definitely a bug (he sent > a patch) but the second one is not quite related to inheritance; that > is, it can happen irrespective of foreign tables participating in > inheritance and is documented. My mistake. Sorry for the noise. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> On 2015/04/28 15:17, Amit Langote wrote:
>> The code at the beginning of ATSimpleRecursion() looks like -
>> if (recurse && rel->rd_rel->relkind == RELKIND_RELATION)
>> Not sure if it's great idea, but now that foreign tables can also have
>> children, should above be changed to take that into account?
> Yeah, I think we should now allow the recursion for inheritance parents
> that are foreign tables as well. Attached is a patch for that.
Yeah, this is just an oversight. Fix pushed, and also a similar fix in
parse_utilcmd.c. I thought I'd reviewed all the references to
RELKIND_RELATION before, but evidently I missed these.
regards, tom lane
On 2015/04/29 4:35, Tom Lane wrote: > Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes: >> On 2015/04/28 15:17, Amit Langote wrote: >>> The code at the beginning of ATSimpleRecursion() looks like - >>> if (recurse && rel->rd_rel->relkind == RELKIND_RELATION) >>> Not sure if it's great idea, but now that foreign tables can also have >>> children, should above be changed to take that into account? > >> Yeah, I think we should now allow the recursion for inheritance parents >> that are foreign tables as well. Attached is a patch for that. > > Yeah, this is just an oversight. Fix pushed, and also a similar fix in > parse_utilcmd.c. Thanks! Best regards, Etsuro Fujita