Re: inherit support for foreign tables

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