pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #16242: convert_tuple_* not handling missing values correctly
9+ messages / 5 participants
[nested] [flat]
* BUG #16242: convert_tuple_* not handling missing values correctly
@ 2020-02-04 02:24 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 9+ messages in thread
From: PG Bug reporting form @ 2020-02-04 02:24 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: andrew@tao11.riddles.org.uk
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;
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #16242: convert_tuple_* not handling missing values correctly
@ 2020-02-05 02:03 Vik Fearing <vik@postgresfriends.org>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 9+ messages in thread
From: Vik Fearing @ 2020-02-05 02:03 UTC (permalink / raw)
To: andrew@tao11.riddles.org.uk; pgsql-bugs@lists.postgresql.org
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
Attachments:
[text/x-patch] bug16242_master.diff (999B, ../../31edb400-f065-73c4-7b1f-2953ad3413e6@postgresfriends.org/2-bug16242_master.diff)
download | inline diff:
diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c
index a074852a01..2cd16d7eaf 100644
--- a/src/backend/access/common/attmap.c
+++ b/src/backend/access/common/attmap.c
@@ -294,8 +294,14 @@ check_attrmap_match(TupleDesc indesc,
for (i = 0; i < attrMap->maplen; i++)
{
- Form_pg_attribute inatt;
- Form_pg_attribute outatt;
+ Form_pg_attribute inatt = TupleDescAttr(indesc, i);
+ Form_pg_attribute outatt = TupleDescAttr(outdesc, i);
+
+ /*
+ * If the input column has a missing attribute, we need a conversion.
+ */
+ if (inatt->atthasmissing)
+ return false;
if (attrMap->attnums[i] == (i + 1))
continue;
@@ -305,8 +311,6 @@ check_attrmap_match(TupleDesc indesc,
* dropped, we don't need a conversion. However, attlen and attalign
* must agree.
*/
- inatt = TupleDescAttr(indesc, i);
- outatt = TupleDescAttr(outdesc, i);
if (attrMap->attnums[i] == 0 &&
inatt->attisdropped &&
inatt->attlen == outatt->attlen &&
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #16242: convert_tuple_* not handling missing values correctly
@ 2020-02-05 02:19 Andres Freund <andres@anarazel.de>
parent: Vik Fearing <vik@postgresfriends.org>
0 siblings, 1 reply; 9+ messages in thread
From: Andres Freund @ 2020-02-05 02:19 UTC (permalink / raw)
To: Vik Fearing <vik@postgresfriends.org>; +Cc: andrew@tao11.riddles.org.uk; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #16242: convert_tuple_* not handling missing values correctly
@ 2020-02-05 02:21 Vik Fearing <vik@postgresfriends.org>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 9+ messages in thread
From: Vik Fearing @ 2020-02-05 02:21 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: andrew@tao11.riddles.org.uk; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #16242: convert_tuple_* not handling missing values correctly
@ 2020-02-05 02:26 Andres Freund <andres@anarazel.de>
parent: Vik Fearing <vik@postgresfriends.org>
0 siblings, 1 reply; 9+ messages in thread
From: Andres Freund @ 2020-02-05 02:26 UTC (permalink / raw)
To: Vik Fearing <vik@postgresfriends.org>; +Cc: andrew@tao11.riddles.org.uk; pgsql-bugs@lists.postgresql.org
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?
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #16242: convert_tuple_* not handling missing values correctly
@ 2020-02-05 02:38 Vik Fearing <vik@postgresfriends.org>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 9+ messages in thread
From: Vik Fearing @ 2020-02-05 02:38 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: andrew@tao11.riddles.org.uk; pgsql-bugs@lists.postgresql.org
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
Attachments:
[text/x-patch] bug16242_master.diff (3.6K, ../../5e0747c0-68af-c937-b8f7-c11b12126d7a@postgresfriends.org/2-bug16242_master.diff)
download | inline diff:
diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c
index a074852a01..2cd16d7eaf 100644
--- a/src/backend/access/common/attmap.c
+++ b/src/backend/access/common/attmap.c
@@ -294,8 +294,14 @@ check_attrmap_match(TupleDesc indesc,
for (i = 0; i < attrMap->maplen; i++)
{
- Form_pg_attribute inatt;
- Form_pg_attribute outatt;
+ Form_pg_attribute inatt = TupleDescAttr(indesc, i);
+ Form_pg_attribute outatt = TupleDescAttr(outdesc, i);
+
+ /*
+ * If the input column has a missing attribute, we need a conversion.
+ */
+ if (inatt->atthasmissing)
+ return false;
if (attrMap->attnums[i] == (i + 1))
continue;
@@ -305,8 +311,6 @@ check_attrmap_match(TupleDesc indesc,
* dropped, we don't need a conversion. However, attlen and attalign
* must agree.
*/
- inatt = TupleDescAttr(indesc, i);
- outatt = TupleDescAttr(outdesc, i);
if (attrMap->attnums[i] == 0 &&
inatt->attisdropped &&
inatt->attlen == outatt->attlen &&
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index e73ce4b6f3..a04f1c6756 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4258,3 +4258,41 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+/* Test case for bug #16242 */
+-- 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;
+ a | b
+---+---
+ 1 | 1
+(1 row)
+
+-- 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;
+INFO: a=1, b=1
+/* End test case for bug #16242 */
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index a16e4c9a29..1f6327296a 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2801,3 +2801,48 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+
+
+/* Test case for bug #16242 */
+
+-- 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;
+
+/* End test case for bug #16242 */
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #16242: convert_tuple_* not handling missing values correctly
@ 2020-02-05 03:09 Andrew Gierth <andrew@tao11.riddles.org.uk>
parent: Vik Fearing <vik@postgresfriends.org>
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Gierth @ 2020-02-05 03:09 UTC (permalink / raw)
To: Vik Fearing <vik@postgresfriends.org>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-bugs@lists.postgresql.org
>>>>> "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)
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #16242: convert_tuple_* not handling missing values correctly
@ 2020-02-05 19:22 Andrew Gierth <andrew@tao11.riddles.org.uk>
parent: Vik Fearing <vik@postgresfriends.org>
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Gierth @ 2020-02-05 19:22 UTC (permalink / raw)
To: Vik Fearing <vik@postgresfriends.org>; +Cc: Andres Freund <andres@anarazel.de>; pgsql-bugs@lists.postgresql.org
>>>>> "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)
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #16242: convert_tuple_* not handling missing values correctly
@ 2020-02-06 05:48 Michael Paquier <michael@paquier.xyz>
parent: Andrew Gierth <andrew@tao11.riddles.org.uk>
0 siblings, 0 replies; 9+ messages in thread
From: Michael Paquier @ 2020-02-06 05:48 UTC (permalink / raw)
To: Andrew Gierth <andrew@tao11.riddles.org.uk>; +Cc: Vik Fearing <vik@postgresfriends.org>; Andres Freund <andres@anarazel.de>; pgsql-bugs@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200206054846.GI23913@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2020-02-06 05:48 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-04 02:24 BUG #16242: convert_tuple_* not handling missing values correctly PG Bug reporting form <noreply@postgresql.org>
2020-02-05 02:03 ` Vik Fearing <vik@postgresfriends.org>
2020-02-05 02:19 ` Andres Freund <andres@anarazel.de>
2020-02-05 02:21 ` Vik Fearing <vik@postgresfriends.org>
2020-02-05 02:26 ` Andres Freund <andres@anarazel.de>
2020-02-05 02:38 ` Vik Fearing <vik@postgresfriends.org>
2020-02-05 03:09 ` Andrew Gierth <andrew@tao11.riddles.org.uk>
2020-02-05 19:22 ` Andrew Gierth <andrew@tao11.riddles.org.uk>
2020-02-06 05:48 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox