Horton

Thread

Commits

  1. Teach PL/pgSQL about partitioned tables.

  2. Teach RemoveRoleFromObjectPolicy() about partitioned tables.

  3. Teach relation_is_updatable() about partitioned tables.

  1. PG10 Partitioned tables and relation_is_updatable()

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-06-11T11:32:48Z

    Hi,

It looks like relation_is_updatable() didn't get the message about
partitioned tables. Thus, for example, information_schema.views and
information_schema.columns report that simple views built on top of
partitioned tables are non-updatable, which is wrong. Attached is a
patch to fix this.

I think this kind of omission is an easy mistake to make when adding a
new relkind, so it might be worth having more pairs of eyes looking
out for more of the same. I did a quick scan of the rewriter code
(prompted by the recent similar omission for RLS on partitioned
tables) and I didn't find any more problems there, but I haven't
looked elsewhere yet.

Regards,
Dean

  2. Re: PG10 Partitioned tables and relation_is_updatable()

    Joe Conway <mail@joeconway.com> — 2017-06-11T15:55:21Z

    On 06/11/2017 04:32 AM, Dean Rasheed wrote:
> It looks like relation_is_updatable() didn't get the message about
> partitioned tables. Thus, for example, information_schema.views and
> information_schema.columns report that simple views built on top of
> partitioned tables are non-updatable, which is wrong. Attached is a
> patch to fix this.

> I think this kind of omission is an easy mistake to make when adding a
> new relkind, so it might be worth having more pairs of eyes looking
> out for more of the same. I did a quick scan of the rewriter code
> (prompted by the recent similar omission for RLS on partitioned
> tables) and I didn't find any more problems there, but I haven't
> looked elsewhere yet.

Yeah, I noticed the same while working on the RLS related patch. I did
not see anything else in rewriteHandler.c but it is probably worth
looking wider for other omissions.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  3. Re: PG10 Partitioned tables and relation_is_updatable()

    Joe Conway <mail@joeconway.com> — 2017-06-11T15:59:46Z

    On 06/11/2017 08:55 AM, Joe Conway wrote:
> On 06/11/2017 04:32 AM, Dean Rasheed wrote:
>> It looks like relation_is_updatable() didn't get the message about
>> partitioned tables. Thus, for example, information_schema.views and
>> information_schema.columns report that simple views built on top of
>> partitioned tables are non-updatable, which is wrong. Attached is a
>> patch to fix this.
> 
>> I think this kind of omission is an easy mistake to make when adding a
>> new relkind, so it might be worth having more pairs of eyes looking
>> out for more of the same. I did a quick scan of the rewriter code
>> (prompted by the recent similar omission for RLS on partitioned
>> tables) and I didn't find any more problems there, but I haven't
>> looked elsewhere yet.
> 
> Yeah, I noticed the same while working on the RLS related patch. I did
> not see anything else in rewriteHandler.c but it is probably worth
> looking wider for other omissions.

So in particular:

src/include/utils/rel.h:318:
8<----------------------------
/*
 * RelationIsUsedAsCatalogTable
 *    Returns whether the relation should be treated as a catalog table
 *    from the pov of logical decoding.  Note multiple eval of argument!
 */
#define RelationIsUsedAsCatalogTable(relation)  \
((relation)->rd_options && \
 ((relation)->rd_rel->relkind == RELKIND_RELATION || \
  (relation)->rd_rel->relkind == RELKIND_MATVIEW) ? \
 ((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false)
8<----------------------------

Does RELKIND_PARTITIONED_TABLE apply there?

Will continue to poke around...

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  4. Re: PG10 Partitioned tables and relation_is_updatable()

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-06-11T18:53:57Z

    On 11 June 2017 at 16:59, Joe Conway <mail@joeconway.com> wrote:
> On 06/11/2017 08:55 AM, Joe Conway wrote:
>> Yeah, I noticed the same while working on the RLS related patch. I did
>> not see anything else in rewriteHandler.c but it is probably worth
>> looking wider for other omissions.
>
> So in particular:
>
> #define RelationIsUsedAsCatalogTable(relation)  \
> ((relation)->rd_options && \
>  ((relation)->rd_rel->relkind == RELKIND_RELATION || \
>   (relation)->rd_rel->relkind == RELKIND_MATVIEW) ? \
>  ((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false)
> 8<----------------------------
>
> Does RELKIND_PARTITIONED_TABLE apply there?
>

Hmm, a quick experiment shows that it won't allow a partitioned table
to be used as a user catalog table, although I'm not sure if that's
intentional. If it is, it doesn't appear to be documented.

I found another RLS-related omission -- RemoveRoleFromObjectPolicy()
doesn't work for partitioned tables, so DROP OWNED BY <role> will fail
if there are any partitioned tables with RLS.

I also found another couple of omissions in PL/pgSQL --
plpgsql_parse_cwordtype() and build_row_from_class(), so for example
%rowtype doesn't work for partitioned tables.

That's it for my quick once-over the code-base, but I'm not confident
that will have caught everything.

Regards,
Dean

  5. Re: PG10 Partitioned tables and relation_is_updatable()

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-12T08:29:44Z

    On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Hi,
>
> It looks like relation_is_updatable() didn't get the message about
> partitioned tables. Thus, for example, information_schema.views and
> information_schema.columns report that simple views built on top of
> partitioned tables are non-updatable, which is wrong. Attached is a
> patch to fix this.
>
> I think this kind of omission is an easy mistake to make when adding a
> new relkind, so it might be worth having more pairs of eyes looking
> out for more of the same. I did a quick scan of the rewriter code
> (prompted by the recent similar omission for RLS on partitioned
> tables) and I didn't find any more problems there, but I haven't
> looked elsewhere yet.
>

Changes look good to me. In order to avoid such instances in future, I
have proposed to bundle the conditions as macros in [1].

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  6. Re: PG10 Partitioned tables and relation_is_updatable()

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-06-12T08:49:58Z

    > On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> Hi,
>>
>> It looks like relation_is_updatable() didn't get the message about
>> partitioned tables. Thus, for example, information_schema.views and
>> information_schema.columns report that simple views built on top of
>> partitioned tables are non-updatable, which is wrong. Attached is a
>> patch to fix this.

Thanks for the patch, Dean.

>> I think this kind of omission is an easy mistake to make when adding a
>> new relkind, so it might be worth having more pairs of eyes looking
>> out for more of the same. I did a quick scan of the rewriter code
>> (prompted by the recent similar omission for RLS on partitioned
>> tables) and I didn't find any more problems there, but I haven't
>> looked elsewhere yet.

As he mentioned in his reply, Ashutosh's proposal to abstract away the
relkind checks is interesting in this regard.

On 2017/06/12 17:29, Ashutosh Bapat wrote:
> Changes look good to me. In order to avoid such instances in future, I
> have proposed to bundle the conditions as macros in [1].

It seems that Ashutosh forgot to include the link:

https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=AvCYh55Q@mail.gmail.com

Thanks,
Amit

  7. Re: PG10 Partitioned tables and relation_is_updatable()

    Joe Conway <mail@joeconway.com> — 2017-06-12T14:40:09Z

    On 06/12/2017 01:49 AM, Amit Langote wrote:
>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>> It looks like relation_is_updatable() didn't get the message about
>>> partitioned tables. Thus, for example, information_schema.views and
>>> information_schema.columns report that simple views built on top of
>>> partitioned tables are non-updatable, which is wrong. Attached is a
>>> patch to fix this.
> 
> Thanks for the patch, Dean.
> 
>>> I think this kind of omission is an easy mistake to make when adding a
>>> new relkind, so it might be worth having more pairs of eyes looking
>>> out for more of the same. I did a quick scan of the rewriter code
>>> (prompted by the recent similar omission for RLS on partitioned
>>> tables) and I didn't find any more problems there, but I haven't
>>> looked elsewhere yet.
> 
> As he mentioned in his reply, Ashutosh's proposal to abstract away the
> relkind checks is interesting in this regard.
> 
> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>> Changes look good to me. In order to avoid such instances in future, I
>> have proposed to bundle the conditions as macros in [1].
> 
> It seems that Ashutosh forgot to include the link:
> 
> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=AvCYh55Q@mail.gmail.com

I have not looked at Ashutosh's patch yet, but it sounds like a good
idea to me.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  8. Re: PG10 Partitioned tables and relation_is_updatable()

    Joe Conway <mail@joeconway.com> — 2017-06-12T16:51:49Z

    On 06/12/2017 07:40 AM, Joe Conway wrote:
> On 06/12/2017 01:49 AM, Amit Langote wrote:
>>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>>> It looks like relation_is_updatable() didn't get the message about
>>>> partitioned tables. Thus, for example, information_schema.views and
>>>> information_schema.columns report that simple views built on top of
>>>> partitioned tables are non-updatable, which is wrong. Attached is a
>>>> patch to fix this.
>> 
>> Thanks for the patch, Dean.
>> 
>>>> I think this kind of omission is an easy mistake to make when adding a
>>>> new relkind, so it might be worth having more pairs of eyes looking
>>>> out for more of the same. I did a quick scan of the rewriter code
>>>> (prompted by the recent similar omission for RLS on partitioned
>>>> tables) and I didn't find any more problems there, but I haven't
>>>> looked elsewhere yet.
>> 
>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>> relkind checks is interesting in this regard.
>> 
>> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>>> Changes look good to me. In order to avoid such instances in future, I
>>> have proposed to bundle the conditions as macros in [1].
>> 
>> It seems that Ashutosh forgot to include the link:
>> 
>> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=AvCYh55Q@mail.gmail.com
> 
> I have not looked at Ashutosh's patch yet, but it sounds like a good
> idea to me.

After looking I remain convinced - +1 in general.

One thing I would change -- in many cases there are ERROR messages
enumerating the relkinds. E.g.:
8<-----------------
if (!RELKIND_CAN_HAVE_COLUMN_COMMENT(relation->rd_rel->relkind))
    ereport(ERROR,
            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
            errmsg("\"%s\" is not a table, view, materialized view,
                    composite type, or foreign table",
8<-----------------

I think those messages should also be rewritten to make them less
relkind specific and more clear, otherwise we still risk getting out of
sync in the future. Maybe something like this:
8<-----------------
   "\"%s\" is not a kind of relation that can have column comments"
8<-----------------
Thoughts?

In any case, unless another committer else wants it, and assuming no
complaints with the concept, I'd be happy to take this.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  9. Re: PG10 Partitioned tables and relation_is_updatable()

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-06-12T18:33:49Z

    On 12 June 2017 at 17:51, Joe Conway <mail@joeconway.com> wrote:
> On 06/12/2017 07:40 AM, Joe Conway wrote:
>> On 06/12/2017 01:49 AM, Amit Langote wrote:
>>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>>> relkind checks is interesting in this regard.
>>>
>> I have not looked at Ashutosh's patch yet, but it sounds like a good
>> idea to me.
>
> After looking I remain convinced - +1 in general.
>

Yes, I think this will probably help, but I worry that it will turn
into quite a large and invasive patch, and there are a number of
design choices to be made over the naming and precise set of macros.
Is this really PG10 material?

My initial thought, looking at the patch, is that it might be better
to have all the macros in one file to make them easier to maintain.


> One thing I would change -- in many cases there are ERROR messages
> enumerating the relkinds. E.g.:
> 8<-----------------
> if (!RELKIND_CAN_HAVE_COLUMN_COMMENT(relation->rd_rel->relkind))
>     ereport(ERROR,
>             (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>             errmsg("\"%s\" is not a table, view, materialized view,
>                     composite type, or foreign table",
> 8<-----------------
>
> I think those messages should also be rewritten to make them less
> relkind specific and more clear, otherwise we still risk getting out of
> sync in the future. Maybe something like this:
> 8<-----------------
>    "\"%s\" is not a kind of relation that can have column comments"
> 8<-----------------
> Thoughts?
>

-1. I think the existing error messages provide useful information
that you'd be removing. If you're worried about the error messages
getting out of sync, then wouldn't it be better to centralise them
along with the corresponding macros?


> In any case, unless another committer else wants it, and assuming no
> complaints with the concept, I'd be happy to take this.
>

OK, have at it.

Barring objections, I'll push my original patch and work up patches
for the other couple of issues I found.

Regards,
Dean

  10. Re: PG10 Partitioned tables and relation_is_updatable()

    Joe Conway <mail@joeconway.com> — 2017-06-12T18:45:44Z

    On 06/12/2017 11:33 AM, Dean Rasheed wrote:
> On 12 June 2017 at 17:51, Joe Conway <mail@joeconway.com> wrote:
>> After looking I remain convinced - +1 in general.
> 
> Yes, I think this will probably help, but I worry that it will turn
> into quite a large and invasive patch, and there are a number of
> design choices to be made over the naming and precise set of macros.
> Is this really PG10 material?


I was wondering the same after responding. Possibly not.


> My initial thought, looking at the patch, is that it might be better
> to have all the macros in one file to make them easier to maintain.


Yeah, that was my thought as well.


>> sync in the future. Maybe something like this:
>> 8<-----------------
>>    "\"%s\" is not a kind of relation that can have column comments"
>> 8<-----------------
>> Thoughts?
> 
> -1. I think the existing error messages provide useful information
> that you'd be removing. If you're worried about the error messages
> getting out of sync, then wouldn't it be better to centralise them
> along with the corresponding macros?


I guess that could work too.


> Barring objections, I'll push my original patch and work up patches
> for the other couple of issues I found.


No objections here -- we definitely need to fix those.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  11. Re: PG10 Partitioned tables and relation_is_updatable()

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-13T04:39:21Z

    On Mon, Jun 12, 2017 at 2:19 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>> Hi,
>>>
>>> It looks like relation_is_updatable() didn't get the message about
>>> partitioned tables. Thus, for example, information_schema.views and
>>> information_schema.columns report that simple views built on top of
>>> partitioned tables are non-updatable, which is wrong. Attached is a
>>> patch to fix this.
>
> Thanks for the patch, Dean.
>
>>> I think this kind of omission is an easy mistake to make when adding a
>>> new relkind, so it might be worth having more pairs of eyes looking
>>> out for more of the same. I did a quick scan of the rewriter code
>>> (prompted by the recent similar omission for RLS on partitioned
>>> tables) and I didn't find any more problems there, but I haven't
>>> looked elsewhere yet.
>
> As he mentioned in his reply, Ashutosh's proposal to abstract away the
> relkind checks is interesting in this regard.
>
> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>> Changes look good to me. In order to avoid such instances in future, I
>> have proposed to bundle the conditions as macros in [1].
>
> It seems that Ashutosh forgot to include the link:
>
> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=AvCYh55Q@mail.gmail.com

Sorry and thanks for providing the link.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  12. Re: PG10 Partitioned tables and relation_is_updatable()

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-13T04:46:59Z

    On Mon, Jun 12, 2017 at 10:21 PM, Joe Conway <mail@joeconway.com> wrote:
> On 06/12/2017 07:40 AM, Joe Conway wrote:
>> On 06/12/2017 01:49 AM, Amit Langote wrote:
>>>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>>>> It looks like relation_is_updatable() didn't get the message about
>>>>> partitioned tables. Thus, for example, information_schema.views and
>>>>> information_schema.columns report that simple views built on top of
>>>>> partitioned tables are non-updatable, which is wrong. Attached is a
>>>>> patch to fix this.
>>>
>>> Thanks for the patch, Dean.
>>>
>>>>> I think this kind of omission is an easy mistake to make when adding a
>>>>> new relkind, so it might be worth having more pairs of eyes looking
>>>>> out for more of the same. I did a quick scan of the rewriter code
>>>>> (prompted by the recent similar omission for RLS on partitioned
>>>>> tables) and I didn't find any more problems there, but I haven't
>>>>> looked elsewhere yet.
>>>
>>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>>> relkind checks is interesting in this regard.
>>>
>>> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>>>> Changes look good to me. In order to avoid such instances in future, I
>>>> have proposed to bundle the conditions as macros in [1].
>>>
>>> It seems that Ashutosh forgot to include the link:
>>>
>>> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=AvCYh55Q@mail.gmail.com
>>
>> I have not looked at Ashutosh's patch yet, but it sounds like a good
>> idea to me.
>
> After looking I remain convinced - +1 in general.
>
> One thing I would change -- in many cases there are ERROR messages
> enumerating the relkinds. E.g.:
> 8<-----------------
> if (!RELKIND_CAN_HAVE_COLUMN_COMMENT(relation->rd_rel->relkind))
>     ereport(ERROR,
>             (errcode(ERRCODE_WRONG_OBJECT_TYPE),
>             errmsg("\"%s\" is not a table, view, materialized view,
>                     composite type, or foreign table",
> 8<-----------------
>
> I think those messages should also be rewritten to make them less
> relkind specific and more clear, otherwise we still risk getting out of
> sync in the future. Maybe something like this:
> 8<-----------------
>    "\"%s\" is not a kind of relation that can have column comments"
> 8<-----------------
> Thoughts?

+1 for bringing this list to a common place. I thought about rewording
the message, but it looks like a user would be interested in the list
of relkinds rather than their connecting property. Dean also seems to
be of the same opinion.

>
> In any case, unless another committer else wants it, and assuming no
> complaints with the concept, I'd be happy to take this.
>

Thanks. I will update the patches with the error message changes and
also add some more macros by first commitfest.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  13. Re: PG10 Partitioned tables and relation_is_updatable()

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-06-13T04:50:23Z

    On Tue, Jun 13, 2017 at 12:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On 12 June 2017 at 17:51, Joe Conway <mail@joeconway.com> wrote:
>> On 06/12/2017 07:40 AM, Joe Conway wrote:
>>> On 06/12/2017 01:49 AM, Amit Langote wrote:
>>>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>>>> relkind checks is interesting in this regard.
>>>>
>>> I have not looked at Ashutosh's patch yet, but it sounds like a good
>>> idea to me.
>>
>> After looking I remain convinced - +1 in general.
>>
>
> Yes, I think this will probably help, but I worry that it will turn
> into quite a large and invasive patch, and there are a number of
> design choices to be made over the naming and precise set of macros.
> Is this really PG10 material?

No this is not for PG10.

>
> My initial thought, looking at the patch, is that it might be better
> to have all the macros in one file to make them easier to maintain.
>

Right now the macros are listed just below relkind enum in pg_class.h.
Is that a good place or do you think, we should list them in a
separate file?

>
> Barring objections, I'll push my original patch and work up patches
> for the other couple of issues I found.

No objections, the patch is good to go as is. Sorry for high-jacking
this thread.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  14. Re: PG10 Partitioned tables and relation_is_updatable()

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-06-13T06:57:30Z

    On 13 June 2017 at 05:50, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Tue, Jun 13, 2017 at 12:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> My initial thought, looking at the patch, is that it might be better
>> to have all the macros in one file to make them easier to maintain.
>
> Right now the macros are listed just below relkind enum in pg_class.h.
> Is that a good place or do you think, we should list them in a
> separate file?
>

Yeah, I wondered about putting them in a separate file, but I think
just below the relkind enum is probably the best place, so that people
changing that enum immediately see the first set of related things to
be updated.

>> Barring objections, I'll push my original patch and work up patches
>> for the other couple of issues I found.
>
> No objections, the patch is good to go as is. Sorry for high-jacking
> this thread.
>

No worries. I missed that other thread initially, so it was useful to
link the 2 threads together.

Regards,
Dean

  15. Re: PG10 Partitioned tables and relation_is_updatable()

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-06-13T17:29:39Z

    On 13 June 2017 at 05:50, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Tue, Jun 13, 2017 at 12:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> Barring objections, I'll push my original patch and work up patches
>> for the other couple of issues I found.
>
> No objections, the patch is good to go as is. Sorry for high-jacking
> this thread.
>

Thanks for the review. Patch pushed.

Here are patches for the other 2 issues, with test cases showing how
they currently fail on HEAD.

Regards,
Dean

  16. Re: PG10 Partitioned tables and relation_is_updatable()

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-06-14T01:37:21Z

    Hi Dean,

On 2017/06/14 2:29, Dean Rasheed wrote:
> On 13 June 2017 at 05:50, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> On Tue, Jun 13, 2017 at 12:03 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>> Barring objections, I'll push my original patch and work up patches
>>> for the other couple of issues I found.
>>
>> No objections, the patch is good to go as is. Sorry for high-jacking
>> this thread.
>>
> 
> Thanks for the review. Patch pushed.
> 
> Here are patches for the other 2 issues, with test cases showing how
> they currently fail on HEAD.

Thanks again.  Both patches look good to me.

Regards,
Amit