Обсуждение: BUG #16242: convert_tuple_* not handling missing values correctly

The following bug has been logged on the website:

Bug reference:      16242
Logged by:          Andrew Gierth
Email address:      andrew@tao11.riddles.org.uk
PostgreSQL version: 12.1
Operating system:   any
Description:

The following example shows a tuple appearing with NULLs in a column
declared NOT NULL, because there is a missing default value not being filled
in. The cause is that convert_tuples_by_name, or more precisely
check_attrmap_match, thinks that it's enough for the tupdescs to match up as
regards attnums/names/types, without considering that the source tupdesc
might have missing values that the destination does not. So it incorrectly
concludes that no conversion is needed, and the missing values become
null.

This test case uses a trigger on a partitioned table to demonstrate, but the
actual problem was discovered in a different context (Vik Fearing's
"periods" extension). It is likely that other users of
convert_tuples_by_name (and I know there are other non-core modules that use
this) are affected likewise.

-- partitioned table (parent)
create table bar1 (a integer, b integer not null default 1)
  partition by range (a);

-- this will become a partition:
create table bar2 (a integer);
insert into bar2 values (1);
alter table bar2 add column b integer not null default 1;

-- (at this point bar2 contains tuple with natts=1)

alter table bar1 attach partition bar2 default;

-- this works:

select * from bar1;

-- this doesn't:

create function xtrig()
  returns trigger
  language plpgsql
as $$
  declare
    r record;
  begin
    for r in select * from old loop
      raise info 'a=%, b=%', r.a, r.b;
    end loop;
    return NULL;
  end;
$$;

create trigger xtrig
  after update on bar1
  referencing old table as old
  for each statement execute procedure xtrig();

update bar1 set a = a + 1;

On 04/02/2020 03:24, PG Bug reporting form wrote:
> The following example shows a tuple appearing with NULLs in a column
> declared NOT NULL, because there is a missing default value not being filled
> in. The cause is that convert_tuples_by_name, or more precisely
> check_attrmap_match, thinks that it's enough for the tupdescs to match up as
> regards attnums/names/types, without considering that the source tupdesc
> might have missing values that the destination does not. So it incorrectly
> concludes that no conversion is needed, and the missing values become
> null.

Here is a quick patch that fixes this on master.
-- 
Vik Fearing

On 2020-02-05 03:03:04 +0100, Vik Fearing wrote:
> On 04/02/2020 03:24, PG Bug reporting form wrote:
> > The following example shows a tuple appearing with NULLs in a column
> > declared NOT NULL, because there is a missing default value not being filled
> > in. The cause is that convert_tuples_by_name, or more precisely
> > check_attrmap_match, thinks that it's enough for the tupdescs to match up as
> > regards attnums/names/types, without considering that the source tupdesc
> > might have missing values that the destination does not. So it incorrectly
> > concludes that no conversion is needed, and the missing values become
> > null.
> 
> Here is a quick patch that fixes this on master.

Should include a regression test...

- Andres

On 05/02/2020 03:19, Andres Freund wrote:
> On 2020-02-05 03:03:04 +0100, Vik Fearing wrote:
>> On 04/02/2020 03:24, PG Bug reporting form wrote:
>>> The following example shows a tuple appearing with NULLs in a column
>>> declared NOT NULL, because there is a missing default value not being filled
>>> in. The cause is that convert_tuples_by_name, or more precisely
>>> check_attrmap_match, thinks that it's enough for the tupdescs to match up as
>>> regards attnums/names/types, without considering that the source tupdesc
>>> might have missing values that the destination does not. So it incorrectly
>>> concludes that no conversion is needed, and the missing values become
>>> null.
>>
>> Here is a quick patch that fixes this on master.
> 
> Should include a regression test...

That's what I thought, too.  Where should I put it?
-- 
Vik Fearing

On 2020-02-05 03:21:52 +0100, Vik Fearing wrote:
> On 05/02/2020 03:19, Andres Freund wrote:
> > Should include a regression test...
> 
> That's what I thought, too.  Where should I put it?

alter.sql?

On 05/02/2020 03:26, Andres Freund wrote:
> On 2020-02-05 03:21:52 +0100, Vik Fearing wrote:
>> On 05/02/2020 03:19, Andres Freund wrote:
>>> Should include a regression test...
>>
>> That's what I thought, too.  Where should I put it?
> 
> alter.sql?

Okay, here you go.
-- 
Vik Fearing

>>>>> "Vik" == Vik Fearing <vik@postgresfriends.org> writes:

 Vik> Okay, here you go.

You should probably update the comments in the test case - regression
test comments shouldn't say they're going to fail unless that's the
expected result.

-- 
Andrew (irc:RhodiumToad)

>>>>> "Vik" == Vik Fearing <vik@postgresfriends.org> writes:

 >>> That's what I thought, too.  Where should I put it?
 >> 
 >> alter.sql?

 Vik> Okay, here you go.

I'll commit this (with fixed comments) unless anyone can immediately
think of any reason not to...

-- 
Andrew (irc:RhodiumToad)

On Wed, Feb 05, 2020 at 07:22:20PM +0000, Andrew Gierth wrote:
> I'll commit this (with fixed comments) unless anyone can immediately
> think of any reason not to...

The patch you have written looks right to me on HEAD and the
back-branches.  Now, you have added only a test that stresses
atthasmissing in convert_tuples_by_name_map_if_req(), but you are not
covering the paths from convert_tuples_by_position().  Shouldn't we
have a second test for that part as well?
--
Michael