ATSimpleRecursion() and inheritance foreign parents

Started by Amit Langoteover 10 years ago8 messages

Langote_Amit_f8@lab.ntt.co.jp

over 10 years ago

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 10 years ago

In reply to: Amit Langote (#1)

1 attachment(s)

Re: ATSimpleRecursion() and inheritance foreign parents

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

Attachments:

ATSimpleRecursion.patchtext/x-diff; name=ATSimpleRecursion.patchDownload

*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 4367,4376 **** ATSimpleRecursion(List **wqueue, Relation rel,
  				  AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode)
  {
  	/*
! 	 * 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)
  	{
  		Oid			relid = RelationGetRelid(rel);
  		ListCell   *child;
--- 4367,4379 ----
  				  AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode)
  {
  	/*
! 	 * Propagate to children if desired.  Relations other than plain tables
! 	 * and foreign tables never have children, so no need to search in that
! 	 * case.
  	 */
! 	if (recurse &&
! 		(rel->rd_rel->relkind == RELKIND_RELATION ||
! 		 rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE))
  	{
  		Oid			relid = RelationGetRelid(rel);
  		ListCell   *child;

Amit Langote

amitlangote09@gmail.com

over 10 years ago

In reply to: Etsuro Fujita (#2)

Re: ATSimpleRecursion() and inheritance foreign parents

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Fetter

david@fetter.org

over 10 years ago

In reply to: Amit Langote (#1)

Re: ATSimpleRecursion() and inheritance foreign parents

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Amit Langote

amitlangote09@gmail.com

over 10 years ago

In reply to: David Fetter (#4)

Re: ATSimpleRecursion() and inheritance foreign parents

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Fetter

david@fetter.org

over 10 years ago

In reply to: Amit Langote (#5)

Re: ATSimpleRecursion() and inheritance foreign parents

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

--
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

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Etsuro Fujita (#2)

Re: ATSimpleRecursion() and inheritance foreign parents

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 10 years ago

In reply to: Tom Lane (#7)

Re: ATSimpleRecursion() and inheritance foreign parents

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers