Thread: 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 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.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
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.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 15 May 2015 at 04:59, Tom Lane <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.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
On 18 May 2015 at 10:53, Tom Lane <tgl@sss.pgh.pa.us> 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.
Sorry, I was hoping for input from Petr but he's just started leave, so its up to us.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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