Re: inherit support for foreign tables

(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