Обсуждение: pgsql: Separate block sampling functions

Separate block sampling functions

Refactoring ahead of tablesample patch

Requested and reviewed by Michael Paquier

Petr Jelinek

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/83e176ec18d2a91dbea1d0d1bd94c38dc47cd77c

Modified Files
--------------
contrib/file_fdw/file_fdw.c         |    9 +-
contrib/postgres_fdw/postgres_fdw.c |   10 +-
src/backend/commands/analyze.c      |  225 +---------------------------------
src/backend/utils/misc/Makefile     |    2 +-
src/backend/utils/misc/sampling.c   |  226 +++++++++++++++++++++++++++++++++++
src/include/commands/vacuum.h       |    3 -
src/include/utils/sampling.h        |   44 +++++++
7 files changed, 287 insertions(+), 232 deletions(-)

Simon Riggs <simon@2ndQuadrant.com> writes:
> Separate block sampling functions

This patch broke buildfarm member crake.

            regards, tom lane

On Fri, May 15, 2015 at 12:03 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 15 May 2015 at 03:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Simon Riggs <simon@2ndQuadrant.com> writes:
>> > Separate block sampling functions
>>
>> This patch broke buildfarm member crake.
>
>
> OK, thanks. I missed that amongst the other unrelated failures. Looking now.

This needs a patch to file_text_array_fdw which I think is available here:
https://github.com/adunstan/file_text_array_fdw
Simon, do you mind if I send a pull request?
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, May 15, 2015 at 12:03 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 15 May 2015 at 03:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>>> Separate block sampling functions

>>> This patch broke buildfarm member crake.

>> OK, thanks. I missed that amongst the other unrelated failures. Looking now.

> This needs a patch to file_text_array_fdw which I think is available here:
> https://github.com/adunstan/file_text_array_fdw
> Simon, do you mind if I send a pull request?

TBH, I think that this patch itself was a bad idea and should be reverted.
I don't object to changing APIs used by external modules when there's a
good reason to break them, but having looked at this patch all I see is
change for the sake of change.  What new functionality have you introduced?

Or to put it more baldly: it's likely that you've broken quite a large
number of third-party FDWs, not just this one.  A lot of people have
probably copied-and-pasted what was in the contrib FDWs.

            regards, tom lane

On Fri, May 15, 2015 at 12:12 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 15 May 2015 at 04:06, Michael Paquier <michael.paquier@gmail.com> wrote:
>>
>> On Fri, May 15, 2015 at 12:03 PM, Simon Riggs <simon@2ndquadrant.com>
>> wrote:
>> > On 15 May 2015 at 03:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >>
>> >> Simon Riggs <simon@2ndQuadrant.com> writes:
>> >> > Separate block sampling functions
>> >>
>> >> This patch broke buildfarm member crake.
>> >
>> >
>> > OK, thanks. I missed that amongst the other unrelated failures. Looking
>> > now.
>>
>> This needs a patch to file_text_array_fdw which I think is available here:
>> https://github.com/adunstan/file_text_array_fdw
>> Simon, do you mind if I send a pull request?
>
>
> Hmm, guess that explains it then, I was just scratching my head.
>
> Yes please sort that out.

Sent a patch here:
https://github.com/adunstan/file_text_array_fdw/pull/1
--
Michael

On Fri, May 15, 2015 at 12:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Fri, May 15, 2015 at 12:03 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> On 15 May 2015 at 03:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>>>> Separate block sampling functions
>
>>>> This patch broke buildfarm member crake.
>
>>> OK, thanks. I missed that amongst the other unrelated failures. Looking now.
>
>> This needs a patch to file_text_array_fdw which I think is available here:
>> https://github.com/adunstan/file_text_array_fdw
>> Simon, do you mind if I send a pull request?
>
> TBH, I think that this patch itself was a bad idea and should be reverted.
> I don't object to changing APIs used by external modules when there's a
> good reason to break them, but having looked at this patch all I see is
> change for the sake of change.  What new functionality have you introduced?

If you look at the TABLESAMPLE patch, separating the block sampling
into a separate facility makes quite some sense.

> Or to put it more baldly: it's likely that you've broken quite a large
> number of third-party FDWs, not just this one.  A lot of people have
> probably copied-and-pasted what was in the contrib FDWs.

make_foreignscan() has been changed as well by 1a8a4e5c..
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, May 15, 2015 at 12:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> TBH, I think that this patch itself was a bad idea and should be reverted.
>> I don't object to changing APIs used by external modules when there's a
>> good reason to break them, but having looked at this patch all I see is
>> change for the sake of change.  What new functionality have you introduced?

> If you look at the TABLESAMPLE patch, separating the block sampling
> into a separate facility makes quite some sense.

Sure, but there was no need to break FDWs that were using an API that
was perfectly reasonable for ANALYZE support.

Had you not made random changes to the argument lists, we could have
solved this with some compatibility macros...

>> Or to put it more baldly: it's likely that you've broken quite a large
>> number of third-party FDWs, not just this one.  A lot of people have
>> probably copied-and-pasted what was in the contrib FDWs.

> make_foreignscan() has been changed as well by 1a8a4e5c..

The difference there was that that was specifically adding a new feature
of value to FDWs.  This is just drive-by breakage.

            regards, tom lane

On 05/15/2015 06:04 AM, Simon Riggs wrote:
> On 15 May 2015 at 04:59, Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
>
>     The difference there was that that was specifically adding a new
>     feature
>     of value to FDWs.  This is just drive-by breakage.
>
>
> I think that comment is reasonable. I will continue with my commits of
> tablesample, then return to see if we can improve/revert the API
> breakage.
>
>


OK, good, but I'm not going to accept Michael's pull request until the
API is stable.

cheers

andrew

On Fri, May 15, 2015 at 8:44 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> On 05/15/2015 06:04 AM, Simon Riggs wrote:
>>
>> On 15 May 2015 at 04:59, Tom Lane <tgl@sss.pgh.pa.us
>> <mailto:tgl@sss.pgh.pa.us>> wrote:
>>
>>     The difference there was that that was specifically adding a new
>>     feature
>>     of value to FDWs.  This is just drive-by breakage.
>>
>>
>> I think that comment is reasonable. I will continue with my commits of
>> tablesample, then return to see if we can improve/revert the API breakage.
>>
>>
>
>
> OK, good, but I'm not going to accept Michael's pull request until the API
> is stable.

Well, I guess that this is going to be incorrect in any case now.
Still any API change can be done cleanly in a matter of minutes for
this FDW.
--
Michael

On 05/15/2015 07:44 AM, Andrew Dunstan wrote:
>
> On 05/15/2015 06:04 AM, Simon Riggs wrote:
>> On 15 May 2015 at 04:59, Tom Lane <tgl@sss.pgh.pa.us
>> <mailto:tgl@sss.pgh.pa.us>> wrote:
>>
>>     The difference there was that that was specifically adding a new
>>     feature
>>     of value to FDWs.  This is just drive-by breakage.
>>
>>
>> I think that comment is reasonable. I will continue with my commits
>> of tablesample, then return to see if we can improve/revert the API
>> breakage.
>>
>>
>
>
> OK, good, but I'm not going to accept Michael's pull request until the
> API is stable.
>
>



What is the current state of this? Are we sticking with what Tom
classified as drive-by breakage?

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> What is the current state of this? Are we sticking with what Tom
> classified as drive-by breakage?

Since I was the one complaining, I'm willing to do the legwork to
insert a compatibility shim.  I probably won't get to it today though,
this being release wrap day.  In the meantime please leave
FileTextArrayFDW as-is.

            regards, tom lane

I wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> What is the current state of this? Are we sticking with what Tom
>> classified as drive-by breakage?

> Since I was the one complaining, I'm willing to do the legwork to
> insert a compatibility shim.  I probably won't get to it today though,
> this being release wrap day.  In the meantime please leave
> FileTextArrayFDW as-is.

I put in a shim --- crake should be happy again on its next run.

            regards, tom lane