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

(2014/08/22 11:51), Noah Misch wrote:
> On Wed, Aug 20, 2014 at 08:11:01PM +0900, Etsuro Fujita wrote:
>> (2014/07/02 11:23), Noah Misch wrote:
>>> 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

>>> Please arrange to omit the 'analyzing "tablename" inheritance tree' message,
>>> since this ANALYZE actually skipped that task.

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

> Today's ANALYZE VERBOSE messaging for former inheritance parents (tables with
> relhassubclass = true but no pg_inherits.inhparent links) is deceptive, and I
> welcome a fix to omit the spurious message.  As defects go, this is quite
> minor.  There's fundamentally no value in collecting inheritance tree
> statistics for such a parent, and no PostgreSQL command will do so.
>
> Your proposed behavior for inheritance parents having at least one foreign
> table child is more likely to mislead DBAs in practice.  An inheritance tree
> genuinely exists, and a different ANALYZE command is quite capable of
> collecting statistics on that inheritance tree.  Messaging should reflect the
> difference between ANALYZE invocations that do such work and ANALYZE
> invocations that skip it.  I'm anticipating a bug report along these lines:
>
>    I saw poor estimates involving a child foreign table, so I ran "ANALYZE
>    VERBOSE", which reported 'INFO:  analyzing "public.parent" inheritance
>    tree'.  Estimates remained poor, so I scratched my head for awhile before
>    noticing that pg_stats didn't report such statistics.  I then ran "ANALYZE
>    VERBOSE parent", saw the same 'INFO:  analyzing "public.parent" inheritance
>    tree', but this time saw relevant rows in pg_stats.  The message doesn't
>    reflect the actual behavior.
>
> I'll sympathize with that complaint.  It's a minor point overall, but the code
> impact is, I predict, small enough that we may as well get it right.  A
> credible alternative is to emit a second message indicating that we skipped
> the inheritance tree statistics after all, and why we skipped them.

I'd like to address this by emitting the second message as shown below:

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:  skipping analyze of "public.parent" inheritance tree --- this
inheritance tree contains foreign tables

Attached is the update version of the patch.  Based on the result of
discussions about attr_needed upthread, I've changed the patch so that
create_foreignscan_plan makes reference to reltargetlist, not to
attr_needed.  (So, the patch in [1] isn't required, anymore.)

Other changes:
* Revise code/comments/docs a bit
* Add more regression tests

A separate patch (analyze.patch) handles the former case in a similar way.

[1] http://www.postgresql.org/message-id/53F4707C.8030904@lab.ntt.co.jp

Thanks,

Best regards,
Etsuro Fujita