pg_ugprade test failure on data set with column with default value with type bit/varbit

Started by Paul Guoalmost 8 years ago9 messages

paulguo@gmail.com

almost 8 years ago

Hello,

While testing pg_upgrade we seemed to find an issue related to default
value of a column with type bit/varbit.
Below are the steps to reproduce. In this case we added two 'create table'
DDLs in the regression database.
Obviously we saw diff after pg_upgrade testing. The pg binaries are based
on the code pulled a couple of days ago.

[pguo@host67:/data2/postgres/src/bin/pg_upgrade]$ git diff
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
old mode 100644
new mode 100755
index 39983abea1..a41105859e
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -188,6 +188,9 @@ if "$MAKE" -C "$oldsrc" installcheck; then
        psql -X -d regression -c "$fix_sql;" || psql_fix_sql_status=$?
    fi

+   psql -X -d regression -c "CREATE TABLE t111 ( a40 bit varying(5)
DEFAULT '1');"
+   psql -X -d regression -c "CREATE TABLE t222 ( a40 bit varying(5)
DEFAULT B'1');"
+
    pg_dumpall --no-sync -f "$temp_root"/dump1.sql || pg_dumpall1_status=$?

if [ "$newsrc" != "$oldsrc" ]; then

[pguo@host67:/data2/postgres/src/bin/pg_upgrade]$ make check

[pguo@host67:/data2/postgres/src/bin/pg_upgrade]$ diff -du
tmp_check/dump1.sql tmp_check/dump2.sql
--- tmp_check/dump1.sql 2018-03-30 16:31:44.329021348 +0800
+++ tmp_check/dump2.sql 2018-03-30 16:31:54.100478202 +0800
@@ -10956,7 +10956,7 @@
 --

 CREATE TABLE public.t111 (
-    a40 bit varying(5) DEFAULT B'1'::bit varying
+    a40 bit varying(5) DEFAULT (B'1'::"bit")::bit varying
 );

There is no diff in functionality of the dump SQLs, but it is annoying. The
simple patch below could fix this. Thanks.

[pguo@host67:/data2/postgres/src/bin/pg_upgrade]$ git diff
diff --git a/src/backend/utils/adt/ruleutils.c
b/src/backend/utils/adt/ruleutils.c
index 2cd54ec33f..ef2ab2d681 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -9389,7 +9389,7 @@ get_const_expr(Const *constval, deparse_context
*context, int showtype)

        case BITOID:
        case VARBITOID:
-           appendStringInfo(buf, "B'%s'", extval);
+           appendStringInfo(buf, "'%s'", extval);
            break;

case BOOLOID:

Robert Haas

robertmhaas@gmail.com

over 7 years ago

In reply to: Paul Guo (#1)

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

On Fri, Mar 30, 2018 at 5:36 AM, Paul Guo <paulguo@gmail.com> wrote:

There is no diff in functionality of the dump SQLs, but it is annoying. The
simple patch below could fix this. Thanks.

--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -9389,7 +9389,7 @@ get_const_expr(Const *constval, deparse_context
*context, int showtype)

case BITOID:
case VARBITOID:
-           appendStringInfo(buf, "B'%s'", extval);
+           appendStringInfo(buf, "'%s'", extval);
break;

case BOOLOID:

My first reaction was to guess that this would be unsafe, but looking
it over I don't see a problem. For the other types handled in that
switch statement, we rely on the custom representation to avoid
needing a typecast, but it seems that for BITOID and VARBITOID we
insert a typecast no matter what. So maybe the presence of absence of
the "B" makes no difference.

This logic seems to have been added by commit
c828ec88205a232a9789f157d8cf9c3d82f85152, Peter Eisentraut, vintage
2002.

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

Paul Guo

paulguo@gmail.com

over 7 years ago

In reply to: Robert Haas (#2)

1 attachment(s)

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

Thanks. I tentatively submitted a patch (See the attachment).

By the way, current pg_upgrade test script depends on the left data on test
database, but it seems that
a lot of tables are dropped in those test SQL files so this affects the
pg_upgrade test coverage much.
Maybe this needs to be addressed in the future. (Maybe when anyone checks
in test cases pg_upgrade
needs to be considered also?)

2018-05-11 3:08 GMT+08:00 Robert Haas <robertmhaas@gmail.com>:

Show quoted text

On Fri, Mar 30, 2018 at 5:36 AM, Paul Guo <paulguo@gmail.com> wrote:

There is no diff in functionality of the dump SQLs, but it is annoying.

The
simple patch below could fix this. Thanks.
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -9389,7 +9389,7 @@ get_const_expr(Const *constval, deparse_context
*context, int showtype)
case BITOID:
case VARBITOID:
-           appendStringInfo(buf, "B'%s'", extval);
+           appendStringInfo(buf, "'%s'", extval);
break;
case BOOLOID:
My first reaction was to guess that this would be unsafe, but looking
it over I don't see a problem. For the other types handled in that
switch statement, we rely on the custom representation to avoid
needing a typecast, but it seems that for BITOID and VARBITOID we
insert a typecast no matter what. So maybe the presence of absence of
the "B" makes no difference.

This logic seems to have been added by commit
c828ec88205a232a9789f157d8cf9c3d82f85152, Peter Eisentraut, vintage
2002.

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

Attachments:

0001-Fix-pg_upgrade-test-failure-caused-by-the-DDL-below.patchapplication/octet-stream; name=0001-Fix-pg_upgrade-test-failure-caused-by-the-DDL-below.patchDownload

From 83d6306242a404c18157f6c3300ad1ad19dc3492 Mon Sep 17 00:00:00 2001
From: Paul Guo <paulguo@gmail.com>
Date: Thu, 17 May 2018 15:20:28 +0800
Subject: [PATCH] Fix pg_upgrade test failure caused by the DDL below.

CREATE TABLE t111 ( a40 BIT VARYING(5) DEFAULT '1')

The failure is caused by the pg_dump diff (before and after pg_upgrade) as below.

 CREATE TABLE public.t111 (
 -    a40 bit varying(5) DEFAULT B'1'::bit varying
 +    a40 bit varying(5) DEFAULT (B'1'::"bit")::bit varying
  );

This patch is co-authored by Richard Guo <guofenglinux@gmail.com>
---
 src/backend/utils/adt/ruleutils.c | 2 +-
 src/bin/pg_upgrade/test.sh        | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 mode change 100644 => 100755 src/bin/pg_upgrade/test.sh

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 065238b0fe..0cd85b788d 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -9425,7 +9425,7 @@ get_const_expr(Const *constval, deparse_context *context, int showtype)
 
 		case BITOID:
 		case VARBITOID:
-			appendStringInfo(buf, "B'%s'", extval);
+			appendStringInfo(buf, "'%s'", extval);
 			break;
 
 		case BOOLOID:
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
old mode 100644
new mode 100755
-- 
2.15.1 (Apple Git-101)

Robert Haas

robertmhaas@gmail.com

over 7 years ago

In reply to: Paul Guo (#3)

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

On Thu, May 17, 2018 at 4:20 AM, Paul Guo <paulguo@gmail.com> wrote:

Thanks. I tentatively submitted a patch (See the attachment).

You probably want to add this to the next Commitfest.

By the way, current pg_upgrade test script depends on the left data on test
database, but it seems that
a lot of tables are dropped in those test SQL files so this affects the
pg_upgrade test coverage much.
Maybe this needs to be addressed in the future. (Maybe when anyone checks in
test cases pg_upgrade
needs to be considered also?)

There's been a deliberate attempt to leave a representative selection
of tables not dropped for this exact reason, but it's definitely
possible that interesting cases have been missed.

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

Davy Machado

machado.davy@gmail.com

over 7 years ago

In reply to: Paul Guo (#3)

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested

Hi Paul,

this is a review of the patch:
CABQrizc90sfkZgi4=+0Bbp1Zu3yEx9sM4rjBE1YNCvzf3qKHkA@mail.gmail.com

There hasn't been any problem, at least that I've been able to find.

This one applies cleanly.

Compile, pg_upgrade and pg_dumpall passed without error too.

Follow below a comparison of the results of the pg_dumpall:

############# Without patch #############

...

CREATE TABLE public.t111 (
a40 bit varying(5) DEFAULT (B'1'::"bit")::bit varying
);

...

CREATE TABLE public.t222 (
a40 bit varying(5) DEFAULT B'1'::"bit"
);

############# With patch #############

...

CREATE TABLE public.t111 (
a40 bit varying(5) DEFAULT ('1'::"bit")::bit varying
);

...

CREATE TABLE public.t222 (
a40 bit varying(5) DEFAULT '1'::"bit"
);

The "B", used to indicated a bit-string constant, removed as expected.

+1 for committer review

--
Davy Machado

Thomas Munro

thomas.munro@enterprisedb.com

over 7 years ago

In reply to: Paul Guo (#3)

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

On Thu, May 17, 2018 at 8:20 PM, Paul Guo <paulguo@gmail.com> wrote:

Thanks. I tentatively submitted a patch (See the attachment).

Hi Paul,

It looks like you missed a couple of changes in the contrib/btree_gist
bit and varbit tests, so make check-world fails:

- Index Cond: ((a >= B'1000000'::"bit") AND (a <= B'1000001'::"bit"))
+ Index Cond: ((a >= '1000000'::"bit") AND (a <= '1000001'::"bit"))

--
Thomas Munro
http://www.enterprisedb.com

Paul Guo

paulguo@gmail.com

over 7 years ago

In reply to: Thomas Munro (#6)

1 attachment(s)

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

Thanks. I updated the patch as attached.

Double-checked those tests passed.

2018-07-30 9:38 GMT+08:00 Thomas Munro <thomas.munro@enterprisedb.com>:

Show quoted text

On Thu, May 17, 2018 at 8:20 PM, Paul Guo <paulguo@gmail.com> wrote:

Thanks. I tentatively submitted a patch (See the attachment).

Hi Paul,

It looks like you missed a couple of changes in the contrib/btree_gist
bit and varbit tests, so make check-world fails:
- Index Cond: ((a >= B'1000000'::"bit") AND (a <= B'1000001'::"bit"))
+ Index Cond: ((a >= '1000000'::"bit") AND (a <= '1000001'::"bit"))
--
Thomas Munro
http://www.enterprisedb.com

Attachments:

0001-Fix-pg_upgrade-test-failure-caused-by-the-DDL-below.v2.patchapplication/octet-stream; name=0001-Fix-pg_upgrade-test-failure-caused-by-the-DDL-below.v2.patchDownload

From 9f2845b44226b5e0379d69d4f202c6a08cfde5f9 Mon Sep 17 00:00:00 2001
From: Paul Guo <paulguo@gmail.com>
Date: Wed, 1 Aug 2018 11:49:08 +0800
Subject: [PATCH] Fix pg_upgrade test failure caused by the DDL below.

CREATE TABLE t111 ( a40 BIT VARYING(5) DEFAULT '1')

The failure is caused by the pg_dump diff (before and after pg_upgrade) as below.

CREATE TABLE public.t111 (
-    a40 bit varying(5) DEFAULT B'1'::bit varying
+    a40 bit varying(5) DEFAULT (B'1'::"bit")::bit varying
);

This patch is co-authored by Richard Guo <guofenglinux@gmail.com>
---
 contrib/btree_gist/expected/bit.out    | 6 +++---
 contrib/btree_gist/expected/varbit.out | 6 +++---
 src/backend/utils/adt/ruleutils.c      | 2 +-
 src/bin/pg_upgrade/test.sh             | 0
 4 files changed, 7 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 src/bin/pg_upgrade/test.sh

diff --git a/contrib/btree_gist/expected/bit.out b/contrib/btree_gist/expected/bit.out
index 8606baf366..e57871f310 100644
--- a/contrib/btree_gist/expected/bit.out
+++ b/contrib/btree_gist/expected/bit.out
@@ -68,9 +68,9 @@ SELECT count(*) FROM bittmp WHERE a >   '011011000100010111011000110000100';
 SET enable_bitmapscan=off;
 EXPLAIN (COSTS OFF)
 SELECT a FROM bittmp WHERE a BETWEEN '1000000' and '1000001';
-                              QUERY PLAN                               
------------------------------------------------------------------------
+                             QUERY PLAN                              
+---------------------------------------------------------------------
  Index Only Scan using bitidx on bittmp
-   Index Cond: ((a >= B'1000000'::"bit") AND (a <= B'1000001'::"bit"))
+   Index Cond: ((a >= '1000000'::"bit") AND (a <= '1000001'::"bit"))
 (2 rows)
 
diff --git a/contrib/btree_gist/expected/varbit.out b/contrib/btree_gist/expected/varbit.out
index 538ace85c9..ede36bc3ea 100644
--- a/contrib/btree_gist/expected/varbit.out
+++ b/contrib/btree_gist/expected/varbit.out
@@ -68,9 +68,9 @@ SELECT count(*) FROM varbittmp WHERE a >   '1110100111010'::varbit;
 SET enable_bitmapscan=off;
 EXPLAIN (COSTS OFF)
 SELECT a FROM bittmp WHERE a BETWEEN '1000000' and '1000001';
-                              QUERY PLAN                               
------------------------------------------------------------------------
+                             QUERY PLAN                              
+---------------------------------------------------------------------
  Index Only Scan using bitidx on bittmp
-   Index Cond: ((a >= B'1000000'::"bit") AND (a <= B'1000001'::"bit"))
+   Index Cond: ((a >= '1000000'::"bit") AND (a <= '1000001'::"bit"))
 (2 rows)
 
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 03e9a28a63..ea63e912df 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -9459,7 +9459,7 @@ get_const_expr(Const *constval, deparse_context *context, int showtype)
 
 		case BITOID:
 		case VARBITOID:
-			appendStringInfo(buf, "B'%s'", extval);
+			appendStringInfo(buf, "'%s'", extval);
 			break;
 
 		case BOOLOID:
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
old mode 100644
new mode 100755
-- 
2.14.3

John Naylor

jcnaylor@gmail.com

over 7 years ago

In reply to: Paul Guo (#7)

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

On 8/1/18, Paul Guo <paulguo@gmail.com> wrote:

Thanks. I updated the patch as attached.

Double-checked those tests passed.

I've verified make check-world passes. I've marked it Ready for Committer.

-John Naylor

Tom Lane

tgl@sss.pgh.pa.us

over 7 years ago

In reply to: Paul Guo (#7)

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

Paul Guo <paulguo@gmail.com> writes:

[ 0001-Fix-pg_upgrade-test-failure-caused-by-the-DDL-below.v2.patch ]

Actually, there's an even easier way to fix this, which is to discard
the special case for BITOID/VARBITOID altogether, and let the "default"
case handle it. Fixing things by removing code is always great when
possible.

Also, it's fairly customary to add a test case that actually exhibits
the behavior you want to fix, so I added a regression test table
that has some bit/varbit columns with defaults. I confirmed that that
makes the pg_upgrade test fail without this ruleutils change.

Pushed with those changes.

regards, tom lane