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

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.
Doing it in get_qual_from_partbound(), as is now, would produce wrong
result in certain multi-level partitioning cases, because it only
considers the current pair of parent-child relations. In certain
multi-level partitioning cases, attnums for the same key attribute(s)
might differ between various levels causing the same attribute to be
numbered differently in different instances of the Var corresponding
to a given attribute.

With this commit, in generate_partition_qual(), we first generate the
the whole partition constraint (considering all levels of partitioning)
and then do the mapping, so that Vars in the final expression are
numbered according the leaf relation (to which it is supposed to apply).

Amit Langote, reviewed by me.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/0563a3a8b59150bf3cc8b2b7077f684e0eaf8aff

Modified Files
--------------
src/backend/catalog/partition.c | 99 +++++++++++++++----------------
src/backend/commands/tablecmds.c | 7 ++-
src/include/catalog/partition.h | 1 +
src/test/regress/expected/alter_table.out | 30 ++++++++++
src/test/regress/sql/alter_table.sql | 25 ++++++++
5 files changed, 110 insertions(+), 52 deletions(-)

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
/message-id/20170109182800.qrkae62kmur3gfeg@alvherre.pgsql
and commit 3957b58b8885441c8d03bc1cfc00e47cf8cd7975. Part of this code
duplicates that ...

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

On Fri, Jan 13, 2017 at 2:18 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)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
> /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(at)gmail(dot)com> wrote:
> On Fri, Jan 13, 2017 at 2:18 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)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
>> /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(at)gmail(dot)com> wrote:
> > On Fri, Jan 13, 2017 at 2:18 PM, Alvaro Herrera
> > <alvherre(at)2ndquadrant(dot)com> wrote:

> >> Hmm, we were discussing this stuff a few days ago, see
> >> /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(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> On Fri, Jan 13, 2017 at 2:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > On Fri, Jan 13, 2017 at 2:18 PM, Alvaro Herrera
>> > <alvherre(at)2ndquadrant(dot)com> wrote:
>
>> >> Hmm, we were discussing this stuff a few days ago, see
>> >> /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(at)gmail(dot)com> wrote:
> On Fri, Jan 13, 2017 at 3:09 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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(at)gmail(dot)com> wrote:
>> On Fri, Jan 13, 2017 at 3:09 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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