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

Hi Ashutosh,

Thanks for the review!

(2014/11/28 18:14), Ashutosh Bapat wrote:

On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
    (2014/11/17 17:55), Ashutosh Bapat wrote:
        Here are my review comments for patch fdw-inh-3.patch.

        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?

    IMO, the test is not so time-consuming, so it doesn't seem worth
    splitting it into two.

I am not worried about the timing but I am worried about the length of
the file and hence ease of debugging in case we find any issues there.
We will leave that for the commiter to decide.

OK

        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.

    You are right, but I feel that there is something out of place in
    saying that there (5.10.2. Implementing Partitioning) because the
    procedure there has been written based on normal tables only.  Put
    another way, if we say that, I think we'd need to add more docs,
    describing the syntax and/or the corresponding examples for
    foreign-table cases.  But I'd like to leave that for another patch.
    So, how about the wording "we assume *here* that", instead of "we
    assume that", as I added the following notice in the previous
    section (5.10.1. Overview)?

    @@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
          table of a single parent table.  The parent table itself is
    normally
          empty; it exists just to represent the entire data set.  You
    should be
          familiar with inheritance (see <xref linkend="ddl-inherit">)
    before
    -    attempting to set up partitioning.
    +    attempting to set up partitioning.  (The setup and management of
    +    partitioned tables illustrated in this section assume that each
    +    partition is a normal table.  However, you can do that in a
    similar way
    +    for cases where some or all partitions are foreign tables.)

This looks ok, though, I would like to see final version of the
document. But I think, we will leave that for committer to handle.

OK

        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">).

    Will follow your revision.

Done.

        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".

    Sorry, I incorrectly wrote the comment.  What I tried to write is
    "We fail and return zero if there are no inheritance children or if
    we are not in VAC_MODE_SINGLE case and inheritance tree contains at
    least one foreign table.".

You might want to use "English" description of VAC_MODE_SINGLE instead
of that macro in the comment, so that reader doesn't have to look up
VAC_MODE_SINGLE. But I think, we will leave this for the committer.

I corrected the comments and translated the macro into the English description.

        2. The function has_foreign() be better named
        has_foreign_child()? This

    How about "has_foreign_table"?

has_foreign_child() would be better, since these are "children" in the
inheritance hierarchy and not mere "table"s.

Done.  But I renamed it to has_foreign_children() because it sounds more natural at least to me.

        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

    Good catch!  Will fix.

        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.

    Will fix.

Done.

        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.

    I introduced that for possible future use.  See the discussion in [1].

Will leave it for the commiter to decide.

I noticed that the signatures need not to be modified, as you said. Thanks for pointing that out!  So, I revised the patch not to change the signatures, though I left the enum, renaming it to AnalyzeMode.  Let's have the committer's review.

        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.

    Will add the comment.

Done.

        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.

    As you mentioned, I think we could set rte->hasForeign directly
    during the scan for the inheritance set, without the separate
    variable, hasForeign.  But ISTM that it'd improve code readability
    to set rte->hasForeign using the separate variable at the end of the
    function because rte->hasForeign has its meaning only when rte->inh
    is true and because we know whether rte->inh is true, at the end of
    the function.

Fine. Please use "hasForeignChild" instead of just "hasForeign" without
a clue as to what is "foreign" here.

Done.  But I renamed it to "hasForeignChildren".

        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;

    Will fix.

Done.

    Apart from the above, I noticed that the patch doesn't consider to
    call ExplainForeignModify during EXPLAIN for an inherited
    UPDATE/DELETE, as shown below (note that there are no UPDATE remote
    queries displayed):

Since there seems to be no objections, I updated the patch as proposed upthread.  Here is an example.

postgres=# explain (format text, verbose) update parent as p set a = a * 2 returning *;
                                   QUERY PLAN
--------------------------------------------------------------------------------
 Update on public.parent p  (cost=0.00..202.33 rows=11 width=10)
   Output: p.a
   For public.ft1 p_1
     Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 RETURNING a
   For public.ft2 p_2
     Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1 RETURNING a
   ->  Seq Scan on public.parent p  (cost=0.00..0.00 rows=1 width=10)
         Output: (p.a * 2), p.ctid
   ->  Foreign Scan on public.ft1 p_1  (cost=100.00..101.16 rows=5 width=10)
         Output: (p_1.a * 2), p_1.ctid
         Remote SQL: SELECT a, ctid FROM public.mytable_1 FOR UPDATE
   ->  Foreign Scan on public.ft2 p_2  (cost=100.00..101.16 rows=5 width=10)
         Output: (p_2.a * 2), p_2.ctid
         Remote SQL: SELECT a, ctid FROM public.mytable_2 FOR UPDATE
(14 rows)

Other changes:
* revised regression tests for contrib/file_fdw to refer to tableoid.
* revised docs a bit further.

Attached are updated patches.  Patch fdw-inh-5.patch has been created on top of patch fdw-chk-5.patch.  Patch fdw-chk-5.patch is basically the same as the previous one fdw-chk-4.patch, but I slightly modified that one.  The changes are the following.
* added to foreign_data.sql more tests for your comments.
* revised docs on ALTER FOREIGN TABLE a bit further.

Thanks,

Best regards,
Etsuro Fujita