Re: TABLESAMPLE patch

On 06/01/15 08:51, Michael Paquier wrote:
> On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek <petr(at)2ndquadrant(dot)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