On Fri, May 17, 2019 at 06:37:22PM -0700, Peter Geoghegan wrote:
> On Fri, May 17, 2019 at 6:36 PM Tom Lane wrote:
>> Will do so tomorrow. Should we back-patch this?
>
> I wouldn't, because I see no reason to. Somebody else might.
FWIW, I see no reason either for a back-patch.
--
Michael
sign
On Fri, May 17, 2019 at 6:36 PM Tom Lane wrote:
> Will do so tomorrow. Should we back-patch this?
I wouldn't, because I see no reason to. Somebody else might.
--
Peter Geoghegan
Peter Geoghegan writes:
> On Tue, Apr 30, 2019 at 3:22 PM Tom Lane wrote:
>> So now I'm feeling more favorable about the idea of adding a
>> PrepareTempTablespaces call to BufFileCreateTemp, and just stopping
>> with that. If we want to do more, I feel like it requires a
>> significant amount of
On Tue, Apr 30, 2019 at 3:22 PM Tom Lane wrote:
> So now I'm feeling more favorable about the idea of adding a
> PrepareTempTablespaces call to BufFileCreateTemp, and just stopping
> with that. If we want to do more, I feel like it requires a
> significant amount of rethinking about what the expe
I wrote:
> So maybe a reasonable compromise is to add the Assert(s) in fd.c as
> per previous patch, but *also* add PrepareTempTablespaces in
> BufFileCreateTemp, so that at least users of buffile.c are insulated
> from the issue. buffile.c is still kind of low-level, but it's not
> part of core i
Melanie Plageman writes:
> I also think that if there is a step that a caller should always take before
> calling a function, then there needs to be a very compelling reason not to
> move that step into the function itself.
Fair complaint.
> PrepareTempTablespaces should not be called in BufFile
On Fri, Apr 26, 2019 at 8:05 AM Tom Lane wrote:
> The version that I posted left it to GetNextTempTableSpace to assert
> that. That seemed cleaner to me than an Assert that has to depend
> on interXact.
>
> Running `make check` with [1] applied and one of the calls to
PrepareTempTablespaces comm
On Mon, Apr 29, 2019 at 12:31 PM Ashwin Agrawal wrote:
> Well the one thing I wish to point out explicitly is just taking fd.c
> changes from [1], and running make check hits no assertions and
> doesn't flag issue exist for gistbuildbuffers.c. Means its missing
> coverage and in future same can ha
On Thu, Apr 25, 2019 at 11:53 PM Michael Paquier wrote:
>
> On Thu, Apr 25, 2019 at 12:45:03PM -0400, Tom Lane wrote:
> > I still remain concerned that invoking catalog lookups from fd.c is a darn
> > bad idea, even if we have a fallback for it to work (for some value of
> > "work") in non-transac
On Fri, Apr 26, 2019 at 11:05:11AM -0400, Tom Lane wrote:
> Michael Paquier writes:
> The version that I posted left it to GetNextTempTableSpace to assert
> that. That seemed cleaner to me than an Assert that has to depend
> on interXact.
Okay, no objections for that approach as well. Are you p
Michael Paquier writes:
> I think that one piece is missing from the patch. Wouldn't it be
> better to add an assertion at the beginning of OpenTemporaryFile() to
> make sure that PrepareTempTablespaces() has been called when interXact
> is true? We could just go with that:
> Assert(!interXact |
On Thu, Apr 25, 2019 at 10:40:01AM -0700, Ashwin Agrawal wrote:
> Is there (easy) way to assert for that assumption? If yes, then can add the
> same and make it not rickety.
IsTransactionState() would be enough?
--
Michael
signature.asc
Description: PGP signature
On Thu, Apr 25, 2019 at 12:45:03PM -0400, Tom Lane wrote:
> I still remain concerned that invoking catalog lookups from fd.c is a darn
> bad idea, even if we have a fallback for it to work (for some value of
> "work") in non-transactional states. It's not really hard to envision
> that kind of thi
On Thu, Apr 25, 2019 at 9:45 AM Tom Lane wrote:
> However, by that argument we should change all 3 of these functions to
> set up the data. If we're eating the layering violation to the extent
> of letting OpenTemporaryFile call into commands/tablespace, then there's
> little reason for the othe
On Thu, Apr 25, 2019 at 9:19 AM Ashwin Agrawal wrote:
> Just to provide my opinion, since we are at intersection and can go
> either way on this. Second approach (just adding assert) only helps
> if the code path for ALL future callers gets excersied and test exist for
> the
> same, to expose pot
Ashwin Agrawal writes:
> On Wed, Apr 24, 2019 at 5:48 PM Peter Geoghegan wrote:
>> Like you, I find it hard to prefer one of the approaches over the
>> other, though I don't really know how to assess this layering
>> business. I'm glad that either approach will prevent oversights,
>> though.
> J
On Wed, Apr 24, 2019 at 5:48 PM Peter Geoghegan wrote:
> On Wed, Apr 24, 2019 at 12:17 PM Tom Lane wrote:
> > After a bit more thought it seemed like another answer would be to
> > make all three of those functions assert that the caller did the
> > right thing, as per attached. This addresses
Peter Geoghegan writes:
> ... I'm not
> seeing why the context that the PrepareTempTablespaces() catalog
> access occurs in actually matters.
The point there is that a catalog access might leak some amount of
memory. Probably not enough to be a big deal, but ...
regards,
On Wed, Apr 24, 2019 at 12:17 PM Tom Lane wrote:
> After a bit more thought it seemed like another answer would be to
> make all three of those functions assert that the caller did the
> right thing, as per attached. This addresses the layering-violation
> complaint, but might be more of a pain i
I wrote:
> Here's a draft patch for that.
>
> It's slightly ugly that this adds a dependency on commands/tablespace
> to fd.c, which is a pretty low-level module. I think wanting to avoid
> that layering violation might've been the reason for doing things the
> way they are. However, this gets ri
On Tue, Apr 23, 2019 at 1:06 PM Tom Lane wrote:
> There are three functions in fd.c that have a dependency on the
> temp tablespace info having been set up:
> OpenTemporaryFile
> GetTempTablespaces
> GetNextTempTableSpace
> This patch makes the first of those automatically
I wrote:
> Peter Geoghegan writes:
>> That doesn't seem like a particularly good or complete answer, though.
>> Perhaps it should simply be called within BufFileCreateTemp(). The
>> BufFile/fd.c layering is confusing in a number of ways IMV.
> I don't actually see why BufFileCreateTemp should do
Peter Geoghegan writes:
> On Mon, Apr 22, 2019 at 3:12 PM Melanie Plageman
> wrote:
>> PrepareTempTablespaces is called by most callers of BufFileCreateTemp, so I
>> was
>> wondering if there is a reason not to call it inside BufFileCreateTemp.
> The best answer I can think of is that a BufFile
On Mon, Apr 22, 2019 at 3:12 PM Melanie Plageman
wrote:
> PrepareTempTablespaces is called by most callers of BufFileCreateTemp, so I
> was
> wondering if there is a reason not to call it inside BufFileCreateTemp.
The best answer I can think of is that a BufFileCreateTemp() caller
might not want
Hi,
PrepareTempTablespaces is called by most callers of BufFileCreateTemp, so I
was
wondering if there is a reason not to call it inside BufFileCreateTemp.
As a developer using BufFileCreateTemp to write code that will create spill
files, it was easy to forget the extra step of checking the temp_t
25 matches
Mail list logo
