public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Tomas Vondra <[email protected]>
Cc: James Finnerty <[email protected]>
Cc: [email protected]
Subject: Re: postgres_fdw: using TABLESAMPLE to collect remote sample
Date: Thu, 05 Jan 2023 16:05:11 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<167112207543.1120.12195641719862911275.pgcf@coridan.postgresql.org>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Tomas Vondra <[email protected]> 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

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: postgres_fdw: using TABLESAMPLE to collect remote sample
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox