Thread: [PATCH]Tablesample Submission

On Mon, Aug 20, 2012 at 9:52 PM, Qi Huang <huangqiyx@outlook.com> wrote:
> Hi, hackers
>      I made the final version tablesample patch. It is implementing SYSTEM
> and BERNOULLI sample method, which is basically "feature-complete". The
> regression test is also included in this patch.
>     There is an wiki documentation on
> https://wiki.postgresql.org/wiki/TABLESAMPLE_Implementation. The detail
> about this patch and this project is all included in this documentation.

Please add your patch here:

https://commitfest.postgresql.org/action/commitfest_view/open

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Aug 21, 2012 at 8:08 AM, Qi Huang <huangqiyx@outlook.com> wrote:
>> Please add your patch here:
>>
>> https://commitfest.postgresql.org/action/commitfest_view/open
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
> Hi, Robert
>     I added it under "Miscellaneous".
>     https://commitfest.postgresql.org/action/patch_view?id=918
>
>

Patch does not apply cleanly against latest master.  outfuncs.c,
allpath.c and cost.h have rejected parts.  The make check failed in a
lot of cases up to 26 out of 133.  I didn't look into each issue but I
suggest rebasing on the latest master and making sure the regression
test passes.

Some of the patch don't follow our coding standard.  Please adjust
brace positions, for example.  For the header include list and
Makefile, place a new files in alphabetical order.

The patch doesn't include any documentation.  Consider add some doc
patch for such a big feature like this.

You should update kwlist.h for REPEATABLE but I'm not sure if
REPEATABLE should become a reserved keyword yet.

I don't see why you created T_TableSampleInfo.  TableSampleInfo looks
fine with a simple struct rather than a Node.

If we want to disable a cursor over a sampling table, we should check
it in the parser not the planner.  In the wiki page, one of the TODOs
says about cursor support, but how much difficult is it?  How does the
standard say?

s/skiped/skipped/

Don't we need to reset seed on ExecReScanSampleScan?  Should we add a
new executor node SampleScan?  It seems everything about random
sampling is happening under heapam.

It looks like BERNOULLI allocates heap tuple array beforehand, and
copy all the tuples into it.  This doesn't look acceptable when you
are dealing with a large number of rows in the table.

As wiki says, BERNOULLI relies on the statistics of the table, which
doesn't sound good to me.  Of course we could say this is our
restriction and say good-bye to users who hadn't run ANALYZE first,
but it is too hard for a normal users to use it.  We may need
quick-and-rough count(*) for this.

That is pretty much I have so far.  I haven't read all the code nor
the standard, so I might be wrong somewhere.

Thanks,
-- 
Hitoshi Harada

Hitoshi Harada escribió:

> Patch does not apply cleanly against latest master.  outfuncs.c,
> allpath.c and cost.h have rejected parts.  The make check failed in a
> lot of cases up to 26 out of 133.  I didn't look into each issue but I
> suggest rebasing on the latest master and making sure the regression
> test passes.

We've been waiting for a rebase for long enough, so I've marked this
patch as Returned with Feedback (for which we thank Hitoshi Harada).
Since this is said to be useful functionality, please make sure you
update the patch and resubmit to the next commitfest.  Thanks.

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

On Sun, Nov 4, 2012 at 10:22 PM, Qi Huang <huangqiyx@outlook.com> wrote:
> Dear hackers
>      Sorry for not replying the patch review. I didn't see the review until
> recently as my mail box is full of Postgres mails and I didn't notice the
> one for me, my mail box configuration problem.
>      I am still kind of busy with my university final year project. I shall
> not have time to work on updating the patch until this semester finishes
> which is December. I will work on then.

While we are still in the middle of a commitfest, i'm curious...
should we still wait for an update of this patch?

i know, any update on this should go to the next commitfest but i
wanted to ask before i forget about it

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

On 11/04/2012 07:22 PM, Qi Huang wrote:
> Dear hackers     Sorry for not replying the patch review. I didn't see the review until recently as my mail box is
fullof Postgres mails and I didn't notice the one for me, my mail box configuration problem.     I am still kind of
busywith my university final year project. I shall not have time to work on updating the patch until this semester
finisheswhich is December. I will work on then.    Thanks.
 
> Best RegardsHuang Qi VictorComputer Science of National University of Singapore

Did you ever do the update of the patch?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 17 January 2013 18:32, Josh Berkus <josh@agliodbs.com> wrote:
> On 11/04/2012 07:22 PM, Qi Huang wrote:
>> Dear hackers     Sorry for not replying the patch review. I didn't see the review until recently as my mail box is
fullof Postgres mails and I didn't notice the one for me, my mail box configuration problem.     I am still kind of
busywith my university final year project. I shall not have time to work on updating the patch until this semester
finisheswhich is December. I will work on then.    Thanks. 
>> Best RegardsHuang Qi VictorComputer Science of National University of Singapore
>
> Did you ever do the update of the patch?

We aren't just waiting for a rebase, we're waiting for Hitoshi's
comments to be addressed.

I would add to them by saying I am very uncomfortable with the idea of
allowing a TABLESAMPLE clause on an UPDATE or a DELETE. If you really
want that you can use a sub-select.

Plus the patch contains zero documentation.

So I can't see this going anywhere for 9.3. I've moved it to CF1 of
9.4 marked Waiting on Author

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

> So I can't see this going anywhere for 9.3. I've moved it to CF1 of
> 9.4 marked Waiting on Author

Agreed.  I wish I'd noticed that it got lost earlier.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Thu, Jan 17, 2013 at 2:04 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 17 January 2013 18:32, Josh Berkus <josh@agliodbs.com> wrote:
>> On 11/04/2012 07:22 PM, Qi Huang wrote:
>>> Dear hackers     Sorry for not replying the patch review. I didn't see the review until recently as my mail box is
fullof Postgres mails and I didn't notice the one for me, my mail box configuration problem.     I am still kind of
busywith my university final year project. I shall not have time to work on updating the patch until this semester
finisheswhich is December. I will work on then.    Thanks. 
>>> Best RegardsHuang Qi VictorComputer Science of National University of Singapore
>>
>> Did you ever do the update of the patch?
>
> We aren't just waiting for a rebase, we're waiting for Hitoshi's
> comments to be addressed.
>
> I would add to them by saying I am very uncomfortable with the idea of
> allowing a TABLESAMPLE clause on an UPDATE or a DELETE. If you really
> want that you can use a sub-select.
>

also, i don't think that the REPEATABLE clause should be included in a
first revision.
and if we ever want to add more sample methods we can't just put
BERNOULLI nor SYSTEM in gram.y, a new catalog is probably needed
there.

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

On Sun, Nov 4, 2012 at 10:22 PM, Qi Huang <huangqiyx@outlook.com> wrote:
> Dear hackers
>      Sorry for not replying the patch review. I didn't see the review until
> recently as my mail box is full of Postgres mails and I didn't notice the
> one for me, my mail box configuration problem.
>      I am still kind of busy with my university final year project. I shall
> not have time to work on updating the patch until this semester finishes
> which is December. I will work on then.

Hi,

Should we expect an updated patch for next commitfest?

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

On 17 May 2013 21:46, Jaime Casanova <jaime@2ndquadrant.com> wrote:
> On Sun, Nov 4, 2012 at 10:22 PM, Qi Huang <huangqiyx@outlook.com> wrote:
>> Dear hackers
>>      Sorry for not replying the patch review. I didn't see the review until
>> recently as my mail box is full of Postgres mails and I didn't notice the
>> one for me, my mail box configuration problem.
>>      I am still kind of busy with my university final year project. I shall
>> not have time to work on updating the patch until this semester finishes
>> which is December. I will work on then.

> Should we expect an updated patch for next commitfest?

This was added to CF1 of 9.4. The patch is nowhere near committable
and hasn't been worked on at all since last time it was submitted.

It's important that we have an efficient implementation of TABLESAMPLE
in Postgres.

I'm going to remove it from CF, for now, but I'll also take
responsibility for this for 9.4, barring objections.

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

On 18 September 2012 10:32, Hitoshi Harada <umi.tanuki@gmail.com> wrote:

> As wiki says, BERNOULLI relies on the statistics of the table, which
> doesn't sound good to me.  Of course we could say this is our
> restriction and say good-bye to users who hadn't run ANALYZE first,
> but it is too hard for a normal users to use it.  We may need
> quick-and-rough count(*) for this.

For Bernoulli sampling, SQL Standard says "Further, whether a given
row of RT is included in result of TF is independent of whether other
rows of RT are included in result of TF."

Which means BERNOULLI sampling looks essentially identical to using
 WHERE random() <= ($percent/100)

So my proposed implementation route for bernoulli sampling is to
literally add an AND-ed qual that does a random() test (and
repeatability also). That looks fairly simple and it is still
accurate, because it doesn't matter whether we do the indpendent test
to include the tuple before or after any other quals. I realise that
isn't a cool and hip approach, but it works and is exactly accurate.
Which would change the patch quite a bit.

Taking the random() approach would mean we don't rely on statistics either.

Thoughts?


SYSTEM sampling uses a completely different approach and is the really
interesting part of this feature.

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