Re: inherit support for foreign tables - Mailing list pgsql-hackers
From | Etsuro Fujita |
---|---|
Subject | Re: inherit support for foreign tables |
Date | |
Msg-id | 53F48245.6020204@lab.ntt.co.jp Whole thread Raw |
In response to | Re: inherit support for foreign tables (Noah Misch <noah@leadboat.com>) |
Responses |
Re: inherit support for foreign tables
|
List | pgsql-hackers |
Hi Noah, Thank you for the review! (2014/07/02 11:23), Noah Misch wrote: > 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. To fix this, I've modified the planner and executor so that the planner adds wholerow as well as ctid and tableoid as resjunk output columns to the plan for an inheritance tree that contains foreign tables, and that while the executor uses the ctid and tableoid in the EPQ processing for child regular tables as before, the executor uses the wholerow and tableoid for child foreign tables. Patch attached. This is based on the patch [1]. > 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. Done. I used your tests as reference. Thanks! > The inheritance section of ddl.sgml should mention child foreign tables, at > least briefly. Consider mentioning it in the partitioning section, too. Done. > 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. I think it would be better that this is handled in the same way as an inheritance tree that turns out to be a singe table that doesn't have any descendants in acquire_inherited_sample_rows(). That would still output the message as shown below, but I think that that would be more consistent with the existing code. The patch works so. (The warnings are also fixed.) INFO: analyzing "public.parent" INFO: "parent": scanned 0 of 0 pages, containing 0 live rows and 0 dead rows; 0 rows in sample, 0 estimated total rows INFO: analyzing "public.parent" inheritance tree INFO: analyzing "pg_catalog.pg_authid" INFO: "pg_authid": scanned 1 of 1 pages, containing 1 live rows and 0 dead rows; 1 rows in sample, 1 estimated total rows > 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. I'd like to give my opinions on those things later on. Sorry for the long delay. [1] http://www.postgresql.org/message-id/53F4707C.8030904@lab.ntt.co.jp Best regards, Etsuro Fujita
Attachment
pgsql-hackers by date: