Thread: Ideas about a better API for postgres_fdw remote estimates

In <1116564.1593813043@sss.pgh.pa.us> I wrote:
> I wonder whether someday we ought to invent a new API that's more
> suited to postgres_fdw's needs than EXPLAIN is.  It's not like the
> remote planner doesn't know the number we want; it just fails to
> include it in EXPLAIN.

I've been thinking about this a little more, and I'd like to get some
ideas down on electrons before they vanish.

The current method for postgres_fdw to obtain remote estimates is to
issue an EXPLAIN command to the remote server and then decipher the
result.  This has just one big advantage, which is that it works
against existing, even very old, remote PG versions.  In every other
way it's pretty awful: it involves a lot of cycles on the far end
to create output details we don't really care about, it requires a
fair amount of logic to parse that output, and we can't get some
details that we *do* care about (such as the total size of the foreign
table, as per the other discussion).

We can do better.  I don't propose removing the existing logic, because
being able to work against old remote PG versions seems pretty useful.
But we could probe at connection start for whether the remote server
has support for a better way, and then use that way if available.

What should the better way look like?  I suggest the following:

* Rather than adding a core-server feature, the remote-end part of the new
API should be a function installed by an extension (either postgres_fdw
itself, or a new extension "postgres_fdw_remote" or the like).  One
attraction of this approach is that it'd be conceivable to back-port the
new code into existing PG releases by updating the extension.  Also
there'd be room for multiple versions of the support.  The
connection-start probe could be of the form "does this function exist
in pg_proc?".

* I'm imagining the function being of the form

    function pg_catalog.postgres_fdw_support(query text) returns something

where the input is still the text of a query we're considering issuing,
and the output is some structure that contains the items of EXPLAIN-like
data we need, but not the items we don't.  The implementation of the
function would run the query through parse/plan, then pick out the
data we want and return that.

* We could do a lot worse than to have the "structure" be JSON.
This'd allow structured, labeled data to be returned; it would not be
too difficult to construct, even in PG server versions predating the
addition of JSON logic to the core; and the receiving postgres_fdw
extension could use the core's JSON logic to parse the data.

* The contents of the structure need to be designed with forethought
for extensibility, but this doesn't seem hard if it's all a collection
of labeled fields.  We can just say that the recipient must ignore
fields it doesn't recognize.  Once a given field has been defined, we
can't change its contents, but we can introduce new fields as needed.
Note that I would not be in favor of putting an overall version number
within the structure; that's way too coarse-grained.

I'm not planning to do anything about these ideas myself, at least
not in the short term.  But perhaps somebody else would like to
run with them.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> In <1116564.1593813043@sss.pgh.pa.us> I wrote:
> > I wonder whether someday we ought to invent a new API that's more
> > suited to postgres_fdw's needs than EXPLAIN is.  It's not like the
> > remote planner doesn't know the number we want; it just fails to
> > include it in EXPLAIN.
>
> I've been thinking about this a little more, and I'd like to get some
> ideas down on electrons before they vanish.
>
> The current method for postgres_fdw to obtain remote estimates is to
> issue an EXPLAIN command to the remote server and then decipher the
> result.  This has just one big advantage, which is that it works
> against existing, even very old, remote PG versions.  In every other
> way it's pretty awful: it involves a lot of cycles on the far end
> to create output details we don't really care about, it requires a
> fair amount of logic to parse that output, and we can't get some
> details that we *do* care about (such as the total size of the foreign
> table, as per the other discussion).
>
> We can do better.  I don't propose removing the existing logic, because
> being able to work against old remote PG versions seems pretty useful.
> But we could probe at connection start for whether the remote server
> has support for a better way, and then use that way if available.

I agree we can, and should, try to do better here.

> What should the better way look like?  I suggest the following:
>
> * Rather than adding a core-server feature, the remote-end part of the new
> API should be a function installed by an extension (either postgres_fdw
> itself, or a new extension "postgres_fdw_remote" or the like).  One
> attraction of this approach is that it'd be conceivable to back-port the
> new code into existing PG releases by updating the extension.  Also
> there'd be room for multiple versions of the support.  The
> connection-start probe could be of the form "does this function exist
> in pg_proc?".
>
> * I'm imagining the function being of the form
>
>     function pg_catalog.postgres_fdw_support(query text) returns something
>
> where the input is still the text of a query we're considering issuing,
> and the output is some structure that contains the items of EXPLAIN-like
> data we need, but not the items we don't.  The implementation of the
> function would run the query through parse/plan, then pick out the
> data we want and return that.
>
> * We could do a lot worse than to have the "structure" be JSON.
> This'd allow structured, labeled data to be returned; it would not be
> too difficult to construct, even in PG server versions predating the
> addition of JSON logic to the core; and the receiving postgres_fdw
> extension could use the core's JSON logic to parse the data.

I also tend to agree with using JSON for this.

> * The contents of the structure need to be designed with forethought
> for extensibility, but this doesn't seem hard if it's all a collection
> of labeled fields.  We can just say that the recipient must ignore
> fields it doesn't recognize.  Once a given field has been defined, we
> can't change its contents, but we can introduce new fields as needed.
> Note that I would not be in favor of putting an overall version number
> within the structure; that's way too coarse-grained.

This also makes sense to me.

> I'm not planning to do anything about these ideas myself, at least
> not in the short term.  But perhaps somebody else would like to
> run with them.

I'm trying to figure out why it makes more sense to use
'postgres_fdw_support(query text)', which would still do parse/plan and
return EXPLAIN-like data, rather than having:

EXPLAIN (FORMAT JSON, FDW true) query ...

(Or, perhaps better, individual boolean options for whatever stuff we
want to ask for, or to exclude if we don't want it, so that other tools
could use this...).

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> * Rather than adding a core-server feature, the remote-end part of the new
>> API should be a function installed by an extension (either postgres_fdw
>> itself, or a new extension "postgres_fdw_remote" or the like).

> I'm trying to figure out why it makes more sense to use
> 'postgres_fdw_support(query text)', which would still do parse/plan and
> return EXPLAIN-like data, rather than having:

> EXPLAIN (FORMAT JSON, FDW true) query ...

I see a couple of reasons not to do it like that:

1. This is specific to postgres_fdw.  Some other extension might want some
other data, and different versions of postgres_fdw might want different
data.  So putting it into core seems like the wrong thing.

2. Wedging this into EXPLAIN would be quite ugly, because (at least
as I envision it) the output would have just about nothing to do with
any existing EXPLAIN output.

3. We surely would not back-patch a core change like this.  OTOH, if
the added infrastructure is in an extension, somebody might want to
back-patch that (even if unofficially).  This argument falls to the
ground of course if we're forced to make any core changes to be able
to get at the data we need; but I'm not sure that will be needed.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> * Rather than adding a core-server feature, the remote-end part of the new
> >> API should be a function installed by an extension (either postgres_fdw
> >> itself, or a new extension "postgres_fdw_remote" or the like).
>
> > I'm trying to figure out why it makes more sense to use
> > 'postgres_fdw_support(query text)', which would still do parse/plan and
> > return EXPLAIN-like data, rather than having:
>
> > EXPLAIN (FORMAT JSON, FDW true) query ...
>
> I see a couple of reasons not to do it like that:
>
> 1. This is specific to postgres_fdw.  Some other extension might want some
> other data, and different versions of postgres_fdw might want different
> data.  So putting it into core seems like the wrong thing.

Another extension or use-case might want exactly the same information
too though.  In a way, we'd be 'hiding' that information from other
potential users unless they want to install their own extension, which
is a pretty big leap.  Are we sure this information wouldn't be at all
interesting to pgAdmin4 or explain.depesz.com?

> 2. Wedging this into EXPLAIN would be quite ugly, because (at least
> as I envision it) the output would have just about nothing to do with
> any existing EXPLAIN output.

This is a better argument for not making it part of EXPLAIN, though I
don't really feel like I've got a decent idea of what you are suggesting
the output *would* look like, so it's difficult for me to agree (or
disagree) about this particular point.

> 3. We surely would not back-patch a core change like this.  OTOH, if
> the added infrastructure is in an extension, somebody might want to
> back-patch that (even if unofficially).  This argument falls to the
> ground of course if we're forced to make any core changes to be able
> to get at the data we need; but I'm not sure that will be needed.

Since postgres_fdw is part of core and core's release cycle, and the
packagers manage the extensions from core in a way that they have to
match up, this argument doesn't hold any weight with me.  For this to be
a viable argument, we would need to segregate extensions from core and
give them their own release cycle and clear indication of which
extension versions work with which PG major versions, etc.  I'm actually
generally in support of *that* idea- and with that, would agree with
your point 3 above, but that's not the reality of today.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> 2. Wedging this into EXPLAIN would be quite ugly, because (at least
>> as I envision it) the output would have just about nothing to do with
>> any existing EXPLAIN output.

> This is a better argument for not making it part of EXPLAIN, though I
> don't really feel like I've got a decent idea of what you are suggesting
> the output *would* look like, so it's difficult for me to agree (or
> disagree) about this particular point.

Per postgres_fdw's get_remote_estimate(), the only data we use right now
is the startup_cost, total_cost, rows and width estimates from the
top-level Plan node.  That's available immediately from the Plan tree,
meaning that basically *nothing* of the substantial display effort
expended by explain.c and ruleutils.c is of any value.  So the level-zero
implementation of this would be to run the parser and planner, format
those four numbers into a JSON object (which would require little more
infrastructure than sprintf), and return that.  Sure, we could make that
into some kind of early-exit path in explain.c, but I think it'd be a
pretty substantial wart, especially since it'd mean that none of the
other EXPLAIN options are sensible in combination with this one.

Further down the road, we might want to rethink the whole idea of
completely constructing a concrete Plan.  We could get the data we need
at the list-of-Paths stage.  Even more interesting, we could (with very
little more work) return data about multiple Paths, so that the client
could find out, for example, the costs of sorted and unsorted output
without paying two network round trips to discover that.  That'd
definitely require changes in the core planner, since it has no API to
stop at that point.  And it's even less within the charter of EXPLAIN.

I grant your point that there might be other users for this besides
postgres_fdw, but that doesn't mean it must be a core feature.

>> 3. We surely would not back-patch a core change like this.  OTOH, if
>> the added infrastructure is in an extension, somebody might want to
>> back-patch that (even if unofficially).

> Since postgres_fdw is part of core and core's release cycle, and the
> packagers manage the extensions from core in a way that they have to
> match up, this argument doesn't hold any weight with me.

Certainly only v14 (or whenever) and later postgres_fdw would be able
to *use* this data.  The scenario I'm imagining is that somebody wants
to be able to use that client against an older remote server, and is
willing to install some simple extension on the remote server to do so.
Perhaps this scenario is not worth troubling over, but I don't think
it's entirely far-fetched.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> 2. Wedging this into EXPLAIN would be quite ugly, because (at least
> >> as I envision it) the output would have just about nothing to do with
> >> any existing EXPLAIN output.
>
> > This is a better argument for not making it part of EXPLAIN, though I
> > don't really feel like I've got a decent idea of what you are suggesting
> > the output *would* look like, so it's difficult for me to agree (or
> > disagree) about this particular point.
>
> Per postgres_fdw's get_remote_estimate(), the only data we use right now
> is the startup_cost, total_cost, rows and width estimates from the
> top-level Plan node.  That's available immediately from the Plan tree,
> meaning that basically *nothing* of the substantial display effort
> expended by explain.c and ruleutils.c is of any value.  So the level-zero

The 'display effort' you're referring to, when using JSON format with
explain, is basically to format the results into JSON and return them-
which is what you're suggesting this mode would do anyway, no..?

If the remote side 'table' is actually a view that's complicated then
having a way to get just the top-level information (and excluding the
rest) sounds like it'd be useful and perhaps excluding that other info
doesn't really fit into EXPLAIN's mandate, but that's also much less
common.

> implementation of this would be to run the parser and planner, format
> those four numbers into a JSON object (which would require little more
> infrastructure than sprintf), and return that.  Sure, we could make that
> into some kind of early-exit path in explain.c, but I think it'd be a
> pretty substantial wart, especially since it'd mean that none of the
> other EXPLAIN options are sensible in combination with this one.

That EXPLAIN has options that only make sense in combination with
certain other options isn't anything new- BUFFERS makes no sense without
ANALYZE, etc.

> Further down the road, we might want to rethink the whole idea of
> completely constructing a concrete Plan.  We could get the data we need
> at the list-of-Paths stage.  Even more interesting, we could (with very
> little more work) return data about multiple Paths, so that the client
> could find out, for example, the costs of sorted and unsorted output
> without paying two network round trips to discover that.  That'd
> definitely require changes in the core planner, since it has no API to
> stop at that point.  And it's even less within the charter of EXPLAIN.

I have to admit that I'm not really sure how we could make it work, but
having a way to get multiple paths returned by EXPLAIN would certainly
be interesting to a lot of users.  Certainly it's easier to see how we
could get at that info in a postgres_fdw-specific function, and be able
to understand how to deal with it there and what could be done, but once
it's there I wonder if other tools might see that and possibly even
build on it because it'd be the only way to get that kind of info, which
certainly wouldn't be ideal.

> I grant your point that there might be other users for this besides
> postgres_fdw, but that doesn't mean it must be a core feature.

That postgres_fdw is an extension is almost as much of a wart as
anything being discussed here and suggesting that things added to
postgres_fdw aren't 'core features' seems akin to ignoring the forest
for the trees- consider that, today, there isn't even an option to
install only the core server from the PGDG repos (at least for Debian /
Ubuntu, not sure if the RPMs have caught up to that yet, but they
probably should).  The 'postgresql-12' .deb includes all the extensions
that are part of the core git repo, because they're released and
maintained just the same as the core server and, from a practical
perspective, to run a decent PG system you really should have them
installed, so why bother having a separate package?

> >> 3. We surely would not back-patch a core change like this.  OTOH, if
> >> the added infrastructure is in an extension, somebody might want to
> >> back-patch that (even if unofficially).
>
> > Since postgres_fdw is part of core and core's release cycle, and the
> > packagers manage the extensions from core in a way that they have to
> > match up, this argument doesn't hold any weight with me.
>
> Certainly only v14 (or whenever) and later postgres_fdw would be able
> to *use* this data.  The scenario I'm imagining is that somebody wants
> to be able to use that client against an older remote server, and is
> willing to install some simple extension on the remote server to do so.
> Perhaps this scenario is not worth troubling over, but I don't think
> it's entirely far-fetched.

I definitely don't think that such an extension should be maintained
outside of core, and I seriously doubt any of our packagers would be
anxious to build an indepedent package for this to be usable in older
servers.  Sure, it's possible someone will care about this enough to
spend the effort to try and build it for an older version and use it but
I definitely don't think we should be considering that a serious design
goal or a reason to put this capability in a separate extension.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Per postgres_fdw's get_remote_estimate(), the only data we use right now
>> is the startup_cost, total_cost, rows and width estimates from the
>> top-level Plan node.  That's available immediately from the Plan tree,
>> meaning that basically *nothing* of the substantial display effort
>> expended by explain.c and ruleutils.c is of any value.  So the level-zero

> The 'display effort' you're referring to, when using JSON format with
> explain, is basically to format the results into JSON and return them-
> which is what you're suggesting this mode would do anyway, no..?

Not hardly.  Spend some time studying ruleutils.c sometime ---
reverse-compiling a plan is *expensive*.  For instance, we have to
look up the names of all the operators used in the query quals,
decide what needs quoting, decide what needs parenthesization, etc.
There's also a fun little bit that assigns unique aliases to each
table appearing in the query, which from memory is at least O(N^2)
and maybe worse.  (Admittedly, shipped queries are usually not so
complicated that N would be large.)  And by the way, we're also
starting up the executor, even if you didn't say ANALYZE.

A little bit of fooling with "perf" suggests that when explaining
a pretty simple bitmapscan query --- I used
    EXPLAIN SELECT * FROM tenk1 WHERE unique1 > 9995
which ought to be somewhat representative of what postgres_fdw needs
--- only about half of the runtime is spent within pg_plan_query, and
the other half is spent on explain.c + ruleutils.c formatting work.
So while getting rid of that overhead wouldn't be an earthshattering
improvement, I think it'd be worthwhile.

>> Further down the road, we might want to rethink the whole idea of
>> completely constructing a concrete Plan.  We could get the data we need
>> at the list-of-Paths stage.  Even more interesting, we could (with very
>> little more work) return data about multiple Paths, so that the client
>> could find out, for example, the costs of sorted and unsorted output
>> without paying two network round trips to discover that.

> I have to admit that I'm not really sure how we could make it work, but
> having a way to get multiple paths returned by EXPLAIN would certainly
> be interesting to a lot of users.  Certainly it's easier to see how we
> could get at that info in a postgres_fdw-specific function, and be able
> to understand how to deal with it there and what could be done, but once
> it's there I wonder if other tools might see that and possibly even
> build on it because it'd be the only way to get that kind of info, which
> certainly wouldn't be ideal.

Yeah, thinking about it as a function that inspects partial planner
results, it might be useful for other purposes besides postgres_fdw.
As I said before, I don't think this necessarily has to be bundled as
part of postgres_fdw.  That still doesn't make it part of EXPLAIN.

> That postgres_fdw is an extension is almost as much of a wart as
> anything being discussed here and suggesting that things added to
> postgres_fdw aren't 'core features' seems akin to ignoring the forest
> for the trees-

I think we just had this discussion in another thread.  The fact that
postgres_fdw is an extension is a feature, not a bug, because (a) it
means that somebody could implement their own version if they wanted
it to act differently; and (b) it keeps us honest about whether the
APIs needed by an FDW are accessible from outside core.  I think moving
postgres_fdw into core would be a large step backwards.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> Per postgres_fdw's get_remote_estimate(), the only data we use right now
> >> is the startup_cost, total_cost, rows and width estimates from the
> >> top-level Plan node.  That's available immediately from the Plan tree,
> >> meaning that basically *nothing* of the substantial display effort
> >> expended by explain.c and ruleutils.c is of any value.  So the level-zero
>
> > The 'display effort' you're referring to, when using JSON format with
> > explain, is basically to format the results into JSON and return them-
> > which is what you're suggesting this mode would do anyway, no..?
>
> Not hardly.  Spend some time studying ruleutils.c sometime ---
> reverse-compiling a plan is *expensive*.  For instance, we have to
> look up the names of all the operators used in the query quals,
> decide what needs quoting, decide what needs parenthesization, etc.

Ah, alright, that makes more sense then.

> There's also a fun little bit that assigns unique aliases to each
> table appearing in the query, which from memory is at least O(N^2)
> and maybe worse.  (Admittedly, shipped queries are usually not so
> complicated that N would be large.)  And by the way, we're also
> starting up the executor, even if you didn't say ANALYZE.
>
> A little bit of fooling with "perf" suggests that when explaining
> a pretty simple bitmapscan query --- I used
>     EXPLAIN SELECT * FROM tenk1 WHERE unique1 > 9995
> which ought to be somewhat representative of what postgres_fdw needs
> --- only about half of the runtime is spent within pg_plan_query, and
> the other half is spent on explain.c + ruleutils.c formatting work.
> So while getting rid of that overhead wouldn't be an earthshattering
> improvement, I think it'd be worthwhile.

Sure.

> >> Further down the road, we might want to rethink the whole idea of
> >> completely constructing a concrete Plan.  We could get the data we need
> >> at the list-of-Paths stage.  Even more interesting, we could (with very
> >> little more work) return data about multiple Paths, so that the client
> >> could find out, for example, the costs of sorted and unsorted output
> >> without paying two network round trips to discover that.
>
> > I have to admit that I'm not really sure how we could make it work, but
> > having a way to get multiple paths returned by EXPLAIN would certainly
> > be interesting to a lot of users.  Certainly it's easier to see how we
> > could get at that info in a postgres_fdw-specific function, and be able
> > to understand how to deal with it there and what could be done, but once
> > it's there I wonder if other tools might see that and possibly even
> > build on it because it'd be the only way to get that kind of info, which
> > certainly wouldn't be ideal.
>
> Yeah, thinking about it as a function that inspects partial planner
> results, it might be useful for other purposes besides postgres_fdw.
> As I said before, I don't think this necessarily has to be bundled as
> part of postgres_fdw.  That still doesn't make it part of EXPLAIN.

Providing it as a function rather than through EXPLAIN does make a bit
more sense if we're going to skip things like the lookups you mention
above.  I'm still inclined to have it be a part of core rather than
having it as postgres_fdw though.  I'm not completely against it being
part of postgres_fdw... but I would think that would really be
appropriate if it's actually using something in postgres_fdw, but if
everything that it's doing is part of core and nothing related
specifically to the postgres FDW, then having it as part of core makes
more sense to me.  Also, having it as part of core would make it more
appropriate for other tools to look at and adding that kind of
inspection capability for partial planner results could be very
interesting for tools like pgAdmin and such.

> > That postgres_fdw is an extension is almost as much of a wart as
> > anything being discussed here and suggesting that things added to
> > postgres_fdw aren't 'core features' seems akin to ignoring the forest
> > for the trees-
>
> I think we just had this discussion in another thread.  The fact that
> postgres_fdw is an extension is a feature, not a bug, because (a) it
> means that somebody could implement their own version if they wanted
> it to act differently; and (b) it keeps us honest about whether the
> APIs needed by an FDW are accessible from outside core.  I think moving
> postgres_fdw into core would be a large step backwards.

I'm not looking to change it today, as that ship has sailed, but while
having FDWs as a general capability that can be implemented by
extensions is certainly great and I'd love to see more of that (even
better would be more of those that are well maintained and cared for by
this community of folks), requiring users to install an extension into
every database where they want to query another PG server from isn't a
feature.

Thanks,

Stephen

On Mon, Jul  6, 2020 at 11:28:28AM -0400, Stephen Frost wrote:
> > Yeah, thinking about it as a function that inspects partial planner
> > results, it might be useful for other purposes besides postgres_fdw.
> > As I said before, I don't think this necessarily has to be bundled as
> > part of postgres_fdw.  That still doesn't make it part of EXPLAIN.
> 
> Providing it as a function rather than through EXPLAIN does make a bit
> more sense if we're going to skip things like the lookups you mention
> above.  I'm still inclined to have it be a part of core rather than
> having it as postgres_fdw though.  I'm not completely against it being
> part of postgres_fdw... but I would think that would really be
> appropriate if it's actually using something in postgres_fdw, but if
> everything that it's doing is part of core and nothing related
> specifically to the postgres FDW, then having it as part of core makes
> more sense to me.  Also, having it as part of core would make it more
> appropriate for other tools to look at and adding that kind of
> inspection capability for partial planner results could be very
> interesting for tools like pgAdmin and such.

I agree the statistics extraction should probably be part of core. 
There is the goal if FDWs returning data, and returning the data
quickly.  I think we can require all-new FDW servers to get improved
performance.  I am not even clear if we have a full understanding of the
performance characteristics of FDWs yet.  I know Tomas did some research
on its DML behavior, but other than that, I haven't seen much.

On a related note, I have wished to be able to see all the costs
associated with plans not chosen, and I think others would like that as
well.  Getting multiple costs for a query goes in that direction.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

On 7/14/20 6:32 AM, Bruce Momjian wrote:
> On Mon, Jul  6, 2020 at 11:28:28AM -0400, Stephen Frost wrote:
>>> Yeah, thinking about it as a function that inspects partial planner
>>> results, it might be useful for other purposes besides postgres_fdw.
>>> As I said before, I don't think this necessarily has to be bundled as
>>> part of postgres_fdw.  That still doesn't make it part of EXPLAIN.
>>
>> Providing it as a function rather than through EXPLAIN does make a bit
>> more sense if we're going to skip things like the lookups you mention
>> above.  I'm still inclined to have it be a part of core rather than
>> having it as postgres_fdw though.  I'm not completely against it being
>> part of postgres_fdw... but I would think that would really be
>> appropriate if it's actually using something in postgres_fdw, but if
>> everything that it's doing is part of core and nothing related
>> specifically to the postgres FDW, then having it as part of core makes
>> more sense to me.  Also, having it as part of core would make it more
>> appropriate for other tools to look at and adding that kind of
>> inspection capability for partial planner results could be very
>> interesting for tools like pgAdmin and such.
> 
> I agree the statistics extraction should probably be part of core.
> There is the goal if FDWs returning data, and returning the data
> quickly.  I think we can require all-new FDW servers to get improved
> performance.  I am not even clear if we have a full understanding of the
> performance characteristics of FDWs yet.  I know Tomas did some research
> on its DML behavior, but other than that, I haven't seen much.
> 
> On a related note, I have wished to be able to see all the costs
> associated with plans not chosen, and I think others would like that as
> well.  Getting multiple costs for a query goes in that direction.
> 

During the implementation of sharding related improvements i noticed 
that if we use a lot of foreign partitions, we have bad plans because of 
vacuum don't update statistics of foreign tables.This is done by the 
ANALYZE command, but it is very expensive operation for foreign table.
Problem with statistics demonstrates with TAP-test from the first patch 
in attachment.

I implemented some FDW + pg core machinery to reduce weight of the 
problem. The ANALYZE command on foreign table executes query on foreign 
server that extracts statistics tuple, serializes it into json-formatted 
string and returns to the caller. The caller deserializes this string, 
generates statistics for this foreign table and update it. The second 
patch is a proof-of-concept.

This patch speedup analyze command and provides statistics relevance on 
a foreign table after autovacuum operation. Its effectiveness depends on 
relevance of statistics on the remote server, but still.

-- 
regards,
Andrey Lepikhov
Postgres Professional

Stephen Frost <sfrost@snowman.net> writes:
> Isn't this going to create a version dependency that we'll need to deal
> with..?  What if a newer major version has some kind of improved ANALYZE
> command, in terms of what it looks at or stores, and it's talking to an
> older server?

Yeah, this proposal is a nonstarter unless it can deal with the remote
server being a different PG version with different stats.

Years ago (when I was still at Salesforce, IIRC, so ~5 years) we had
some discussions about making it possible for pg_dump and/or pg_upgrade
to propagate stats data forward to the new database.  There is at least
one POC patch in the archives for doing that by dumping the stats data
wrapped in a function call, where the target database's version of the
function would be responsible for adapting the data if necessary, or
maybe just discarding it if it couldn't adapt.  We seem to have lost
interest but it still seems like something worth pursuing.  I'd guess
that if such infrastructure existed it could be helpful for this.

> When I was considering the issue with ANALYZE and FDWs, I had been
> thinking it'd make sense to just change the query that's built in
> deparseAnalyzeSql() to have a TABLESAMPLE clause, but otherwise run in
> more-or-less the same manner as today.

+1, that seems like something worth doing in any case, since even if
we do get somewhere with the present idea it would only work for new
remote servers.  TABLESAMPLE would work pretty far back (9.5,
looks like).

            regards, tom lane

On 8/29/20 9:22 PM, Stephen Frost wrote:
> 
>> I implemented some FDW + pg core machinery to reduce weight of the problem.
>> The ANALYZE command on foreign table executes query on foreign server that
>> extracts statistics tuple, serializes it into json-formatted string and
>> returns to the caller. The caller deserializes this string, generates
>> statistics for this foreign table and update it. The second patch is a
>> proof-of-concept.
> 
> Isn't this going to create a version dependency that we'll need to deal
> with..?  What if a newer major version has some kind of improved ANALYZE
> command, in terms of what it looks at or stores, and it's talking to an
> older server?
> 
> When I was considering the issue with ANALYZE and FDWs, I had been
> thinking it'd make sense to just change the query that's built in
> deparseAnalyzeSql() to have a TABLESAMPLE clause, but otherwise run in
> more-or-less the same manner as today.  If we don't like the available
> TABLESAMPLE methods then we could add a new one that's explicitly the
> 'right' sample for an ANALYZE call and use that when it's available on
> the remote side.  Not sure if it'd make any sense for ANALYZE itself to
> start using that same TABLESAMPLE code, but maybe?  Not that I think
> it'd be much of an issue if it's independent either, with appropriate
> comments to note that we should probably try to make them match up for
> the sake of FDWs.
This approach does not contradict your idea.  This is a lightweight 
opportunity to reduce the cost of analysis if we have a set of servers 
with actual versions of system catalog and fdw.
> 
>> This patch speedup analyze command and provides statistics relevance on a
>> foreign table after autovacuum operation. Its effectiveness depends on
>> relevance of statistics on the remote server, but still.
> 
> If we do decide to go down this route, wouldn't it mean we'd have to
> solve the problem of what to do when it's a 9.6 foreign server being
> queried from a v12 server and dealing with any difference in the
> statistics structures of the two?
> 
> Seems like we would... in which case I would say that we should pull
> that bit out and make it general, and use it for pg_upgrade too, which
> would benefit a great deal from having the ability to upgrade stats
> between major versions also.  That's a much bigger piece to take on, of
> course, but seems to be what's implied with this approach for the FDW.
> 

Thank you for this use case.

We can add field "version" to statistics string (btree uses versioning 
too). As you can see, in this patch we are only trying to get 
statistics. If for some reason this does not work out, then we go along 
a difficult route.

Moreover,  I believe this strategy should only work if we analyze a 
relation implicitly. If the user executes analysis explicitly by the 
command "ANALYZE <relname>", we need to perform an fair analysis of the 
table.

-- 
regards,
Andrey Lepikhov
Postgres Professional

On 8/29/20 9:50 PM, Tom Lane wrote:
> Years ago (when I was still at Salesforce, IIRC, so ~5 years) we had
> some discussions about making it possible for pg_dump and/or pg_upgrade
> to propagate stats data forward to the new database.  There is at least
> one POC patch in the archives for doing that by dumping the stats data
> wrapped in a function call, where the target database's version of the
> function would be responsible for adapting the data if necessary, or
> maybe just discarding it if it couldn't adapt.  We seem to have lost
> interest but it still seems like something worth pursuing.  I'd guess
> that if such infrastructure existed it could be helpful for this.

Thanks for this helpful feedback.

I found several threads related to the problem [1-3].
I agreed that this task needs to implement an API for 
serialization/deserialization of statistics:
pg_load_relation_statistics(json_string text);
pg_get_relation_statistics(relname text);
We can use a version number for resolving conflicts with different 
statistics implementations.
"Load" function will validate the values[] anyarray while deserializing 
the input json string to the datatype of the relation column.

Maybe I didn't feel all the problems of this task?

1. https://www.postgresql.org/message-id/flat/724322880.K8vzik8zPz%40abook
2. 
https://www.postgresql.org/message-id/flat/CAAZKuFaWdLkK8eozSAooZBets9y_mfo2HS6urPAKXEPbd-JLCA%40mail.gmail.com
3. 
https://www.postgresql.org/message-id/flat/GNELIHDDFBOCMGBFGEFOOEOPCBAA.chriskl%40familyhealth.com.au

-- 
regards,
Andrey Lepikhov
Postgres Professional

On Mon, Aug 31, 2020 at 3:36 PM Andrey V. Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
>
> Thanks for this helpful feedback.
>
> I found several threads related to the problem [1-3].
> I agreed that this task needs to implement an API for
> serialization/deserialization of statistics:
> pg_load_relation_statistics(json_string text);
> pg_get_relation_statistics(relname text);
> We can use a version number for resolving conflicts with different
> statistics implementations.
> "Load" function will validate the values[] anyarray while deserializing
> the input json string to the datatype of the relation column.
>

This is a valuable feature. Analysing a foreign table by fetching rows
from the foreign server isn't very efficient. In fact the current FDW
API for doing that forges that in-efficiency by requiring the FDW to
return a sample of rows that will be analysed by the core. That's why
I see that your patch introduces a new API to get foreign rel stat. I
don't think there's any point in maintaining these two APIs just for
ANALYSING table. Instead we should have only one FDW API which will do
whatever it wants and return statistics that can be understood by the
core and let core install it in the catalogs. I believe that's doable.

In case of PostgreSQL it could get the stats available as is from the
foreign server, convert it into a form that the core understands and
returns. The patch introduces a new function postgres_fdw_stat() which
will be available only from version 14 onwards. Can we use
row_to_json(), which is available in all the supported versions,
instead?

In case of some other foreign server, an FDW will be responsible to
return statistics in a form that the core will understand. It may
fetch rows from the foreign server or be a bit smart and fetch the
statistics and convert.

This also means that FDWs will have to deal with the statistics format
that the core understands and thus will need changes in their code
with every version in the worst case. But AFAIR, PostgreSQL supports
different forms of statistics so the problem may not remain that
severe if FDWs and core agree on some bare minimum format that the
core supports for long.

I think the patch has some other problems like it works only for
regular tables on foreign server but a foreign table can be pointing
to any relation like a materialized view, partitioned table or a
foreign table on the foreign server all of which have statistics
associated with them. I didn't look closely but it does not consider
that the foreign table may not have all the columns from the relation
on the foreign server or may have different names. But I think those
problems are kind of secondary. We have to agree on the design first.

-- 
Best Wishes,
Ashutosh Bapat

Stephen Frost <sfrost@snowman.net> writes:
> Feature work either requires changes to pg_dump, or not.  I agree that
> features which don't require pg_dump changes are definitionally less
> work than features which do (presuming the rest of the feature is the
> same in both cases) but that isn't a justification to not have pg_dump
> support in cases where it's expected- we just don't currently expect it
> for statistics (which is a rather odd exception when you consider that
> nearly everything else that ends up in the catalog tables is included).

> For my part, at least, I'd like to see us change that expectation, for a
> number of reasons:

Yeah.  I think that originally we expected that the definition of the
stats might change fast enough that porting them cross-version would be
problematic.  Subsequent experience has shown that they don't actually
change any faster than any other aspect of the catalogs.  So, while
I do think we must have a plan for how to cope when/if the definition
changes, I don't buy Bruce's argument that it's going to require more
maintenance effort than any other part of the system does.

            regards, tom lane

On 8/31/20 6:19 PM, Ashutosh Bapat wrote:
> On Mon, Aug 31, 2020 at 3:36 PM Andrey V. Lepikhov
> <a.lepikhov@postgrespro.ru> wrote:
>> I agreed that this task needs to implement an API for
>> serialization/deserialization of statistics:
>> pg_load_relation_statistics(json_string text);
>> pg_get_relation_statistics(relname text);
>> We can use a version number for resolving conflicts with different
>> statistics implementations.
>> "Load" function will validate the values[] anyarray while deserializing
>> the input json string to the datatype of the relation column.
> 
> This is a valuable feature. Analysing a foreign table by fetching rows
> from the foreign server isn't very efficient. In fact the current FDW
> API for doing that forges that in-efficiency by requiring the FDW to
> return a sample of rows that will be analysed by the core. That's why
> I see that your patch introduces a new API to get foreign rel stat. I
> don't think there's any point in maintaining these two APIs just for

> ANALYSING table. Instead we should have only one FDW API which will do
> whatever it wants and return statistics that can be understood by the
> core and let core install it in the catalogs. I believe that's doable.
I think the same.
> 
> In case of PostgreSQL it could get the stats available as is from the
> foreign server, convert it into a form that the core understands and
> returns. The patch introduces a new function postgres_fdw_stat() which
> will be available only from version 14 onwards. Can we use
> row_to_json(), which is available in all the supported versions,
> instead?
I started from here. But we need to convert starelid, staop[] stacoll[] 
oids into portable format. Also we need to explicitly specify the type 
of each values[] array. And no one guaranteed that anyarray values[] 
can't contained an array of complex type values, containing oids, that 
can't be correctly converted to database objects on another server...
These considerations required me to add new postgres_fdw_stat() routine 
that can be moved into the core.
> 
> In case of some other foreign server, an FDW will be responsible to
> return statistics in a form that the core will understand. It may
> fetch rows from the foreign server or be a bit smart and fetch the
> statistics and convert.
I don't think I fully understood your idea. Please explain in more 
detail if possible.
> 
> This also means that FDWs will have to deal with the statistics format
> that the core understands and thus will need changes in their code
> with every version in the worst case. But AFAIR, PostgreSQL supports
> different forms of statistics so the problem may not remain that
> severe if FDWs and core agree on some bare minimum format that the
> core supports for long.
I don't think FDW needs to know anything about the internals of 
statistics. It only need to execute query like
"SELECT extract_statistics(namespace.relation);"
and apply the text representation by the function call like this:
store_statistics(const char *stat);
All validation and update pg_statistic operations will be performed into 
the core.
> 
> I think the patch has some other problems like it works only for
> regular tables on foreign server but a foreign table can be pointing
> to any relation like a materialized view, partitioned table or a
> foreign table on the foreign server all of which have statistics
> associated with them.
Ok. It was implemented for discussion, test and as a base of development.

> I didn't look closely but it does not consider
> that the foreign table may not have all the columns from the relation
> on the foreign server or may have different names.
Here we get full statistics from remote server and extract statistics 
only for columns, included into the tuple descriptor of foreign table.

> But I think those
> problems are kind of secondary. We have to agree on the design first. 
+1.
I only want to point out the following. In previous threads statistics 
was converted row-by-row. I want to suggest to serialize all statistics 
tuples for the relation into single json string. On apply phase we can 
filter unneeded attributes.

-- 
regards,
Andrey Lepikhov
Postgres Professional

On 8/31/20 6:19 PM, Ashutosh Bapat wrote:
> On Mon, Aug 31, 2020 at 3:36 PM Andrey V. Lepikhov
> <a.lepikhov@postgrespro.ru> wrote:
>>
>> Thanks for this helpful feedback.
> I think the patch has some other problems like it works only for
> regular tables on foreign server but a foreign table can be pointing
> to any relation like a materialized view, partitioned table or a
> foreign table on the foreign server all of which have statistics
> associated with them. I didn't look closely but it does not consider
> that the foreign table may not have all the columns from the relation
> on the foreign server or may have different names. But I think those
> problems are kind of secondary. We have to agree on the design first.
> 
In accordance with discussion i made some changes in the patch:
1. The extract statistic routine moved into the core.
2. Serialized stat contains 'version' field to indicate format of 
statistic received.
3. ANALYZE and VACUUM ANALYZE uses this approach only in the case of 
implicit analysis of the relation.

I am currently keeping limitation of using the approach for regular 
relations only, because i haven't studied the specifics of another types 
of relations.
But I don't know any reason to keep this limit in the future.

The patch in attachment is very raw. I publish for further substantive 
discussion.

-- 
regards,
Andrey Lepikhov
Postgres Professional

On Thu, Sep 03, 2020 at 10:14:41AM +0500, Andrey V. Lepikhov wrote:
>On 8/31/20 6:19 PM, Ashutosh Bapat wrote:
>>On Mon, Aug 31, 2020 at 3:36 PM Andrey V. Lepikhov
>><a.lepikhov@postgrespro.ru> wrote:
>>>
>>>Thanks for this helpful feedback.
>>I think the patch has some other problems like it works only for
>>regular tables on foreign server but a foreign table can be pointing
>>to any relation like a materialized view, partitioned table or a
>>foreign table on the foreign server all of which have statistics
>>associated with them. I didn't look closely but it does not consider
>>that the foreign table may not have all the columns from the relation
>>on the foreign server or may have different names. But I think those
>>problems are kind of secondary. We have to agree on the design first.
>>
>In accordance with discussion i made some changes in the patch:
>1. The extract statistic routine moved into the core.
>2. Serialized stat contains 'version' field to indicate format of 
>statistic received.
>3. ANALYZE and VACUUM ANALYZE uses this approach only in the case of 
>implicit analysis of the relation.
>
>I am currently keeping limitation of using the approach for regular 
>relations only, because i haven't studied the specifics of another 
>types of relations.
>But I don't know any reason to keep this limit in the future.
>
>The patch in attachment is very raw. I publish for further substantive 
>discussion.
>

Thanks for working on this. I briefly looked at the patch today, and I
have some comments/feedback:

1) I wonder why deparseGetStatSql looks so different from e.g.
deparseAnalyzeSizeSql - no deparseStringLiteral on relname, no cast to
pg_catalog.regclass, function name not qualified with pg_catalog, ...


2) I'm a bit annoyed by the amount of code added to analyze.c only to
support output/input in JSON format. I'm no expert, but I don't recall
explain needing this much new stuff (OTOH it just produces json, it does
not need to read it). Maybe we also need to process wider range of data
types here. But the code is almost perfectly undocumented :-(


3) Why do we need to change vacuum_rel this way?


4) I wonder if we actually want/need to simply output pg_statistic data
verbatim like this. Is postgres_fdw actually going to benefit from it? I
kinda doubt that, and my assumption was that we'd return only a small
subset of the data, needed by get_remote_estimate.

This has a couple of issues. Firstly, it requires the knowledge of what
the stakind constants in pg_statistic mean and how to interpret it - but
OK, it's true that does not change very often (or at all). Secondly, it
entirely ignores extended statistics - OK, we might extract those too,
but it's going to be much more complex. And finally it entirely ignores
costing on the remote node. Surely we can't just apply local
random_page_cost or whatever, because those may be entirely different.
And we don't know if the remote is going to use index etc.

So is extracting data from pg_statistic the right approach?


5) I doubt it's enough to support relnames - we also need to estimate
joins, so this needs to support plain queries I think. At least that's
what Tom envisioned in his postgres_fdw_support(query text) proposal.


6) I see you've included a version number in the data - why not to just
check 


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 9/4/20 6:23 PM, Ashutosh Bapat wrote:
> 
> 
> On Thu, 3 Sep 2020 at 10:44, Andrey V. Lepikhov 
> <a.lepikhov@postgrespro.ru <mailto:a.lepikhov@postgrespro.ru>> wrote:
> 
>     On 8/31/20 6:19 PM, Ashutosh Bapat wrote:
>      > On Mon, Aug 31, 2020 at 3:36 PM Andrey V. Lepikhov
>      > <a.lepikhov@postgrespro.ru <mailto:a.lepikhov@postgrespro.ru>> wrote:
>      >>
>      >> Thanks for this helpful feedback.
>      > I think the patch has some other problems like it works only for
>      > regular tables on foreign server but a foreign table can be pointing
>      > to any relation like a materialized view, partitioned table or a
>      > foreign table on the foreign server all of which have statistics
>      > associated with them. I didn't look closely but it does not consider
>      > that the foreign table may not have all the columns from the relation
>      > on the foreign server or may have different names. But I think those
>      > problems are kind of secondary. We have to agree on the design first.
>      >
>     In accordance with discussion i made some changes in the patch:
>     1. The extract statistic routine moved into the core.
> 
> 
> Bulk of the patch implements the statistics conversion to and fro json 
> format. I am still not sure whether we need all of that code here.
Yes, i'm sure we'll replace it with something.

Right now, i want to discuss format of statistics dump. Remind, that a 
statistics dump is needed not only for fdw, but it need for the pg_dump. 
And in the dump will be placed something like this:
'SELECT store_relation_statistics(rel, serialized_stat)'

my reasons for using JSON:
* it have conversion infrastructure like json_build_object()
* this is flexible readable format, that can be useful in text dumps of 
relations.

> Can we re-use pg_stats view? That is converting some of the OIDs to names. I 
> agree with anyarray but if that's a problem here it's also a problem for 
> pg_stats view, isn't it?
Right now, I don't know if it is possible to unambiguously convert the 
pg_stats information to a pg_statistic tuple.

> If we can reduce the stats handling code to a 
> minimum or use it for some other purpose as well e.g. pg_stats 
> enhancement, the code changes required will be far less compared to the 
> value that this patch provides.
+1

-- 
regards,
Andrey Lepikhov
Postgres Professional

On Tue, Sep 08, 2020 at 05:55:09PM +0530, Ashutosh Bapat wrote:
>On Fri, 4 Sep 2020 at 20:27, Tomas Vondra <tomas.vondra@2ndquadrant.com>
>wrote
>
>>
>>
>> 4) I wonder if we actually want/need to simply output pg_statistic data
>> verbatim like this. Is postgres_fdw actually going to benefit from it? I
>> kinda doubt that, and my assumption was that we'd return only a small
>> subset of the data, needed by get_remote_estimate.
>>
>> This has a couple of issues. Firstly, it requires the knowledge of what
>> the stakind constants in pg_statistic mean and how to interpret it - but
>> OK, it's true that does not change very often (or at all). Secondly, it
>> entirely ignores extended statistics - OK, we might extract those too,
>> but it's going to be much more complex. And finally it entirely ignores
>> costing on the remote node. Surely we can't just apply local
>> random_page_cost or whatever, because those may be entirely different.
>> And we don't know if the remote is going to use index etc.
>>
>> So is extracting data from pg_statistic the right approach?
>>
>>
>There are two different problems, which ultimately might converge.
>1. If use_remote_estimates = false, more generally if querying costs from
>foreign server for costing paths is impractical, we want to use local
>estimates and try to come up with costs. For that purpose we keep some
>statistics locally and user is expected to refresh it periodically by
>running ANALYZE on the foreign table. This patch is about a. doing this
>efficiently without requiring to fetch every row from the foreign server b.
>through autovacuum automatically without user firing ANALYZE. I think this
>also answers your question about vacuum_rel() above.
>
>2. How to efficiently extract costs from an EXPLAIN plan when
>use_remote_eestimates is true. That's the subject of some nearby thread. I
>think you are referring to that problem here. Hence your next point.
>

I think that was the topic of *this* thread as started by Tom, but I now
realize Andrey steered it in the direction to allow re-using remote
stats. Which seems useful too, but it confused me a bit.

>Using EXPLAIN to get costs from the foreign server isn't efficient. It
>increases planning time a lot; sometimes planning time exceeds execution
>time. If usage of foreign tables becomes more and more common, this isn't
>ideal. I think we should move towards a model in which the optimizer can
>decide whether a subtree involving a foreign server should be evaluated
>locally or on the foreign server without the help of foreign server. One
>way to do it (I am not saying that this is the only or the best way) is to
>estimate the cost of foreign query locally based on the information
>available locally about the foreign server and foreign table. This might
>mean that we have to get that information from the foreign server and cache
>it locally and use it several times, including the indexes on foreign
>table, values of various costs etc. Though this approach doesn't solve all
>of those problems it's one step forward + it makes the current scenario
>also efficient.
>

True, but that ptoject is way more ambitious than providing a simple API
for postgres_fdw to obtain the estimates more efficiently.

>I agree that the patch needs some work though, esp the code dealing with
>serialization and deserialization of statistics.

I think there's a bunch of open questions, e.g. what to do with extended
statistics - for example what should happen when the extended statistics
object is defined only on local/remote server, or when the definitions
don't match? What should happen when the definitions don't match? This
probably is not an issue for "regular" stats, because that seems pretty
stable, but for extended stats there are differences between versions.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services