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
drop schema test_foreign_inherit cascade;
create schema test_foreign_inherit;
set search_path = test_foreign_inherit;
create extension postgres_fdw;
CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (dbname 'test');
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
create extension file_fdw;
CREATE SERVER f FOREIGN DATA WRAPPER file_fdw;
create table parent (
a int,
b text,
c date,
d path
);
insert into parent values (4, 'foo', current_date, '0,0,1,2');
-- regular child
create table tchild () inherits (parent);
insert into tchild values (5, 'oof', current_date - 2, '-1,-1,0,0');
-- cursor update works
begin;
declare c cursor for select 1 from parent where b = 'oof';
fetch from c;
explain analyze update parent set b = null where current of c;
select * from parent;
rollback;
-- "foreign" side
create table _child (
c date,
b text,
extra numeric,
d path,
a int
);
-- Foreign child. No column reorder.
create foreign table child (
c date,
b text,
extra numeric,
d path,
a int
) SERVER LOOPBACK OPTIONS (table_name '_child');
alter table child inherit parent;
insert into child (a, b, c, d, extra)
values (7, 'bar', current_date - 1, '5,5,9,9', 1.1);
\copy (select * from child) to /tmp/child.pgcopy
-- update works
select * from parent;
update parent set b = 'baz' where b = 'bar';
-- cursor update fails, because postgres_fdw doesn't handle WHERE CURRENT OF
begin;
declare c cursor for select 1 from child where b = 'baz';
fetch from c;
update child set b = 'bak' where current of c;
rollback;
-- cursor update fails, even though target isn't in the foreign table
begin;
declare c cursor for select 1 from parent where b = 'oof';
fetch from c;
update parent set b = null where current of c;
rollback;
-- Add a file_fdw child. No reorder.
create foreign table fchild0 (
c date,
b text,
extra numeric,
d path,
a int
) SERVER f OPTIONS (filename '/tmp/child.pgcopy');
alter table fchild0 inherit parent;
-- no more update, since file_fdw doesn't cooperate
select * from parent;
update parent set b = 'baz' where b = 'foo';
select * from child for update of child;
select * from fchild0 for update of fchild0;
select * from parent for update;
-- No LOCK TABLE for foreign tables, ...
begin; lock table child in exclusive mode; rollback;
-- ... but locking indirectly via a parent works.
begin;
select relation::regclass, mode from pg_locks
where mode = 'ExclusiveLock' and relation is not null order by 1;
lock table parent in exclusive mode;
select relation::regclass, mode from pg_locks
where mode = 'ExclusiveLock' and relation is not null order by 1;
rollback;
alter server loopback options (use_remote_estimate 'true');
select * from parent;
analyze verbose;
analyze verbose parent;
-- message correctly observes that foreign table is now allowed
create view v as select 1;
alter table v inherit parent;
-- for testing dump/reload of column-level options
create foreign table nonlocal () inherits (parent)
server loopback options (table_name '_child');
alter foreign table child alter c options (column_name 'c');
alter foreign table nonlocal alter c options (column_name 'c');
-- column reorder breaks the foreign table
create foreign table fchild1 (
c date,
b text,
extra numeric,
d path,
a int
) inherits (parent) SERVER f OPTIONS (filename '/tmp/child.pgcopy');
select * from fchild1;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
