Postgres Pro
Downloads
Home   >   mailing lists

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

From Ashutosh Bapat
Subject Re: inherit support for foreign tables
Date
Msg-id CAFjFpRcd=F8Jqx+9yioHKVUA71LXBxO41ia61acjzyzQ4MZqVQ@mail.gmail.com
Whole thread Raw
In response to Re: inherit support for foreign tables  (Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>)
Responses Re: inherit support for foreign tables  (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
List pgsql-hackers
Tree view
Hi Fujita-san,
Here are my review comments for patch fdw-inh-3.patch.

Sanity
--------
1. The patch applies cleanly
2. It compiles cleanly.
3. The server regression tests and tests in contrib modules pass.

Tests
-------
1. It seems like you have copied from testcase inherit.sql to postgres_fdw testcase. That's a good thing, but it makes the test quite long. May be we should have two tests in postgres_fdw contrib module, one for simple cases, and other for inheritance. What do you say?

Documentation
--------------------
1. The change in ddl.sgml
-        We will refer to the child tables as partitions, though they
-        are in every way normal <productname>PostgreSQL</> tables.
+        We will refer to the child tables as partitions, though we assume
+        that they are normal <productname>PostgreSQL</> tables.

adds phrase "we assume that", which confuses the intention behind the sentence. The original text is intended to highlight the equivalence between "partition" and "normal table", where as the addition esp. the word "assume" weakens that equivalence. Instead now we have to highlight the equivalence between "partition" and "normal or foreign table". The wording similar to "though they are either normal or foreign tables" should be used there.

2. The wording "some kind of optimization" gives vague picture. May be it can be worded as "Since the constraints are assumed to be true, they are used in constraint-based query optimization like constraint exclusion for partitioned tables.".
+    Those constraints are used in some kind of query optimization such
+    as constraint exclusion for partitioned tables (see
+    <xref linkend="ddl-partitioning">).

Code
-------
1. In the following change
+/*
  * acquire_inherited_sample_rows -- acquire sample rows from inheritance tree
  *
  * This has the same API as acquire_sample_rows, except that rows are
  * collected from all inheritance children as well as the specified table.
- * We fail and return zero if there are no inheritance children.
+ * We fail and return zero if there are no inheritance children or there are
+ * inheritance children that foreign tables.

The addition should be "there are inheritance children that are all foreign tables. Note the addition "are all".

2. The function has_foreign() be better named has_foreign_child()? This function loops through all the tableoids passed even after it has found a foreign table. Why can't we return true immediately after finding the first foreign table, unless the side effects of heap_open() on all the table are required. But I don't see that to be the case, since these tables are locked already through a previous call to heap_open(). In the same function instead of argument name parentrelId may be we should use name parent_oid, so that we use same notation for parent and child table OIDs.

3.  Regarding enum VacuumMode - it's being used only in case of acquire_inherited_sample_rows() and that too, to check only a single value of the three defined there. May be we should just infer that value inside acquire_inherited_sample_rows() or pass a boolean true or false from do_analyse_rel() based on the VacuumStmt. I do not see need for a separate three value enum of which only one value is used and also to pass it down from vacuum() by changing signatures of the minion functions.

4. In postgresGetForeignPlan(), the code added by this patch is required to handle the case when the row mark is placed on a parent table and hence is required to be applied on the child table. We need a comment explaining this. Otherwise, the three step process to get the row mark information isn't clear for a reader.

5. In expand_inherited_rtentry() why do you need a separate variable hasForeign? Instead of using that variable, you can actually set/reset rte->hasForeign since existence of even a single foreign child would mark that member as true. - After typing this, I got the answer when I looked at the function code. Every child's RTE is initially a copy of parent's RTE and hence hasForeign status would be inherited by every child after the first foreign child. I think, this reasoning should be added as comment before assignment to rte->hasForeign at the end of the function.

6. The tests in foreign_data.sql are pretty extensive. Thanks for that. I think, we should also check the effect of each of the following command using \d on appropriate tables.
+CREATE FOREIGN TABLE ft2 () INHERITS (pt1)
+  SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value');
+ALTER FOREIGN TABLE ft2 NO INHERIT pt1;
+DROP FOREIGN TABLE ft2;
+CREATE FOREIGN TABLE ft2 (
+   c1 integer NOT NULL,
+   c2 text,
+   c3 date
+) SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value');
+ALTER FOREIGN TABLE ft2 INHERIT pt1;

Rest of the changes look good.


On Thu, Nov 13, 2014 at 12:21 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote:


On Thu, Nov 13, 2014 at 12:20 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
Hi Ashutosh,

Thanks for the review!

(2014/11/13 15:23), Ashutosh Bapat wrote:
I tried to apply fdw-inh-3.patch on the latest head from master branch.
It failed to apply using both patch and git apply.

"patch" failed to apply because of rejections in
contrib/file_fdw/output/file_fdw.source and
doc/src/sgml/ref/create_foreign_table.sgml

 As I said upthread, fdw-inh-3.patch has been created on top of [1] and fdw-chk-3.patch.  Did you apply these patche first?

Oh, sorry, I didn't pay attention to that. I will apply both the patches and review the inheritance patch. Thanks for pointing that out.
 
[1] https://commitfest.postgresql.org/action/patch_view?id=1599

Best regards,
Etsuro Fujita



--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company



--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Next
From: Simon Riggs
Date:
Subject: Re: Index scan optimization