Re: pg_ugprade test failure on data set with column with default value with type bit/varbit
Paul Guo 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
Re: pg_ugprade test failure on data set with column with default value with type bit/varbit
On 8/1/18, Paul Guo 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
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 : > On Thu, May 17, 2018 at 8:20 PM, Paul Guo 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'100'::"bit") AND (a <= B'101'::"bit")) > + Index Cond: ((a >= '100'::"bit") AND (a <= '101'::"bit")) > > -- > Thomas Munro > http://www.enterprisedb.com > 0001-Fix-pg_upgrade-test-failure-caused-by-the-DDL-below.v2.patch Description: Binary data
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 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'100'::"bit") AND (a <= B'101'::"bit")) + Index Cond: ((a >= '100'::"bit") AND (a <= '101'::"bit")) -- Thomas Munro http://www.enterprisedb.com
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=+0bbp1zu3yex9sm4rjbe1yncvzf3qk...@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
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 Guowrote: > 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
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: > On Fri, Mar 30, 2018 at 5:36 AM, Paul Guo 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 > 0001-Fix-pg_upgrade-test-failure-caused-by-the-DDL-below.patch Description: Binary data
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 Guowrote: > 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
pg_ugprade test failure on data set with column with default value with type bit/varbit
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: