On Sun, Jan 31, 2016 at 11:50 AM, Tom Lane wrote:
> Noah Misch writes:
>> On Sat, Jan 30, 2016 at 04:28:47PM -0500, Robert Haas wrote:
>>> I think I've
>>> pretty much said what I have to say about this; if nothing I wrote up
>>> until now swayed you, it's unlikely that anything else I say after
Noah Misch writes:
> On Sat, Jan 30, 2016 at 04:28:47PM -0500, Robert Haas wrote:
>> I think I've
>> pretty much said what I have to say about this; if nothing I wrote up
>> until now swayed you, it's unlikely that anything else I say after
>> this point will either.
> Say I drop the parts that c
On Sat, Jan 30, 2016 at 04:28:47PM -0500, Robert Haas wrote:
> I think I've
> pretty much said what I have to say about this; if nothing I wrote up
> until now swayed you, it's unlikely that anything else I say after
> this point will either.
Say I drop the parts that change the binary. Does the
On Sat, Jan 30, 2016 at 2:13 PM, Noah Misch wrote:
> You could offer that paragraph as an objection to almost all Assert(), elog(),
> and automated tests. Why levy it against this patch? The valuable ways
> assertions and tests supplement review are well-established.
Sure, that's true, but I do
On Sat, Jan 30, 2016 at 07:37:45AM -0500, Robert Haas wrote:
> On Fri, Jan 29, 2016 at 11:19 PM, Noah Misch wrote:
> > As you say, forbidding things makes friction in the event that someone comes
> > along wanting to do the forbidden thing. Forbidding things also simplifies
> > the system, making
On Fri, Jan 29, 2016 at 11:19 PM, Noah Misch wrote:
> As you say, forbidding things makes friction in the event that someone comes
> along wanting to do the forbidden thing. Forbidding things also simplifies
> the system, making it easier to verify. This decision should depend on the
> API's aud
On Thu, Jan 28, 2016 at 11:12:15AM -0500, Robert Haas wrote:
> I'm honestly failing to understand why we should change anything at
> all. I don't believe that doing something more severe than marking
> the portal failed actually improves anything. I suppose if I had to
> pick between what you pro
On Wed, Jan 27, 2016 at 11:47 PM, Noah Misch wrote:
> On Wed, Jan 27, 2016 at 11:04:33PM -0500, Robert Haas wrote:
>> +Assert(portal->status != PORTAL_ACTIVE);
>> if (portal->status == PORTAL_ACTIVE)
>> MarkPortalFailed(portal);
>>
>> Now that
On Wed, Jan 27, 2016 at 11:04:33PM -0500, Robert Haas wrote:
> +Assert(portal->status != PORTAL_ACTIVE);
> if (portal->status == PORTAL_ACTIVE)
> MarkPortalFailed(portal);
>
> Now that just looks kooky to me. We assert that the portal isn't
>
On Thu, Jan 28, 2016 at 12:57:36PM +0900, Michael Paquier wrote:
> On Thu, Jan 28, 2016 at 12:40 PM, Noah Misch wrote:
> + * fresh transaction. No part of core PostgreSQL functions that way,
> + * though it's a fair thing to want. Such code would wish the portal
> From the point
On Wed, Jan 27, 2016 at 10:40 PM, Noah Misch wrote:
> On Sun, Jan 03, 2016 at 12:35:56AM -0500, Noah Misch wrote:
>> On Sat, Jan 02, 2016 at 07:22:13PM -0500, Tom Lane wrote:
>> > Noah Misch writes:
>> > > I am inclined to add an Assert(portal->status != PORTAL_ACTIVE) to
>> > > emphasize
>> > >
On Thu, Jan 28, 2016 at 12:40 PM, Noah Misch wrote:
> On Sun, Jan 03, 2016 at 12:35:56AM -0500, Noah Misch wrote:
>> On Sat, Jan 02, 2016 at 07:22:13PM -0500, Tom Lane wrote:
>> > Noah Misch writes:
>> > > I am inclined to add an Assert(portal->status != PORTAL_ACTIVE) to
>> > > emphasize
>> > >
On Sun, Jan 03, 2016 at 12:35:56AM -0500, Noah Misch wrote:
> On Sat, Jan 02, 2016 at 07:22:13PM -0500, Tom Lane wrote:
> > Noah Misch writes:
> > > I am inclined to add an Assert(portal->status != PORTAL_ACTIVE) to
> > > emphasize
> > > that this is backup only. MarkPortalActive() callers remai
On Sat, Jan 02, 2016 at 07:22:13PM -0500, Tom Lane wrote:
> Noah Misch writes:
> > I am inclined to add an Assert(portal->status != PORTAL_ACTIVE) to emphasize
> > that this is backup only. MarkPortalActive() callers remain responsible for
> > updating the status to something else before relinqui
Noah Misch writes:
> I am inclined to add an Assert(portal->status != PORTAL_ACTIVE) to emphasize
> that this is backup only. MarkPortalActive() callers remain responsible for
> updating the status to something else before relinquishing control.
No, I do not think that would be an improvement.
On Sat, Jan 02, 2016 at 01:46:07PM -0500, Tom Lane wrote:
> Noah Misch writes:
> > On Thu, Sep 03, 2015 at 11:04:11PM -0400, Tom Lane wrote:
> >> *** AtSubAbort_Portals(SubTransactionId mySu
>
> >> --- 909,966
> >> {
> >> Portal portal = hentry->portal;
> >>
> >> +
Noah Misch writes:
> On Thu, Sep 03, 2015 at 11:04:11PM -0400, Tom Lane wrote:
>> *** AtSubAbort_Portals(SubTransactionId mySu
>> --- 909,966
>> {
>> Portal portal = hentry->portal;
>>
>> +/* Was it created in this subtransaction? */
>> if (portal->crea
On Thu, Sep 03, 2015 at 11:04:11PM -0400, Tom Lane wrote:
> *** AtSubAbort_Portals(SubTransactionId mySu
> --- 909,966
> {
> Portal portal = hentry->portal;
>
> + /* Was it created in this subtransaction? */
> if (portal->c
Michael Paquier writes:
> On Sat, Sep 5, 2015 at 3:41 PM, Jim Nasby wrote:
>> I was about to test this and was verifying that the crash still worked
>> when I noticed this in the logs (9.4.1, not HEAD). Not sure if there's any
>> realion here or not...
>>
>> WARNING: relation "pg_proc" page 121
On Sat, Sep 5, 2015 at 3:41 PM, Jim Nasby wrote:
> I was about to test this and was verifying that the crash still worked
> when I noticed this in the logs (9.4.1, not HEAD). Not sure if there's any
> realion here or not...
>
> WARNING: relation "pg_proc" page 121 is uninitialized --- fixing
> W
On 9/4/15 12:41 PM, Tom Lane wrote:
I wrote:
After review of all the callers of RelationClearRelation, it seems like
most of them already have sufficient guards to prevent triggering of the
Assert. The ones that lack such tests are AtEOXact_cleanup and
AtEOSubXact_cleanup. So what I'm now thin
I wrote:
>> After review of all the callers of RelationClearRelation, it seems like
>> most of them already have sufficient guards to prevent triggering of the
>> Assert. The ones that lack such tests are AtEOXact_cleanup and
>> AtEOSubXact_cleanup. So what I'm now thinking is that those should d
I wrote:
> After review of all the callers of RelationClearRelation, it seems like
> most of them already have sufficient guards to prevent triggering of the
> Assert. The ones that lack such tests are AtEOXact_cleanup and
> AtEOSubXact_cleanup. So what I'm now thinking is that those should do
>
Michael Paquier writes:
> On Fri, Sep 4, 2015 at 10:52 PM, Tom Lane wrote:
>> Also, I am very strongly tempted to convert the original problem-causing
>> Assert in relcache.c into a regular runtime test-and-elog. If we're wrong
>> about having sealed off this hole completely, I'd much rather see
On Fri, Sep 4, 2015 at 10:52 PM, Tom Lane wrote:
> Also, I am very strongly tempted to convert the original problem-causing
> Assert in relcache.c into a regular runtime test-and-elog. If we're wrong
> about having sealed off this hole completely, I'd much rather see the
> result be an elog than
Michael Paquier writes:
> On Fri, Sep 4, 2015 at 12:04 PM, Tom Lane wrote:
>> Attached is an updated patch with comments to this effect and some
>> minor other code cleanup (mainly, not assuming that CurrentResourceOwner
>> is the right thing to use in AtSubAbort_Portals).
> Thanks! This looks g
On Fri, Sep 4, 2015 at 12:04 PM, Tom Lane wrote:
> I wrote:
> > Hmm. I am not feeling especially comfortable about this: it's not clear
> > that there's anything preventing a suspended portal from containing a
> > dangerous reference. However, a quick trial did not show that it was
> > possible
I wrote:
> Hmm. I am not feeling especially comfortable about this: it's not clear
> that there's anything preventing a suspended portal from containing a
> dangerous reference. However, a quick trial did not show that it was
> possible to break it --- in the cases I tried, we detected that a cac
Michael Paquier writes:
> On Fri, Sep 4, 2015 at 9:57 AM, Tom Lane wrote:
>> On reflection I think that the tracking of activeSubid in my patch is
>> probably overkill if we're attacking it this way. We can just have
>> AtSubAbort_Portals fail any ACTIVE portal regardless of subxact level,
>> wh
On Fri, Sep 4, 2015 at 9:57 AM, Tom Lane wrote:
> On reflection I think that the tracking of activeSubid in my patch is
> probably overkill if we're attacking it this way. We can just have
> AtSubAbort_Portals fail any ACTIVE portal regardless of subxact level,
> which is pretty analogous to wha
Michael Paquier writes:
> On Fri, Sep 4, 2015 at 6:51 AM, Tom Lane wrote:
>> Ideas?
> Yes. This diff on top of your patch:
> @@ -922,8 +922,7 @@ AtSubAbort_Portals(SubTransactionId mySubid,
> * must be forced into FAILED state, for
> the same reasons
>
On Fri, Sep 4, 2015 at 6:51 AM, Tom Lane wrote:
> I experimented with the attached patch. It seems to work to stop the
> crash Michael exhibited (I've not tried to duplicate Jim's original
> example, though). However, it also causes a regression test failure,
> because transactions.sql does thi
I wrote:
> I'm inclined to think that the only real fix for this type of problem
> is that at subtransaction abort, we have to prevent further execution
> of any Portals that have executed at all within the subxact (ie force
> them into PORTAL_FAILED state and clean out their resources, see
> MarkP
Michael Paquier writes:
> Finally I have been able to crack down the problem, and it can be
> reproduced with the following test case for example:
Hm. It looks like the problem is that we're executing the function
within the Portal created for cursor "h", and that Portal is older
than the subtra
On Thu, Sep 3, 2015 at 1:46 AM, Jim Nasby wrote:
> The double set of exceptions seems to be critical; if instead of calling
> tf.get(invoice) (which recursively does tf.get(customer)) I define the
> cursor to call tf.get(customer) directly there's no assert.
Finally I have been able to crack down
On 9/2/15 2:56 PM, Jim Nasby wrote:
On 9/2/15 2:17 PM, Alvaro Herrera wrote:
Michael Paquier wrote:
I haven't written yet a test case but I think that we could reproduce
that simply by having a relation referenced in the exception block of
a first function, calling a second function that itsel
On 9/2/15 2:17 PM, Alvaro Herrera wrote:
Michael Paquier wrote:
I haven't written yet a test case but I think that we could reproduce
that simply by having a relation referenced in the exception block of
a first function, calling a second function that itself raises an
exception, causing the re
Michael Paquier wrote:
> I haven't written yet a test case but I think that we could reproduce
> that simply by having a relation referenced in the exception block of
> a first function, calling a second function that itself raises an
> exception, causing the referencing error.
Hm, so function 2
On Wed, Sep 2, 2015 at 6:21 AM, Michael Paquier wrote:
> Yes, that's what I have been looking at actually by having some markers in
> relcache.c. The reference count of this relation get incremented here:
So, I have been playing more with this code, and as mentioned by
Andres this test case goes
On Wed, Sep 2, 2015 at 6:13 AM, Jim Nasby wrote:
> On 9/1/15 11:59 PM, Michael Paquier wrote:
>>
>> On Wed, Sep 2, 2015 at 12:59 PM, Jim Nasby wrote:
>>>
>>> On 9/1/15 8:42 PM, Michael Paquier wrote:
>>> The crash is triggered by having an exception raised in this particular
>>> call
>>> stack. Si
On 9/1/15 11:59 PM, Michael Paquier wrote:
On Wed, Sep 2, 2015 at 12:59 PM, Jim Nasby wrote:
On 9/1/15 8:42 PM, Michael Paquier wrote:
The crash is triggered by having an exception raised in this particular call
stack. Since there's no syntax error in master/0.2.1, there's no assert
failure eith
On Wed, Sep 2, 2015 at 12:59 PM, Jim Nasby wrote:
> On 9/1/15 8:42 PM, Michael Paquier wrote:
> The crash is triggered by having an exception raised in this particular call
> stack. Since there's no syntax error in master/0.2.1, there's no assert
> failure either. Were you running asserts?
I thoug
On 9/1/15 8:42 PM, Michael Paquier wrote:
test_factory is a jungle to me. Perhaps you could just extract a
self-contained test case? It does not matter if the file is long as
long as the problem can be easily reproduced.
Sorry, more info on what's happening here.
The idea behind test_factory i
On 9/1/15 8:42 PM, Michael Paquier wrote:
On Wed, Sep 2, 2015 at 9:39 AM, Michael Paquier
wrote:
On Wed, Sep 2, 2015 at 7:23 AM, Jim Nasby wrote:
Well nuts, pretty sure that means the error isn't reproducing for you. :/ Do
you maybe have unusual config options or postgresql.conf options?
Yep
On Wed, Sep 2, 2015 at 9:39 AM, Michael Paquier
wrote:
> On Wed, Sep 2, 2015 at 7:23 AM, Jim Nasby wrote:
>> Well nuts, pretty sure that means the error isn't reproducing for you. :/ Do
>> you maybe have unusual config options or postgresql.conf options?
>
> Yep. And I have found one reason why it
On Wed, Sep 2, 2015 at 7:23 AM, Jim Nasby wrote:
> Well nuts, pretty sure that means the error isn't reproducing for you. :/ Do
> you maybe have unusual config options or postgresql.conf options?
Yep. And I have found one reason why it was not working, I have been
using --extra-version with a cust
On 9/1/15 1:08 AM, Michael Paquier wrote:
On Sun, Aug 30, 2015 at 1:06 AM, Jim Nasby wrote:
Steps to reproduce:
Download https://github.com/BlueTreble/test_factory/archive/crash.zip
Unzip, cd into directory
pgxn install pgtap (or just make test)
FWIW, make test fails:
! ERROR: 42703: column "
On Sun, Aug 30, 2015 at 1:06 AM, Jim Nasby wrote:
> Steps to reproduce:
> Download https://github.com/BlueTreble/test_factory/archive/crash.zip
> Unzip, cd into directory
> pgxn install pgtap (or just make test)
FWIW, make test fails:
! ERROR: 42703: column "c_data_table_name" does not exist
! LI
On 8/29/15 11:31 AM, Andres Freund wrote:
Hi,
On 2015-08-29 11:06:05 -0500, Jim Nasby wrote:
Stack trace below. Relevant assert:
What exact revision is this on? Either there's some aggressive inlining
going on, or these lines don't entirely match up with HEAD.
Oops, that was 9.4.1. Trace fr
Hi,
On 2015-08-29 11:06:05 -0500, Jim Nasby wrote:
> Stack trace below. Relevant assert:
What exact revision is this on? Either there's some aggressive inlining
going on, or these lines don't entirely match up with HEAD.
That's compiled with optimization, isn't it? Could you compile with -O0?
C
On 8/29/15 8:04 AM, Tom Lane wrote:
Michael Paquier writes:
Ah, OK, you meant this file... Yes I was able to receive it as well in your
original email. I'll try to investigate further later, but Tom may beat me
first. He usually does.
Jim had indicated the bug wasn't reproducible on the stren
Michael Paquier writes:
> Ah, OK, you meant this file... Yes I was able to receive it as well in your
> original email. I'll try to investigate further later, but Tom may beat me
> first. He usually does.
Jim had indicated the bug wasn't reproducible on the strength of that
info, so I was waiting
On Sat, Aug 29, 2015 at 11:18 AM, Jim Nasby
wrote:
> On 8/28/15 8:39 PM, Tom Lane wrote:
>
>> Michael Paquier writes:
>>
>>> On Sat, Aug 29, 2015 at 5:02 AM, Jim Nasby
>>> wrote:
>>>
Looks like a 98k file won't get through the list...
>>>
>> Is it compressed? Note that we have sometim
On 8/28/15 8:39 PM, Tom Lane wrote:
Michael Paquier writes:
On Sat, Aug 29, 2015 at 5:02 AM, Jim Nasby wrote:
Looks like a 98k file won't get through the list...
Is it compressed? Note that we have sometimes larger patches than that, but
perhaps those had special permissions by the admins
Michael Paquier writes:
> On Sat, Aug 29, 2015 at 5:02 AM, Jim Nasby wrote:
>> Looks like a 98k file won't get through the list...
> Is it compressed? Note that we have sometimes larger patches than that, but
> perhaps those had special permissions by the admins of this list.
Messages significa
On Sat, Aug 29, 2015 at 5:02 AM, Jim Nasby wrote:
> Looks like a 98k file won't get through the list...
Is it compressed? Note that we have sometimes larger patches than that, but
perhaps those had special permissions by the admins of this list.
--
Michael
56 matches
Mail list logo
