Re: missing locking in at least INSERT INTO view WITH CHECK

Hi,

Using the same debugging hack^Wpatch (0001) as in the matview patch
(0002) an hour or so ago I noticed that INSERT INTO view WITH CHECK
doesn't lock the underlying relations properly.

I've attached a sort-of-working (0003) hack but I really doubt it's the
correct approach, I don't really know enough about that area of the
code.
This looks like something that needs to be fixed.

Also attached is 0004 which just adds a heap_lock() around a newly
created temporary table in the matview code which shouldn't be required
for correctness but gives warm and fuzzy feelings as well as less
debugging noise.

Wouldn't it be a good idea to tack such WARNINGs (in a proper and clean
form) to index_open (checking the underlying relation is locked),
relation_open(..., NoLock) (checking the relation has previously been
locked) and maybe RelationIdGetRelation() when cassert is enabled? ISTM
we frequently had bugs around this.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-10-23 03:18:55 +0200, Andres Freund wrote:
> (000[1-4])

Attached.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 23 October 2013 02:18, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> Using the same debugging hack^Wpatch (0001) as in the matview patch
> (0002) an hour or so ago I noticed that INSERT INTO view WITH CHECK
> doesn't lock the underlying relations properly.
>
> I've attached a sort-of-working (0003) hack but I really doubt it's the
> correct approach, I don't really know enough about that area of the
> code.
> This looks like something that needs to be fixed.
>

Hmm, my first thought is that rewriteTargetView() should be calling
AcquireRewriteLocks() on viewquery, before doing too much with it.
There may be sub-queries in viewquery's quals (and also now in its
targetlist) and I don't think the relations referred to by those
sub-queries are getting locked.

I think that any code that is doing anything significant with a rule
action's query needs to think about locking the query's relations. I
did a quick search and the only suspicious code I found was the
matview and auto-updatable view code.

Regards,
Dean

> Also attached is 0004 which just adds a heap_lock() around a newly
> created temporary table in the matview code which shouldn't be required
> for correctness but gives warm and fuzzy feelings as well as less
> debugging noise.
>
> Wouldn't it be a good idea to tack such WARNINGs (in a proper and clean
> form) to index_open (checking the underlying relation is locked),
> relation_open(..., NoLock) (checking the relation has previously been
> locked) and maybe RelationIdGetRelation() when cassert is enabled? ISTM
> we frequently had bugs around this.
>
> Greetings,
>
> Andres Freund
>
> --
> Andres Freund http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote:
> On 23 October 2013 02:18, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > Hi,
> >
> > Using the same debugging hack^Wpatch (0001) as in the matview patch
> > (0002) an hour or so ago I noticed that INSERT INTO view WITH CHECK
> > doesn't lock the underlying relations properly.
> >
> > I've attached a sort-of-working (0003) hack but I really doubt it's the
> > correct approach, I don't really know enough about that area of the
> > code.
> > This looks like something that needs to be fixed.
> >
>
> Hmm, my first thought is that rewriteTargetView() should be calling
> AcquireRewriteLocks() on viewquery, before doing too much with it.
> There may be sub-queries in viewquery's quals (and also now in its
> targetlist) and I don't think the relations referred to by those
> sub-queries are getting locked.

Well, that wouldn't follow the currently documented rule ontop
of QueryRewrite:
* NOTE: the parsetree must either have come straight from the parser,
* or have been scanned by AcquireRewriteLocks to acquire suitable locks.

It might still be the right thing to do, but it seems suspicious that
the rules need to be tweaked like that.

> I think that any code that is doing anything significant with a rule
> action's query needs to think about locking the query's relations. I
> did a quick search and the only suspicious code I found was the
> matview and auto-updatable view code.

Yea, that were the locations the debugging patch cried on...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 23 October 2013 21:08, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote:
>> On 23 October 2013 02:18, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > Hi,
>> >
>> > Using the same debugging hack^Wpatch (0001) as in the matview patch
>> > (0002) an hour or so ago I noticed that INSERT INTO view WITH CHECK
>> > doesn't lock the underlying relations properly.
>> >
>> > I've attached a sort-of-working (0003) hack but I really doubt it's the
>> > correct approach, I don't really know enough about that area of the
>> > code.
>> > This looks like something that needs to be fixed.
>> >
>>
>> Hmm, my first thought is that rewriteTargetView() should be calling
>> AcquireRewriteLocks() on viewquery, before doing too much with it.
>> There may be sub-queries in viewquery's quals (and also now in its
>> targetlist) and I don't think the relations referred to by those
>> sub-queries are getting locked.
>
> Well, that wouldn't follow the currently documented rule ontop
> of QueryRewrite:
> * NOTE: the parsetree must either have come straight from the parser,
> * or have been scanned by AcquireRewriteLocks to acquire suitable locks.
>
> It might still be the right thing to do, but it seems suspicious that
> the rules need to be tweaked like that.
>

Well it matches what already happens in other places in the rewriter
--- see rewriteRuleAction() and ApplyRetrieveRule(). It's precisely
because the rule action's query hasn't come from the parser that it
needs to be processed in this way.

Regards,
Dean

On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote:
> On 23 October 2013 21:08, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote:
> >> Hmm, my first thought is that rewriteTargetView() should be calling
> >> AcquireRewriteLocks() on viewquery, before doing too much with it.
> >> There may be sub-queries in viewquery's quals (and also now in its
> >> targetlist) and I don't think the relations referred to by those
> >> sub-queries are getting locked.
> >
> > Well, that wouldn't follow the currently documented rule ontop
> > of QueryRewrite:
> > * NOTE: the parsetree must either have come straight from the parser,
> > * or have been scanned by AcquireRewriteLocks to acquire suitable locks.
> >
> > It might still be the right thing to do, but it seems suspicious that
> > the rules need to be tweaked like that.
> >
>
> Well it matches what already happens in other places in the rewriter
> --- see rewriteRuleAction() and ApplyRetrieveRule(). It's precisely
> because the rule action's query hasn't come from the parser that it
> needs to be processed in this way.

I really don't know that are of code that well, fortunately I never had
to wade around it much so far...

Reading your explanation and a bit of the code your plan sound sane. Are
you going to propose a patch?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 24 October 2013 18:28, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote:
>> On 23 October 2013 21:08, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote:
>> >> Hmm, my first thought is that rewriteTargetView() should be calling
>> >> AcquireRewriteLocks() on viewquery, before doing too much with it.
>> >> There may be sub-queries in viewquery's quals (and also now in its
>> >> targetlist) and I don't think the relations referred to by those
>> >> sub-queries are getting locked.
>> >
>> > Well, that wouldn't follow the currently documented rule ontop
>> > of QueryRewrite:
>> > * NOTE: the parsetree must either have come straight from the parser,
>> > * or have been scanned by AcquireRewriteLocks to acquire suitable locks.
>> >
>> > It might still be the right thing to do, but it seems suspicious that
>> > the rules need to be tweaked like that.
>> >
>>
>> Well it matches what already happens in other places in the rewriter
>> --- see rewriteRuleAction() and ApplyRetrieveRule(). It's precisely
>> because the rule action's query hasn't come from the parser that it
>> needs to be processed in this way.
>
> I really don't know that are of code that well, fortunately I never had
> to wade around it much so far...
>
> Reading your explanation and a bit of the code your plan sound sane. Are
> you going to propose a patch?
>

I thought about making rewriteTargetView() call AcquireRewriteLocks(),
but on closer inspection I think that is probably over the top. 90% of
the code in AcquireRewriteLocks() is to process the query's RTEs, but
rewriteTargetView() is already locking the single RTE in viewquery
anyway. Also AcquireRewriteLocks() would acquire the wrong kind of
lock on that RTE, since viewquery is a SELECT, but we want an
exclusive lock because the RTE is about to become the outer query's
result relation. AcquireRewriteLocks() also processes CTEs, but we
know that we won't have any of those in rewriteTargetView(). So I
think the only thing missing from rewriteTargetView() is to lock any
relations from any sublink subqueries in viewquery using
acquireLocksOnSubLinks() -- patch attached.

Regards,
Dean

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> the matview patch (0002)

This is definitely needed as a bug fix.  Will adjust comments and
commit, back-patched to 9.3.

Thanks for spotting this, and thanks for the fix!

> Also attached is 0004 which just adds a heap_lock() around a
> newly created temporary table in the matview code which shouldn't
> be required for correctness but gives warm and fuzzy feelings as
> well as less debugging noise.

Will think about this.  I agree is is probably worth doing
something to reduce the noise when looking for cases that actually
matter.

> Wouldn't it be a good idea to tack such WARNINGs (in a proper and
> clean form) to index_open (checking the underlying relation is
> locked), relation_open(..., NoLock) (checking the relation has
> previously been locked) and maybe RelationIdGetRelation() when
> cassert is enabled? ISTM we frequently had bugs around this.

It would be nice to have such omissions pointed out during early
testing.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-11-02 17:05:24 -0700, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> > the matview patch (0002)
>
> This is definitely needed as a bug fix.  Will adjust comments and
> commit, back-patched to 9.3.

Thanks.

> > Also attached is 0004 which just adds a heap_lock() around a
> > newly created temporary table in the matview code which shouldn't
> > be required for correctness but gives warm and fuzzy feelings as
> > well as less debugging noise.
>
> Will think about this.  I agree is is probably worth doing
> something to reduce the noise when looking for cases that actually
> matter.

It's pretty much free, so I don't think there really is any reason to
deviate from other parts of the code. Note how e.g. copy_heap_data(),
DefineRelation() and ATRewriteTable() all lock the new relation, even if
it just has been created and is (and in the latter two cases will always
be) invisible.

> > Wouldn't it be a good idea to tack such WARNINGs (in a proper and
> > clean form) to index_open (checking the underlying relation is
> > locked), relation_open(..., NoLock) (checking the relation has
> > previously been locked) and maybe RelationIdGetRelation() when
> > cassert is enabled? ISTM we frequently had bugs around this.
>
> It would be nice to have such omissions pointed out during early
> testing.

Will try to come up with something.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-11-02 17:05:24 -0700, Kevin Grittner wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

>>> Also attached is 0004 which just adds a heap_lock() around a
>>> newly created temporary table in the matview code which
>>> shouldn't be required for correctness but gives warm and fuzzy
>>> feelings as well as less debugging noise.
>>
>> Will think about this.  I agree is is probably worth doing
>> something to reduce the noise when looking for cases that
>> actually matter.
>
> It's pretty much free, so I don't think there really is any
> reason to deviate from other parts of the code. Note how e.g.
> copy_heap_data(), DefineRelation() and ATRewriteTable() all lock
> the new relation, even if it just has been created and is (and in
> the latter two cases will always be) invisible.

None of those locations are using heap_open() as the first
parameter to heap_close().  That looks kinda iffy, and the fact
that it is not yet done anywhere in the code gives me pause.  You
probably had a reason for preferring that to a simple call to
LockRelationOid(), but I'm not seeing what that reason is.  I also
don't understand the use of the lockmode variable here.

I'm thinking of something like the attached instead.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-11-05 11:56:25 -0800, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-11-02 17:05:24 -0700, Kevin Grittner wrote:
> >> Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> >>> Also attached is 0004 which just adds a heap_lock() around a
> >>> newly created temporary table in the matview code which
> >>> shouldn't be required for correctness but gives warm and fuzzy
> >>> feelings as well as less debugging noise.
> >>
> >> Will think about this.  I agree is is probably worth doing
> >> something to reduce the noise when looking for cases that
> >> actually matter.
> >
> > It's pretty much free, so I don't think there really is any
> > reason to deviate from other parts of the code. Note how e.g.
> > copy_heap_data(), DefineRelation() and ATRewriteTable() all lock
> > the new relation, even if it just has been created and is (and in
> > the latter two cases will always be) invisible.
>
> None of those locations are using heap_open() as the first
> parameter to heap_close().

Oh! Sure, what I'd posted was just an absolutely crude hack that surely
isn't committable.

> I'm thinking of something like the attached instead.

Looks fine to me, maybe add a short comment?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com>

> Looks fine to me

Any thoughts on whether this should be back-patched to 9.3?  I can
see arguments both ways, and don't have a particularly strong
feeling one way or the other.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-11-05 12:21:23 -0800, Kevin Grittner wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com>
>
> > Looks fine to me
>
> Any thoughts on whether this should be back-patched to 9.3?  I can
> see arguments both ways, and don't have a particularly strong
> feeling one way or the other.

Hehe. I was wondering myself. I'd tentatively say no - unless we also
backpatch the debugging patch there doesn't seem to be good reason to
since the likelihood of conficts due to it doesn't seem very high.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-11-05 12:21:23 -0800, Kevin Grittner wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com>
>>
>>> Looks fine to me
>>
>> Any thoughts on whether this should be back-patched to 9.3?  I
>> can see arguments both ways, and don't have a particularly
>> strong feeling one way or the other.
>
> Hehe. I was wondering myself. I'd tentatively say no - unless we
> also backpatch the debugging patch there doesn't seem to be good
> reason to since the likelihood of conficts due to it doesn't seem
> very high.

Works for me.  Done.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-10-24 20:58:55 +0100, Dean Rasheed wrote:
> On 24 October 2013 18:28, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-10-23 21:20:58 +0100, Dean Rasheed wrote:
> >> On 23 October 2013 21:08, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> > On 2013-10-23 20:51:27 +0100, Dean Rasheed wrote:
> >> >> Hmm, my first thought is that rewriteTargetView() should be calling
> >> >> AcquireRewriteLocks() on viewquery, before doing too much with it.
> >> >> There may be sub-queries in viewquery's quals (and also now in its
> >> >> targetlist) and I don't think the relations referred to by those
> >> >> sub-queries are getting locked.
> >> >
> >> > Well, that wouldn't follow the currently documented rule ontop
> >> > of QueryRewrite:
> >> > * NOTE: the parsetree must either have come straight from the parser,
> >> > * or have been scanned by AcquireRewriteLocks to acquire suitable locks.
> >> >
> >> > It might still be the right thing to do, but it seems suspicious that
> >> > the rules need to be tweaked like that.
> >> >
> >>
> >> Well it matches what already happens in other places in the rewriter
> >> --- see rewriteRuleAction() and ApplyRetrieveRule(). It's precisely
> >> because the rule action's query hasn't come from the parser that it
> >> needs to be processed in this way.
> >
> > I really don't know that are of code that well, fortunately I never had
> > to wade around it much so far...
> >
> > Reading your explanation and a bit of the code your plan sound sane. Are
> > you going to propose a patch?
> >
>
> I thought about making rewriteTargetView() call AcquireRewriteLocks(),
> but on closer inspection I think that is probably over the top. 90% of
> the code in AcquireRewriteLocks() is to process the query's RTEs, but
> rewriteTargetView() is already locking the single RTE in viewquery
> anyway. Also AcquireRewriteLocks() would acquire the wrong kind of
> lock on that RTE, since viewquery is a SELECT, but we want an
> exclusive lock because the RTE is about to become the outer query's
> result relation. AcquireRewriteLocks() also processes CTEs, but we
> know that we won't have any of those in rewriteTargetView(). So I
> think the only thing missing from rewriteTargetView() is to lock any
> relations from any sublink subqueries in viewquery using
> acquireLocksOnSubLinks() -- patch attached.

This is still an open problem :(

> diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
> new file mode 100644
> index c52a374..e6a9e7b
> *** a/src/backend/rewrite/rewriteHandler.c
> --- b/src/backend/rewrite/rewriteHandler.c
> *************** rewriteTargetView(Query *parsetree, Rela
> *** 2589,2594 ****
> --- 2589,2604 ----
> heap_close(base_rel, NoLock);
>
> /*
> + * If the view query contains any sublink subqueries, we should also
> + * acquire locks on any relations they refer to. We know that there won't
> + * be any subqueries in the range table or CTEs, so we can skip those, as
> + * in AcquireRewriteLocks.
> + */
> + if (viewquery->hasSubLinks)
> + query_tree_walker(viewquery, acquireLocksOnSubLinks, NULL,
> + QTW_IGNORE_RC_SUBQUERIES);
> +
> + /*
> * Create a new target RTE describing the base relation, and add it to the
> * outer query's rangetable. (What's happening in the next few steps is
> * very much like what the planner would do to "pull up" the view into the

These days this seems to require a context parameter being passed
down. Other than that this patch still fixes the problem.

Greetings,

Andres Freund

On 27 August 2015 at 12:18, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2013-10-24 20:58:55 +0100, Dean Rasheed wrote:
>> So I
>> think the only thing missing from rewriteTargetView() is to lock any
>> relations from any sublink subqueries in viewquery using
>> acquireLocksOnSubLinks() -- patch attached.
>
> This is still an open problem :(
>

Oh yes, I forgot to follow up on this.

>> diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
>> new file mode 100644
>> index c52a374..e6a9e7b
>> *** a/src/backend/rewrite/rewriteHandler.c
>> --- b/src/backend/rewrite/rewriteHandler.c
>> *************** rewriteTargetView(Query *parsetree, Rela
>> *** 2589,2594 ****
>> --- 2589,2604 ----
>> heap_close(base_rel, NoLock);
>>
>> /*
>> + * If the view query contains any sublink subqueries, we should also
>> + * acquire locks on any relations they refer to. We know that there won't
>> + * be any subqueries in the range table or CTEs, so we can skip those, as
>> + * in AcquireRewriteLocks.
>> + */
>> + if (viewquery->hasSubLinks)
>> + query_tree_walker(viewquery, acquireLocksOnSubLinks, NULL,
>> + QTW_IGNORE_RC_SUBQUERIES);
>> +
>> + /*
>> * Create a new target RTE describing the base relation, and add it to the
>> * outer query's rangetable. (What's happening in the next few steps is
>> * very much like what the planner would do to "pull up" the view into the
>
> These days this seems to require a context parameter being passed
> down. Other than that this patch still fixes the problem.
>

Yes, I concur. It now needs an acquireLocksOnSubLinks_context with
for_execute = true, but otherwise it should work.

I have a feeling that RLS might suffer from the same issue, but I
haven't looked yet.

Regards,
Dean

On 2015-08-27 18:37:10 +0100, Dean Rasheed wrote:
> On 27 August 2015 at 12:18, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >>
> >> /*
> >> + * If the view query contains any sublink subqueries, we should also
> >> + * acquire locks on any relations they refer to. We know that there won't
> >> + * be any subqueries in the range table or CTEs, so we can skip those, as
> >> + * in AcquireRewriteLocks.
> >> + */
> >> + if (viewquery->hasSubLinks)
> >> + query_tree_walker(viewquery, acquireLocksOnSubLinks, NULL,
> >> + QTW_IGNORE_RC_SUBQUERIES);
> >> +
> >> + /*
> >> * Create a new target RTE describing the base relation, and add it to the
> >> * outer query's rangetable. (What's happening in the next few steps is
> >> * very much like what the planner would do to "pull up" the view into the
> >
> > These days this seems to require a context parameter being passed
> > down. Other than that this patch still fixes the problem.
> >
>
> Yes, I concur. It now needs an acquireLocksOnSubLinks_context with
> for_execute = true, but otherwise it should work.

Ok, I'll push that then, unless somebody else wants to do the honors.

> I have a feeling that RLS might suffer from the same issue, but I
> haven't looked yet.

There's a similar issue there, yes:
http://archives.postgresql.org/message-id/20150827124931.GD15922%40awork2.anarazel.de

Are you thinking of that angle, or yet another one?

Regards,

Andres

On 27 August 2015 at 18:44, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2015-08-27 18:37:10 +0100, Dean Rasheed wrote:
>> I have a feeling that RLS might suffer from the same issue, but I
>> haven't looked yet.
>
> There's a similar issue there, yes:
> http://archives.postgresql.org/message-id/20150827124931.GD15922%40awork2.anarazel.de
>
> Are you thinking of that angle, or yet another one?
>

Yeah, that's what I was thinking of - I just hadn't caught up on all my mail.

It also seems to me that this warning has proved its worth, although I
don't think it's something a production build should be producing.
Perhaps it could be an Assert?

Regards,
Dean

On 2015-08-27 19:19:35 +0100, Dean Rasheed wrote:
> It also seems to me that this warning has proved its worth

Same here - I plan to re-submit it. Perhaps the number of bugs it found
convinces Tom, after I address some of his points.

> although I don't think it's something a production build should be
> producing. Perhaps it could be an Assert?

It's currently protected by a #ifdef USE_ASSERT_CHECKING. A warning
seems to make it easier to actually run the whole regression test, and
it's consistent with what we do in a bunch of other places.

Greetings,

Andres Freund

On 27 August 2015 at 19:29, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2015-08-27 19:19:35 +0100, Dean Rasheed wrote:
>> It also seems to me that this warning has proved its worth
>
> Same here - I plan to re-submit it. Perhaps the number of bugs it found
> convinces Tom, after I address some of his points.
>
>> although I don't think it's something a production build should be
>> producing. Perhaps it could be an Assert?
>
> It's currently protected by a #ifdef USE_ASSERT_CHECKING. A warning
> seems to make it easier to actually run the whole regression test, and
> it's consistent with what we do in a bunch of other places.
>

OK, that seems reasonable.

Regards,
Dean

* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2015-08-27 18:37:10 +0100, Dean Rasheed wrote:
> > Yes, I concur. It now needs an acquireLocksOnSubLinks_context with
> > for_execute = true, but otherwise it should work.
>
> Ok, I'll push that then, unless somebody else wants to do the honors.

Done. Apologies about it taking so long, the TAP test failure issue
with 'make check' had me tied up for a bit.

Thanks!

Stephen