Re: inherit support for foreign tables

(2014/02/10 21:00), Etsuro Fujita wrote:
> (2014/02/07 21:31), Etsuro Fujita wrote:
>> So, I've modified the patch so
>> that we continue to disallow SET STORAGE on a foreign table *in the same
>> manner as before*, but, as your patch does, allow it on an inheritance
>> hierarchy that contains foreign tables, with the semantics that we
>> quietly ignore the foreign tables and apply the operation to the plain
>> tables, by modifying the ALTER TABLE simple recursion mechanism.

> While reviwing the patch, I've found some issues on interactions with
> other commands, other than the SET STORAGE command.

> * ADD table_constraint NOT VALID: the patch allows ADD table_constraint
> *NOT VALID* to be set on a foreign table as well as an inheritance
> hierarchy that contains foreign tables. But I think it would be better
> to disallow ADD table_constraint *NOT VALID* on a foreign table, but
> allow it on an inheritance hierarchy that contains foreign tables, with
> the semantics that we apply the operation to the plain tables and apply
> the transformed operation *ADD table_constraint* to the foreign tables.

Done.

> * VALIDATE CONSTRAINT constraint_name:

I've modified the patch so that though we continue to disallow the
operation on a foreign table, we allow it on an inheritance hierarchy
that contains foreign tables. In that case, the to-be-validated
constraints on the plain tables are validated by the operation, as
before, and the constraints on the foreign tables are ignored. Note
that the constraints on the foreign tables are not NOT VALID due to the
spec mentioned above.

> * SET WITH OIDS: though the patch disallow the direct operation on
> foreign tables, it allows the operation on an inheritance hierarchy that
> contains foreign tables, and that operation is successfully done on
> foreign tables. I think it would be better to modify the patch so that
> we allow it on an inheritance hierarchy that contains foreign tables,
> with the semantics that we quietly ignore the foreign tables and apply
> the operation to the plain tables just like the semantics of SET STORAGE.

I noticed this breaks inheritance hierarchy. To avoid this, I've
modified the patch to disallow SET WITH OIDS on an inheritance hierarchy
that contains at least one foreign table.

The other changes:

- if (stmt->constraints != NIL && relkind == RELKIND_FOREIGN_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("constraints are not supported
on foreign tables")));
+/*
+ * Shouldn't this have been checked in parser?
+ */
+ if (relkind == RELKIND_FOREIGN_TABLE)
+ {
+ ListCell *lc;
+ foreach(lc, stmt->constraints)
+ {
+ NewConstraint *nc = lfirst(lc);
+
+ if (nc->contype != CONSTR_CHECK &&
+ nc->contype != CONSTR_DEFAULT &&
+ nc->contype != CONSTR_NULL &&
+ nc->contype != CONSTR_NOTNULL)
+ ereport(ERROR,
+
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("only check
constraints are supported on foreign tables")));
+ }
+ }

ISTM we wouldn't need the above check in DefineRelation(), so I've
removed the check from the patch. And I've modified the vague error
messages in parse_utilcmd.c.

There seems to be no objections, I've merged the ANALYZE patch [1] with
your patch. I noticed that the patch in [1] isn't efficient. (To
ANALYZE one inheritance tree that contains foreign tables, the patch in
[1] calls the AnalyzeForeignTable() routine two times for each such
foreign table.) So, the updated version has been merged that calls the
routine only once for each such foreign table.

Todo:

ISTM the documentation would need to be updated further.

Thanks,

[1] http://www.postgresql.org/message-id/52EB10AC.4070307@lab.ntt.co.jp

Best regards,
Etsuro Fujita