Обсуждение: pg_ugprade test failure on data set with column with default valuewith 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

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

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

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

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

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