Re: Import Statistics in postgres_fdw before resorting to sampling. - Mailing list pgsql-hackers

CCing Jonathan Katz and Nathan Bossart as they had been sounding boards for me when I started designing this feature.

 

Returning in the FDW_IMPORT_STATS_OK case isn't 100% correct; if the
foreign table is an inheritance parent, we would fail to do inherited
stats.

Perhaps I'm not understanding completely, but I believe what we're doing now should be ok.

The local table of type 'f' can be a member of a partition, but can't be a partition itself, so whatever stats we get for it, we're storing them as inh = false.

On the remote side, the table could be an inheritance parent, in which case we ONLY want the inh=true stats, but we will still store them locally as inh = false.

The DISTINCT ON(a.attname)....ORDER BY a.attname, s.inherited DESC part of the query ensures that we get inh=true stats if they're there in preference to the inh=false steps.

I will grant you that in an old-style inheritance (i.e. not formal partitions) situation we'd probably want some weighted mix of the inherited and non-inherited stats, but 1) very few people use old-style inheritance anymore, 2) few of those export tables via a FDW, and 3) there's no way to do that weighting so we should fall back to sampling anyway.

None of this takes away from your suggestions down below.

 

IIUC, the FDW needs to do two things within the ImportStatistics
callback function: check the importability, and if ok, do the work.  I
think that would make the API complicated.  To avoid that, how about
1) splitting the callback function into the two functions shown below
and then 2) rewriting the above to something like the attached?  The
attached addresses the returning issue mentioned above as well.

bool
StatisticsAreImportable(Relation relation)

Checks the importability, and if ok, returns true, in which case the
following callback function is called.  In the postgres_fdw case, we
could implement this to check if the fetch_stats flag for the foreign
table is set to true, and if so, return true.

+1

 

void
ImportStatistics(Relation relation, List *va_cols, int elevel)

Imports the stats for the foreign table from the foreign server.  As
mentioned in previous emails, I don't think it's a good idea to fall
back to the normal processing when the attempt to import the stats
fails, in which case I think we should just throw an error (or
warning) so that the user can re-try to import the stats after fixing
the foreign side in some way.  So I re-defined this as a void
function.  Note that this re-definition removes the concern mentioned
in the comment starting with "XXX:".  In the postgres_fdw case, as
mentioned in a previous email, I think it would be good to implement
this so that it checks whether the remote table is a view or not when
importing the relation stats from the remote server, and if so, just
throws an error (or warning), making the user reset the fetch_stats
flag.

I think I'm ok with this design as the decision, as it still leaves open the fdw-specific options of how to handle initially finding no remote stats.

I can still see a situation where a local table expects the remote table to eventually have proper statistics on it, but until that happens it will fall back to table samples. This design decision means that either the user lives without any statistics for a while, or alters the foreign table options and hopefully remembers to set them back. While I understand the desire to first implement something very simple, I think that adding the durability that fallback allows for will be harder to implement if we don't build it in from the start. I'm interested to hear with Nathan and/or Jonathan have to say about that.

 

I added two arguments to the callback function: va_cols, for
supporting the column list in the ANALYZE command, and elevel, for
proper logging.  I'm not sure if VaccumParams should be added as well.

Good catch, I forgot about that one.

Going to think some more on this before I work incorporating your