Hi,
On 18.12.2014 13:14, Petr Jelinek wrote:
> Hi,
>
> v2 version of this patch is attached.
I did a review of this v2 patch today. I plan to do a bit more testing,
but these are my comments/questions so far:
(0) There's a TABLESAMPLE page at the wiki, not updated since 2012:
https://wiki.postgresql.org/wiki/TABLESAMPLE_Implementation
We should either update it or mark it as obsolete I guess. Also,
I'd like to know what's the status regarding the TODO items
mentioned there. Are those still valid with this patch?
(1) The patch adds a new catalog, but does not bump CATVERSION.
(2) The catalog naming (pg_tablesamplemethod) seems a bit awkward,
as it squishes everything into a single chunk. That's inconsistent
with naming of the other catalogs. I think pg_table_sample_method
would be better.
(3) There are a few more strange naming decisions, but that's mostly
because of the SQL standard requires that naming. I mean SYSTEM and
BERNOULLI method names, and also the fact that the probability is
specified as 0-100 value, which is inconsistent with other places
(e.g. percentile_cont uses the usual 0-1 probability notion). But
I don't think this can be fixed, that's what the standard says.
(4) I noticed there's an interesting extension in SQL Server, which
allows specifying PERCENT or ROWS, so you can say
SELECT * FROM table TABLESAMPLE SYSTEM (25 PERCENT);
or
SELECT * FROM table TABLESAMPLE SYSTEM (2500 ROWS);
That seems handy, and it'd make migration from SQL Server easier.
What do you think?
(5) I envision a lot of confusion because of the REPEATABLE clause.
With READ COMMITTED, it's not really repeatable because of changes
done by the other users (and maybe things like autovacuum). Shall
we mention this in the documentation?
(6) This seems slightly wrong, because of long/uint32 mismatch:
long seed = PG_GETARG_UINT32(1);
I think uint32 would be more appropriate, no?
(7) NITPICKING: I think a 'sample_rate' would be a better name here:
double percent = sampler->percent;
(8) NITPICKING: InitSamplingMethod contains a command with ';;'
fcinfo.arg[i] = (Datum) 0;;
(9) The current regression tests only use the REPEATABLE cases. I
understand queries without this clause are RANDOM, but maybe we
could do something like this:
SELECT COUNT(*) BETWEEN 5000 AND 7000 FROM (
SELECT id FROM test_tablesample TABLESAMPLE SYSTEM (50)
) foo;
Granted, there's still a small probability of false positive, but
maybe that's sufficiently small? Or is the amount of code this
tests negligible?
(10) In the initial patch you mentioned it's possible to write custom
sampling methods. Do you think a CREATE TABLESAMPLE METHOD,
allowing custom methods implemented as extensions would be useful?
regards
Tomas
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
