Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> The second patch adds the relkind check, so that we only issue
> TABLESAMPLE on valid relation types (tables, matviews). But I'm not sure
> we actually need it - the example presented earlier was foreign table
> pointing to a sequence. But that actually works fine even without this
> patch, because fore sequences we have reltuples=1, which disables
> sampling due to the check mentioned above (because targrows >= 1).
Seems pretty accidental still.
> The other issue with this patch is that it seems wrong to check the
> relkind value locally - instead we should check this on the remote
> server, because that's where we'll run TABLESAMPLE. Currently there are
> no differences between versions, but what if we implement TABLESAMPLE
> for another relkind in the future? Then we'll either fail to use
> sampling or (worse) we'll try using TABLESAMPLE for unsupported relkind,
> depending on which of the servers has newer version.
We don't have a way to do that, and I don't think we need one. The
worst case is that we don't try to use TABLESAMPLE when the remote
server is a newer version that would allow it. If we do extend the
set of supported relkinds, we'd need a version-specific test in
postgres_fdw so that we don't wrongly use TABLESAMPLE with an older
remote server ... but that's hardly complicated, or a new concept.
On the other hand, if we let the code stand, the worst case is that
ANALYZE fails because we try to apply TABLESAMPLE in an unsupported
case, presumably with some remote relkind that doesn't exist today.
I think that's clearly worse than not exploiting TABLESAMPLE
although we could (with some other new remote relkind).
As far as the patch details go: I'd write 0001 as
+ Assert(sample_frac >= 0.0 && sample_frac <= 1.0);
The way you have it seems a bit contorted. As for 0002, personally
I'd rename the affected functions to reflect their expanded duties,
and they *definitely* require adjustments to their header comments.
Functionally it looks fine though.
regards, tom lane
