Thread: Re: [HACKERS] [COMMITTERS] pgsql: Fix a bug in how we generate partition constraints.

On Fri, Jan 13, 2017 at 2:18 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>> Fix a bug in how we generate partition constraints.
>>
>> Move the code for doing parent attnos to child attnos mapping for Vars
>> in partition constraint expressions to a separate function
>> map_partition_varattnos() and call it from the appropriate places.
>
> Hmm, we were discussing this stuff a few days ago, see
> https://www.postgresql.org/message-id/20170109182800.qrkae62kmur3gfeg@alvherre.pgsql
> and commit 3957b58b8885441c8d03bc1cfc00e47cf8cd7975.  Part of this code
> duplicates that ...

Is that bad?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jan 13, 2017 at 2:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jan 13, 2017 at 2:18 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Robert Haas wrote:
>>> Fix a bug in how we generate partition constraints.
>>>
>>> Move the code for doing parent attnos to child attnos mapping for Vars
>>> in partition constraint expressions to a separate function
>>> map_partition_varattnos() and call it from the appropriate places.
>>
>> Hmm, we were discussing this stuff a few days ago, see
>> https://www.postgresql.org/message-id/20170109182800.qrkae62kmur3gfeg@alvherre.pgsql
>> and commit 3957b58b8885441c8d03bc1cfc00e47cf8cd7975.  Part of this code
>> duplicates that ...
>
> Is that bad?

If you are expressing a concern about who wrote this code, I took
Amit's word for it that he did.  His patch file says:

Reported by: n/a
Patch by: Amit Langote
Reports: n/a

If that ain't right, that's bad.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Fri, Jan 13, 2017 at 2:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Fri, Jan 13, 2017 at 2:18 PM, Alvaro Herrera
> > <alvherre@2ndquadrant.com> wrote:

> >> Hmm, we were discussing this stuff a few days ago, see
> >> https://www.postgresql.org/message-id/20170109182800.qrkae62kmur3gfeg@alvherre.pgsql
> >> and commit 3957b58b8885441c8d03bc1cfc00e47cf8cd7975.  Part of this code
> >> duplicates that ...
> >
> > Is that bad?
> 
> If you are expressing a concern about who wrote this code, I took
> Amit's word for it that he did.

I'm just saying that the problem at hand is already solved for a related
feature, so ISTM this new code should use the recently added routine
rather than doing the same thing in a different way.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jan 13, 2017 at 3:09 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>> On Fri, Jan 13, 2017 at 2:31 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> > On Fri, Jan 13, 2017 at 2:18 PM, Alvaro Herrera
>> > <alvherre@2ndquadrant.com> wrote:
>
>> >> Hmm, we were discussing this stuff a few days ago, see
>> >> https://www.postgresql.org/message-id/20170109182800.qrkae62kmur3gfeg@alvherre.pgsql
>> >> and commit 3957b58b8885441c8d03bc1cfc00e47cf8cd7975.  Part of this code
>> >> duplicates that ...
>> >
>> > Is that bad?
>>
>> If you are expressing a concern about who wrote this code, I took
>> Amit's word for it that he did.
>
> I'm just saying that the problem at hand is already solved for a related
> feature, so ISTM this new code should use the recently added routine
> rather than doing the same thing in a different way.

Oh, I see.  Amit, thoughts?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Jan 14, 2017 at 6:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jan 13, 2017 at 3:09 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>>
>> I'm just saying that the problem at hand is already solved for a related
>> feature, so ISTM this new code should use the recently added routine
>> rather than doing the same thing in a different way.
>
> Oh, I see.  Amit, thoughts?

Hm, perhaps.  The code in map_partition_varattnos() that creates the
map could be replaced by a call to the new
convert_tuples_by_name_map().  In fact, it could even have used the
old version of it (convert_tuples_by_name()).  I guess I just aped
what other callers of map_variable_attnos() were doing, which is to
generate the map themselves (not that they ought to be changed to use
convert_tuples_by_name_map).

I will send a patch at my earliest convenience. Thanks to Alvaro for
pointing that out.

Thanks,
Amit

On 2017/01/14 13:36, Amit Langote wrote:
> On Sat, Jan 14, 2017 at 6:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Jan 13, 2017 at 3:09 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>>>
>>> I'm just saying that the problem at hand is already solved for a related
>>> feature, so ISTM this new code should use the recently added routine
>>> rather than doing the same thing in a different way.
>>
>> Oh, I see.  Amit, thoughts?
> 
> Hm, perhaps.  The code in map_partition_varattnos() that creates the
> map could be replaced by a call to the new
> convert_tuples_by_name_map().  In fact, it could even have used the
> old version of it (convert_tuples_by_name()).  I guess I just aped
> what other callers of map_variable_attnos() were doing, which is to
> generate the map themselves (not that they ought to be changed to use
> convert_tuples_by_name_map).
> 
> I will send a patch at my earliest convenience. Thanks to Alvaro for
> pointing that out.

And here is the patch.

Thanks,
Amit

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers