TABLESAMPLE patch

On 11/12/14 00:24, Petr Jelinek wrote:

Hello,

Attached is a basic implementation of TABLESAMPLE clause. It's SQL
standard clause and couple of people tried to submit it before so I
think I don't need to explain in length what it does - basically returns
"random" sample of a table using a specified sampling method.

I implemented both SYSTEM and BERNOULLI sampling as specified by SQL
standard. The SYSTEM sampling does block level sampling using same
algorithm as ANALYZE, BERNOULLI scans whole table and picks tuple randomly.

There is API for sampling methods which consists of 4 functions at the
moment - init, end, nextblock and nexttuple. I added catalog which maps
the sampling method to the functions implementing this API. The grammar
creates new TableSampleRange struct that I added for sampling. Parser
then uses the catalog to load information about the sampling method into
TableSampleClause which is then attached to RTE. Planner checks for if
this parameter is present in the RTE and if it finds it it will create
plan with just one path - SampleScan. SampleScan implements standard
executor API and calls the sampling method API as needed.

It is possible to write custom sampling methods. The sampling method
parameters are not limited to just percent number as in standard but
dynamic list of expressions which is checked against the definition of
the init function in a similar fashion (although much simplified) as
function calls are.

Notable lacking parts are:
- proper costing and returned row count estimation - given the dynamic
nature of parameters I think for we'll need to let the sampling method
do this, so there will have to be fifth function in the API.
- ruleutils support (it needs a bit of code in get_from_clause_item
function)
- docs are sparse at the moment

Forgot the obligatory:

The research leading to these results has received funding from the
European Union's Seventh Framework Programme (FP7/2007-2013) under grant
agreement n° 318633.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Dec 10, 2014 at 6:24 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

Hello,

Attached is a basic implementation of TABLESAMPLE clause. It's SQL standard
clause and couple of people tried to submit it before so I think I don't
need to explain in length what it does - basically returns "random" sample
of a table using a specified sampling method.

Tablesample, yay!

Sadly when the jsonb functions patch was committed a few oids where
used, so you should update the ones you are using. at least to make
the patch easier for testing.

The test added for this failed, attached is the diff. i didn't looked
up why it failed

Finally, i created a view with a tablesample clause. i used the view
and the tablesample worked, then dumped and restored and the
tablesample clause went away... actually pg_get_viewdef() didn't see
it at all.

will look at the patch more close tomorrow when my brain wake up ;)

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157

On 16/12/14 08:43, Jaime Casanova wrote:

On Wed, Dec 10, 2014 at 6:24 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:

Hello,

Attached is a basic implementation of TABLESAMPLE clause. It's SQL standard
clause and couple of people tried to submit it before so I think I don't
need to explain in length what it does - basically returns "random" sample
of a table using a specified sampling method.

Tablesample, yay!

Sadly when the jsonb functions patch was committed a few oids where
used, so you should update the ones you are using. at least to make
the patch easier for testing.

Will do.

The test added for this failed, attached is the diff. i didn't looked
up why it failed

It isn't immediately obvious to me why, will look into it.

Finally, i created a view with a tablesample clause. i used the view
and the tablesample worked, then dumped and restored and the
tablesample clause went away... actually pg_get_viewdef() didn't see
it at all.

Yeah, as I mentioned in the submission the ruleutils support is not
there yet, so that's expected.

will look at the patch more close tomorrow when my brain wake up ;)

Thanks!

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I noticed that this makes REPEATABLE a reserved keyword, which is
currently an unreserved one. Can we avoid that?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 17/12/14 19:55, Alvaro Herrera wrote:

I noticed that this makes REPEATABLE a reserved keyword, which is
currently an unreserved one. Can we avoid that?

I added it because I did not find any other way to fix the shift/reduce
conflicts that bison complained about. I am no bison expert though.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

v2 version of this patch is attached.

On 16/12/14 09:31, Petr Jelinek wrote:

On 16/12/14 08:43, Jaime Casanova wrote:

Sadly when the jsonb functions patch was committed a few oids where
used, so you should update the ones you are using. at least to make
the patch easier for testing.

Will do.

Done.

The test added for this failed, attached is the diff. i didn't looked
up why it failed

It isn't immediately obvious to me why, will look into it.

Fixed.

Finally, i created a view with a tablesample clause. i used the view
and the tablesample worked, then dumped and restored and the
tablesample clause went away... actually pg_get_viewdef() didn't see
it at all.

Yeah, as I mentioned in the submission the ruleutils support is not
there yet, so that's expected.

Also fixed.

I also added proper costing/row estimation. I consider this patch
feature complete now, docs could still use improvement though.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On Mon, Dec 22, 2014 at 2:38 AM, Tomas Vondra <tv@fuzzy.cz> wrote:

(1) The patch adds a new catalog, but does not bump CATVERSION.

FWIW, this part is managed by the committer when this patch is picked up.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 21/12/14 18:38, Tomas Vondra wrote:

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:

Thanks for looking at it!

(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?

I will have to look in more detail over the holidays and update it, but
the general info about table sampling there applies and will apply to
any patch trying to implement it.

Quick look on the mail thread suggest that at least the concerns
mentioned in the mailing list should not apply to this implementation.
And looking at the patch the problem with BERNOULLI sampling should not
exist either as I use completely different implementation for that.

I do also have some issues with joins which I plan to look at but it's
different one (my optimizer code overestimates the number of rows)

(1) The patch adds a new catalog, but does not bump CATVERSION.

I thought this was always done by committer?

(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.

Fair point, but perhaps pg_tablesample_method then as tablesample is
used as single word everywhere including the standard.

(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.

Yeah, I don't exactly love that either but what standard says, 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?

Well doing it exactly this way somewhat kills the extensibility which
was one of the main goals for me - I allow any kind of parameters for
sampling and the handling of those depends solely on the sampling
method. This means that in my approach if you'd want to change what you
are limiting you'd have to write new sampling method and the query would
then look like SELECT * FROM table TABLESAMPLE SYSTEM_ROWLIMIT (2500);
or some such (depending on how you name the sampling method). Or SELECT
* FROM table TABLESAMPLE SYSTEM (2500, 'ROWS'); if we chose to extend
the SYSTEM sampling method, that would be also doable.

The reason for this is that I don't want to really limit too much what
parameters can be send to sampling (I envision even SYSTEM_TIMED
sampling method that will get limit as time interval for example).

(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?

Yes docs need improvement and this should be mentioned, also code-docs -
I found few places which I forgot to update when changing code and now
have misleading comments.

(6) This seems slightly wrong, because of long/uint32 mismatch:

long seed = PG_GETARG_UINT32(1);

I think uint32 would be more appropriate, no?

Probably, although I need long later in the algorithm anyway.

(7) NITPICKING: I think a 'sample_rate' would be a better name here:

double percent = sampler->percent;

Ok.

(8) NITPICKING: InitSamplingMethod contains a command with ';;'

fcinfo.arg[i] = (Datum) 0;;

Yeah :)

(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?

Well, depending on fillfactor and limit it could be made quite reliable
I think, I also want to add test with VIEW (I think v2 has a bug there)
and perhaps some JOIN.

(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?

Yes, I think so, CREATE/DROP TABLESAMPLE METHOD is on my TODO, but since
that's just simple mechanical work with no hard problems to solve there
I can add it later once we have agreement on the general approach of the
patch (especially in the terms of extensibility).

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 22.12.2014 10:07, Petr Jelinek wrote:

On 21/12/14 18:38, Tomas Vondra wrote:

(1) The patch adds a new catalog, but does not bump CATVERSION.

I thought this was always done by committer?

Right. Sorry for the noise.

(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.

Fair point, but perhaps pg_tablesample_method then as tablesample is
used as single word everywhere including the standard.

Sounds good.

(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?

Well doing it exactly this way somewhat kills the extensibility which
was one of the main goals for me - I allow any kind of parameters for
sampling and the handling of those depends solely on the sampling
method. This means that in my approach if you'd want to change what you
are limiting you'd have to write new sampling method and the query would
then look like SELECT * FROM table TABLESAMPLE SYSTEM_ROWLIMIT (2500);
or some such (depending on how you name the sampling method). Or SELECT
* FROM table TABLESAMPLE SYSTEM (2500, 'ROWS'); if we chose to extend
the SYSTEM sampling method, that would be also doable.

The reason for this is that I don't want to really limit too much what
parameters can be send to sampling (I envision even SYSTEM_TIMED
sampling method that will get limit as time interval for example).

Good point.

(6) This seems slightly wrong, because of long/uint32 mismatch:

long seed = PG_GETARG_UINT32(1);

I think uint32 would be more appropriate, no?

Probably, although I need long later in the algorithm anyway.

Really? ISTM the sampler_setseed() does not really require long, uint32
would work exactly the same.

(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?

Well, depending on fillfactor and limit it could be made quite reliable
I think, I also want to add test with VIEW (I think v2 has a bug there)
and perhaps some JOIN.

OK.

(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?

Yes, I think so, CREATE/DROP TABLESAMPLE METHOD is on my TODO, but since
that's just simple mechanical work with no hard problems to solve there
I can add it later once we have agreement on the general approach of the
patch (especially in the terms of extensibility).

OK, good to know.

Tomas

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Dec 18, 2014 at 7:14 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:

Hi,

v2 version of this patch is attached.

a few more tests revealed that passing null as the sample size
argument works, and it shouldn't.
in repeatable it gives an error if i use null as argument but it gives
a syntax error, and it should be a data exception (data exception --
invalid repeat argument in a sample clause) according to the standard

also you need to add CHECK_FOR_INTERRUPTS somewhere, i tried with a
big table and had to wait a long time for it to finish

"""
regression=# select count(1) from tenk1 tablesample system (null);
count
-------
28
(1 row)

regression=# select count(1) from tenk1 tablesample bernoulli (null);
count
-------
0
(1 row)
"""

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 22/12/14 20:14, Jaime Casanova wrote:

On Thu, Dec 18, 2014 at 7:14 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:

Hi,

v2 version of this patch is attached.

a few more tests revealed that passing null as the sample size
argument works, and it shouldn't.

Fixed.

in repeatable it gives an error if i use null as argument but it gives
a syntax error, and it should be a data exception (data exception --
invalid repeat argument in a sample clause) according to the standard

Also fixed.

also you need to add CHECK_FOR_INTERRUPTS somewhere, i tried with a
big table and had to wait a long time for it to finish

Ah yeah, I can't rely on CHECK_FOR_INTERRUPTS in ExecScan because it
might take a while to fetch a row if percentage is very small and table
is big... Fixed.

Attached is v3 which besides the fixes mentioned above also includes
changes discussed with Tomas (except the CREATE/DROP TABLESAMPLE
METHOD), fixes for crash with FETCH FIRST and is rebased against current
master.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:

Attached is v3 which besides the fixes mentioned above also includes changes
discussed with Tomas (except the CREATE/DROP TABLESAMPLE METHOD), fixes for
crash with FETCH FIRST and is rebased against current master.

This patch needs a rebase, there is a small conflict in parallel_schedule.

Structurally speaking, I think that the tsm methods should be added in
src/backend/utils and not src/backend/access which is more high-level
as tsm_bernoulli.c and tsm_system.c contain only a set of new
procedure functions. Having a single header file tsm.h would be also a
good thing.

Regarding the naming, is "tsm" (table sample method) really appealing?
Wouldn't it be better to use simply tablesample_* for the file names
and the method names?

This is a large patch... Wouldn't sampling.[c|h] extracted from
ANALYZE live better as a refactoring patch? This would limit a bit bug
occurrences on the main patch.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/01/15 08:51, Michael Paquier wrote:

On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:

Attached is v3 which besides the fixes mentioned above also includes changes
discussed with Tomas (except the CREATE/DROP TABLESAMPLE METHOD), fixes for
crash with FETCH FIRST and is rebased against current master.

This patch needs a rebase, there is a small conflict in parallel_schedule.

Sigh, I really wish we had automation that checks this automatically for
patches in CF.

Structurally speaking, I think that the tsm methods should be added in
src/backend/utils and not src/backend/access which is more high-level
as tsm_bernoulli.c and tsm_system.c contain only a set of new

I am not sure if I parsed this correctly, do you mean to say that only
low-level access functions belong to src/backend/access? Makes sense.

procedure functions. Having a single header file tsm.h would be also a
good thing.

I was thinking about single header also, didn't find a good precedent so
went with two, but don't have problem doing one :).

Regarding the naming, is "tsm" (table sample method) really appealing?
Wouldn't it be better to use simply tablesample_* for the file names
and the method names?

Doesn't really matter to me, I just really don't want to have
tablesample_method_* there as that's way too long for my taste. I'll
think about the naming when I move it to src/backend/utils.

This is a large patch... Wouldn't sampling.[c|h] extracted from
ANALYZE live better as a refactoring patch? This would limit a bit bug
occurrences on the main patch.

That's a good idea, I'll split it into patch series.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2015-01-06 14:22:16 +0100, Petr Jelinek wrote:

On 06/01/15 08:51, Michael Paquier wrote:

On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:

Attached is v3 which besides the fixes mentioned above also includes changes
discussed with Tomas (except the CREATE/DROP TABLESAMPLE METHOD), fixes for
crash with FETCH FIRST and is rebased against current master.

This patch needs a rebase, there is a small conflict in parallel_schedule.

Sigh, I really wish we had automation that checks this automatically for
patches in CF.

FWIW, I personally think minor conflicts aren't really an issue and
don't really require a rebase. At least if the patches are in git
format, the reviewer can just use the version they're based on. Perhaps
always stating which version of the tree the patches apply on would be
good practice.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/01/15 14:22, Petr Jelinek wrote:

On 06/01/15 08:51, Michael Paquier wrote:

On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek <petr@2ndquadrant.com>
wrote:

Attached is v3 which besides the fixes mentioned above also includes
changes
discussed with Tomas (except the CREATE/DROP TABLESAMPLE METHOD),
fixes for
crash with FETCH FIRST and is rebased against current master.

This patch needs a rebase, there is a small conflict in
parallel_schedule.

Sigh, I really wish we had automation that checks this automatically for
patches in CF.

Here is rebase against current master.

Structurally speaking, I think that the tsm methods should be added in
src/backend/utils and not src/backend/access which is more high-level
as tsm_bernoulli.c and tsm_system.c contain only a set of new

I am not sure if I parsed this correctly, do you mean to say that only
low-level access functions belong to src/backend/access? Makes sense.

Made this change.

procedure functions. Having a single header file tsm.h would be also a
good thing.

Moved into single tablesample.h file.

Regarding the naming, is "tsm" (table sample method) really appealing?
Wouldn't it be better to use simply tablesample_* for the file names
and the method names?

I created the src/backend/tablesample and files are named just system.c
and bernoulli.c, but I kept tsm_ prefix for methods as they become too
long for my taste when prefixing with tablesample_.

This is a large patch... Wouldn't sampling.[c|h] extracted from
ANALYZE live better as a refactoring patch? This would limit a bit bug
occurrences on the main patch.

That's a good idea, I'll split it into patch series.

I've split the sampling.c/h into separate patch.

I also wrote basic CREATE/DROP TABLESAMPLE METHOD support, again as
separate patch in the attached patch-set. This also includes modules
test with simple custom tablesample method.

There are some very minor cleanups in the main tablesample code itself
but no functional changes.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Jan 9, 2015 at 1:10 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:

On 06/01/15 14:22, Petr Jelinek wrote:

On 06/01/15 08:51, Michael Paquier wrote:

On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek <petr@2ndquadrant.com>
wrote:

Attached is v3 which besides the fixes mentioned above also includes
changes
discussed with Tomas (except the CREATE/DROP TABLESAMPLE METHOD),
fixes for
crash with FETCH FIRST and is rebased against current master.

This patch needs a rebase, there is a small conflict in
parallel_schedule.

Sigh, I really wish we had automation that checks this automatically for
patches in CF.

Here is rebase against current master.

Thanks!

Structurally speaking, I think that the tsm methods should be added in
src/backend/utils and not src/backend/access which is more high-level
as tsm_bernoulli.c and tsm_system.c contain only a set of new

I am not sure if I parsed this correctly, do you mean to say that only
low-level access functions belong to src/backend/access? Makes sense.

Made this change.

procedure functions. Having a single header file tsm.h would be also a
good thing.

Moved into single tablesample.h file.

Regarding the naming, is "tsm" (table sample method) really appealing?
Wouldn't it be better to use simply tablesample_* for the file names
and the method names?

I created the src/backend/tablesample and files are named just system.c and
bernoulli.c, but I kept tsm_ prefix for methods as they become too long for
my taste when prefixing with tablesample_.

This is a large patch... Wouldn't sampling.[c|h] extracted from
ANALYZE live better as a refactoring patch? This would limit a bit bug
occurrences on the main patch.

That's a good idea, I'll split it into patch series.

I've split the sampling.c/h into separate patch.

I also wrote basic CREATE/DROP TABLESAMPLE METHOD support, again as separate
patch in the attached patch-set. This also includes modules test with simple
custom tablesample method.

There are some very minor cleanups in the main tablesample code itself but
no functional changes.

Some comments about the 1st patch:
1) Nitpicking:
+ * Block sampling routines shared by ANALYZE and TABLESAMPLE.
TABLESAMPLE is not added yet. You may as well mention simplify this
description with something like "Sampling routines for relation
blocks".

2) file_fdw and postgres_fdw do not compile correctly as they still
use anl_random_fract. This function is still mentioned in vacuum.h as
well.

3) Not really an issue of this patch, but I'd think that this comment
should be reworked:
+ * BlockSampler is used for stage one of our new two-stage tuple
+ * sampling mechanism as discussed on pgsql-hackers 2004-04-02 (subject
+ * "Large DB").  It selects a random sample of samplesize blocks out of
+ * the nblocks blocks in the table.  If the table has less than
+ * samplesize blocks, all blocks are selected.

4) As a refactoring patch, why is the function providing a random
value changed? Shouldn't sample_random_fract be consistent with
anl_random_fract?

5) The seed numbers can be changed to RAND48_SEED_0, RAND48_SEED_1 and
RAND48_SEED_2 instead of being hardcoded?
Regards,
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 09/01/15 09:27, Michael Paquier wrote:

Some comments about the 1st patch:
1) Nitpicking:
+ * Block sampling routines shared by ANALYZE and TABLESAMPLE.
TABLESAMPLE is not added yet. You may as well mention simplify this
description with something like "Sampling routines for relation
blocks".

Changed.

2) file_fdw and postgres_fdw do not compile correctly as they still
use anl_random_fract. This function is still mentioned in vacuum.h as
well.

Gah, didn't notice this, fixed. And also since the Vitter's reservoir
sampling methods are used by other component than just analyze, I moved
those to sampling.c/h.

3) Not really an issue of this patch, but I'd think that this comment
should be reworked:
+ * BlockSampler is used for stage one of our new two-stage tuple
+ * sampling mechanism as discussed on pgsql-hackers 2004-04-02 (subject
+ * "Large DB").  It selects a random sample of samplesize blocks out of
+ * the nblocks blocks in the table.  If the table has less than
+ * samplesize blocks, all blocks are selected.

I changed the wording slightly, it's still not too great though.

4) As a refactoring patch, why is the function providing a random
value changed? Shouldn't sample_random_fract be consistent with
anl_random_fract?

Yeah I needed that for TABLESAMPLE and it should not really affect the
randomness but you are correct it should be part of second patch of the
patch-set.

5) The seed numbers can be changed to RAND48_SEED_0, RAND48_SEED_1 and
RAND48_SEED_2 instead of being hardcoded?
Regards,

Removed this part from the first patch as it's not needed there anymore.

In second patch which implements the TABLESAMPLE itself I changed the
implementation of random generator because when I looked at the code
again I realized the old one would produce wrong results if there were
multiple TABLESAMPLE statements in same query or multiple cursors in
same transaction.

In addition to the above changes I added test for cursors and test for
the issue with random generator I mentioned above. Also fixed some typos
in comments and function name. And finally I added note to docs saying
that same REPEATABLE might produce different results in subsequent queries.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Jan 11, 2015 at 1:29 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:

In second patch which implements the TABLESAMPLE itself I changed the

implementation of random generator because when I looked at the code again
I realized the old one would produce wrong results if there were multiple
TABLESAMPLE statements in same query or multiple cursors in same
transaction.

I have looked into this patch and would like to share my
findings with you.

1.
+static void
+set_tablesample_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
RangeTblEntry *rte)
{
..
+ /*
+ * There is only one plan to consider but we still need to set
+ * parameters for RelOptInfo.
+ */
+ set_cheapest(rel);
}

It seems we already call set_cheapest(rel) in set_rel_pathlist()
which is a caller of set_tablesample_rel_pathlist(), so why do
we need it inside set_tablesample_rel_pathlist()?

2.
+static void
+set_tablesample_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
RangeTblEntry *rte)
{
..
+ /* We only do sample scan if it was requested */
+ add_path(rel, (Path *) create_samplescan_path(root, rel, required_outer));
}

Do we need to add_path, if there is only one path?

3.
@@ -332,6 +334,11 @@ set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
  /* Foreign table */
  set_foreign_pathlist(root, rel, rte);
  }
+ else if (rte->tablesample != NULL)
+ {
+ /* Build sample scan on relation */
+ set_tablesample_rel_pathlist(root, rel, rte);
+ }

Have you consider to have a different RTE for this?

4.
SampleNext()
a.
Isn't it better to code SampleNext() similar to SeqNext() and
IndexNext(), which encapsulate the logic to get the tuple in
another function(tbs_next() or something like that)?

b.
Also don't we want to handle pruning of page while
scanning (heap_page_prune_opt()) and I observed
in heap scan API's after visibility check we do check
for serializable conflict (CheckForSerializableConflictOut()).
Another thing is don't we want to do anything for sync scans
for these method's as they are doing heap scan?

c.
for bernoulli method, it will first get the tupoffset with
the help of function and then apply visibility check, it seems
that could reduce the sample set way lower than expected
in case lot of tuples are not visible, shouldn't it be done in reverse
way(first visibility check, then call function to see if we want to
include it in the sample)?
I think something similar is done in acquire_sample_rows which
seems right to me.

5.

CREATE TABLE test_tablesample (id INT, name text) WITH (fillfactor=10);
INSERT INTO test_tablesample SELECT i, repeat(i::text, 200) FROM
generate_series(0, 9) s(i) ORDER BY i;

postgres=# SELECT id FROM test_tablesample TABLESAMPLE BERNOULLI (80);
id
----
1
3
4
7
8
9
(6 rows)

postgres=# SELECT id FROM test_tablesample TABLESAMPLE BERNOULLI (80);
id
----
0
1
2
3
4
5
6
7
8
9
(10 rows)

So above test yield 60% rows first time and 100% rows next time,
when the test has requested 80%.
I understand that sample percentage will result an approximate
number of rows, however I just wanted that we should check if the
implementation has any problem or not?

In this regard, I have one question related to below code:

+Datum
+tsm_bernoulli_nexttuple(PG_FUNCTION_ARGS)
+{
..
+ /* Every tuple has percent chance of being returned */
+ while (sampler_random_fract(sampler->randstate) > samplesize)
+ {
+ tupoffset++;
+
+ if (tupoffset > maxoffset)
+ break;
+ }

Why are we not considering tuple in above code
if tupoffset is less than maxoffset?

6.
One larger question about the approach used in patch, why do you
think it is better to have a new node (SampleScan/SamplePath) like
you have used in patch instead of doing it as part of Sequence Scan.
I agree that we need some changes in different parts of Sequence Scan
execution, but still sounds like a viable way. One simple thing that
occurs to me is that in ExecSeqScan(), we can use something like
SampleSeqNext instead of SeqNext to scan heap in a slightly different
way, probably doing it as part of sequence scan will have advantage that
we can use most of the existing infrastructure (sequence node path)
and have less discrepancies as mentioned in point-4.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com