Horton

Thread

Commits

  1. Fix uninitialized memory reference.

  2. Centralize executor-related partitioning code.

  1. Use of uninitialized variables in ExecFindPartition() for parent partition without leaves (HEAD only)

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-30T00:07:06Z

    Hi all,

Since commit 4e5fe9ad (committer Robert Haas and author Amit Langote),
coverity has been complaining that the new code of ExecFindPartition()
may use a set of values and isnull values which never get initialized.
This is a state which can be easily reached with the following SQLs of
a parent partition with no children:
create table parent_tab (a int) partition by list ((a+0));
insert into parent_tab values (1);

The mistake is visibly that FormPartitionKeyDatum() should be called
even for a root partition, initializing the fields of isnull and
values on the way. That's actually what happens in execMain.c's
ExecFindPartition() if you look at REL_10_STABLE. So the answer would
be to move a bit down the quick exit code. Attached is a patch.

Amit, that's your code. What do you think?

Thanks,
-- 
Michael

  2. Re: Use of uninitialized variables in ExecFindPartition() for parent partition without leaves (HEAD only)

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-11-30T01:11:09Z

    Hi Michael.

On 2017/11/30 9:07, Michael Paquier wrote:
> Hi all,
> 
> Since commit 4e5fe9ad (committer Robert Haas and author Amit Langote),
> coverity has been complaining that the new code of ExecFindPartition()
> may use a set of values and isnull values which never get initialized.
> This is a state which can be easily reached with the following SQLs of
> a parent partition with no children:
> create table parent_tab (a int) partition by list ((a+0));
> insert into parent_tab values (1);
> 
> The mistake is visibly that FormPartitionKeyDatum() should be called
> even for a root partition, initializing the fields of isnull and
> values on the way. That's actually what happens in execMain.c's
> ExecFindPartition() if you look at REL_10_STABLE. So the answer would
> be to move a bit down the quick exit code. Attached is a patch.
> 
> Amit, that's your code. What do you think?

Thanks for the report.  That's clearly a bug. :-(

Your patch seems enough to fix it.  I added a test and expanded the
comment a bit in the attached updated version.

Thanks,
Amit

  3. Re: Use of uninitialized variables in ExecFindPartition() for parent partition without leaves (HEAD only)

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-11-30T01:17:55Z

    On 2017/11/30 10:11, Amit Langote wrote:
> in the attached updated version.

Oops, I messed up taking the diff and mistakenly added noise to the patch.
 Fixed in the attached.

Thanks,
Amit

  4. Re: Use of uninitialized variables in ExecFindPartition() for parent partition without leaves (HEAD only)

    Michael Paquier <michael.paquier@gmail.com> — 2017-12-01T02:48:48Z

    On Thu, Nov 30, 2017 at 10:17 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Oops, I messed up taking the diff and mistakenly added noise to the patch.

Which is that bit:
- * BuildSlotPartitionKeyDescription
+ * ExecBuildSlotPartitionKeyDescription

>  Fixed in the attached.

For information, it is easy enough to run into rather nasty behaviors
here. Taking for example the test case in the last patch, but without
the actual fix:
  -- no partitions, so fail
  insert into range_parted values ('a', 11);
! ERROR:  invalid memory alloc request size 2139062147
So the test case you are adding is a good thing to have, and I am fine
with the comment.

I have added a CF entry for this thread by the way
(https://commitfest.postgresql.org/16/1392/), and marked the thing as
ready for committer as we agree about the fix. Let's track properly
this issue until it gets committed.
-- 
Michael

  5. Re: Use of uninitialized variables in ExecFindPartition() for parent partition without leaves (HEAD only)

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-12-01T04:54:56Z

    On 2017/12/01 11:48, Michael Paquier wrote:
> On Thu, Nov 30, 2017 at 10:17 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Oops, I messed up taking the diff and mistakenly added noise to the patch.
> 
> Which is that bit:
> - * BuildSlotPartitionKeyDescription
> + * ExecBuildSlotPartitionKeyDescription

Yep.

>>  Fixed in the attached.
> 
> For information, it is easy enough to run into rather nasty behaviors
> here. Taking for example the test case in the last patch, but without
> the actual fix:
>   -- no partitions, so fail
>   insert into range_parted values ('a', 11);
> ! ERROR:  invalid memory alloc request size 2139062147

Yikes.

> So the test case you are adding is a good thing to have, and I am fine
> with the comment.

Thanks.

> I have added a CF entry for this thread by the way
> (https://commitfest.postgresql.org/16/1392/), and marked the thing as
> ready for committer as we agree about the fix. Let's track properly
> this issue until it gets committed.

Yeah, thanks.

Regards,
Amit

  6. Re: Use of uninitialized variables in ExecFindPartition() for parent partition without leaves (HEAD only)

    Robert Haas <robertmhaas@gmail.com> — 2017-12-01T15:07:16Z

    On Thu, Nov 30, 2017 at 11:54 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> I have added a CF entry for this thread by the way
>> (https://commitfest.postgresql.org/16/1392/), and marked the thing as
>> ready for committer as we agree about the fix. Let's track properly
>> this issue until it gets committed.
>
> Yeah, thanks.

Committed.

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