Re: inherit support for foreign tables - Mailing list pgsql-hackers

On Fri, Jun 20, 2014 at 05:04:06PM +0900, Kyotaro HORIGUCHI wrote:
> Attached is the rebased patch of v11 up to the current master.

I've been studying this patch.

SELECT FOR UPDATE on the inheritance parent fails with a can't-happen error
condition, even when SELECT FOR UPDATE on the child foreign table alone would
have succeeded.

The patch adds zero tests.  Add principal tests to postgres_fdw.sql.  Also,
create a child foreign table in foreign_data.sql; this will make dump/reload
tests of the regression database exercise an inheritance tree that includes a
foreign table.

The inheritance section of ddl.sgml should mention child foreign tables, at
least briefly.  Consider mentioning it in the partitioning section, too.


Your chosen ANALYZE behavior is fair, but the messaging from a database-wide
ANALYZE VERBOSE needs work:

INFO:  analyzing "test_foreign_inherit.parent"
INFO:  "parent": scanned 1 of 1 pages, containing 1 live rows and 0 dead rows; 1 rows in sample, 1 estimated total rows
INFO:  analyzing "test_foreign_inherit.parent" inheritance tree
WARNING:  relcache reference leak: relation "child" not closed
WARNING:  relcache reference leak: relation "tchild" not closed
WARNING:  relcache reference leak: relation "parent" not closed

Please arrange to omit the 'analyzing "tablename" inheritance tree' message,
since this ANALYZE actually skipped that task.  (The warnings obviously need a
fix, too.)  I do find it awkward that adding a foreign table to an inheritance
tree will make autovacuum stop collecting statistics on that inheritance tree,
but I can't think of a better policy.


The rest of these review comments are strictly observations; they're not
requests for you to change the patch, but they may deserve more discussion.

We use the term "child table" in many messages.  Should that change to
something more inclusive, now that the child may be a foreign table?  Perhaps
one of "child relation", plain "child", or "child foreign table"/"child table"
depending on the actual object?  "child relation" is robust technically, but
it might add more confusion than it removes.  Varying the message depending on
the actual object feels like a waste.  Opinions?

LOCK TABLE on the inheritance parent locks child foreign tables, but LOCK
TABLE fails when given a foreign table directly.  That's odd, but I see no
cause to change it.

A partition root only accepts an UPDATE command if every child is updatable.
Similarly, "UPDATE ... WHERE CURRENT OF cursor_name" fails if any child does
not support it.  That seems fine.  Incidentally, does anyone have a FDW that
supports WHERE CURRENT OF?


The longstanding behavior of CREATE TABLE INHERITS is to reorder local columns
to match the order found in parents.  That is, both of these tables actually
have columns in the order (a,b):

create table parent (a int, b int);
create table child (b int, a int) inherits (parent);

Ordinary dump/reload always uses CREATE TABLE INHERITS, thereby changing
column order in this way.  (pg_dump --binary-upgrade uses ALTER TABLE INHERIT
and some catalog hacks to avoid doing so.)  I've never liked that dump/reload
can change column order, but it's tolerable if you follow the best practice of
always writing out column lists.  The stakes rise for foreign tables, because
column order is inherently significant to file_fdw and probably to certain
other non-RDBMS FDWs.  If pg_dump changes column order in your file_fdw
foreign table, the table breaks.  I would heartily support making pg_dump
preserve column order for all inheritance children.  That doesn't rise to the
level of being a blocker for this patch, though.


I am attaching rough-hewn tests I used during this review.

Thanks,
nm

--
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com