Improve regex parsing of CHECK constraints.

[gordthompson]

Fixes: #5039

Description

Avoid parse warnings for PostgreSQL CHECK constraints that are a simple function call.

Checklist

This pull request is:

• A documentation / typographical error fix
  • Good to go, no issue or tests are needed
• A short code fix
  • please include the issue number, and create an issue if none exists, which
    must include a complete example of the issue. one line code fixes without an
    issue and demonstration will not be accepted.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests. one line code fixes without tests will not be accepted.
• A new feature implementation
  • please include the issue number, and create an issue if none exists, which must
    include a complete example of how the feature would look.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests.

Have a nice day!

[zzzeek]

looks good, lets use fixtures for the test case. thanks!

[gordthompson]

The beginning of the test now looks like this:

    @testing.provide_metadata
    def test_reflect_check_constraint(self):
        meta = self.metadata

        udf_create = """\
            CREATE OR REPLACE FUNCTION is_positive(
                x integer DEFAULT '-1'::integer)
                RETURNS boolean
                LANGUAGE 'plpgsql'
                COST 100
                VOLATILE 
            AS $BODY$BEGIN
                RETURN x > 0;
            END;$BODY$;
        """
        sa.event.listen(meta, "before_create",
                        sa.DDL(udf_create))
        sa.event.listen(meta, "after_drop",
                        sa.DDL("DROP FUNCTION is_positive"))

        cc_table = Table(
            "pgsql_cc",
            meta,
            Column("a", Integer()),
            CheckConstraint("a > 1 AND a < 5", name="cc1"),
            CheckConstraint("a = 1 OR (a > 2 AND a < 5)", name="cc2"),
            CheckConstraint("is_positive(a)", name="cc3"),
        )

        cc_table.create()

Unfortunately, the CREATE FUNCTION is not getting sent (at all; see attached log), so the CREATE TABLE fails. However, the DROP FUNCTION is getting sent, so that fails, too.

postgresql.log

[zzzeek]

yes call metadata.create_all() instead of table.create()

[gordthompson]

Yup, that did the trick. Thanks.

[zzzeek]

are you interested in writing changelog files ?

[gordthompson]

are you interested in writing changelog files ?

Sure, I could give it a bash. Just create a 5039.rst file? Right now it looks like they all live in the unreleased_14 folder.

[zzzeek]

OK so, this is a bug that we are targeting at 1.3.x, so it goes into unreleased_13. but yes thanks!

[sqla-tester]

OK, this is sqla-tester setting up my work to try to get revision b6903c6 of this pull request into gerrit so we can run tests and reviews and stuff

[sqla-tester]

New Gerrit review created for change b6903c6: https://gerrit.sqlalchemy.org/#/c/sqlalchemy/sqlalchemy/+/1623

[sqla-tester]

mike bayer (zzzeek) wrote:

so on older Posgresql 9.6, we're getting this error:

psycopg2.errors.SyntaxError: syntax error at end of input
LINE 1: DROP FUNCTION is_positive

does CREATE FUNCTION / DROP FUNCTION not exist on PG 9? we can add an exclusion rule to the test if this is the case.

[gordthompson]

Ah, it appears that DROP FUNCTION without the argument type(s) is only supported in 10+.

DROP FUNCTION is_positive(integer)

works in 12.1 and it should work in 9.6 as well.

[sqla-tester]

Gord Thompson (gordthompson) wrote:

The pep8/1843 failure "F841 local variable 'cc_table' is assigned to but never used" looks like a new rule. (I have already found the one bloody space that triggered the "trailing whitespace" failure, which also appears to be a new rule.) The sqlalchemy_gerrit/3577 failure looks like a transient.

[zzzeek]

Gord Thompson (gordthompson) wrote:

The pep8/1843 failure "F841 local variable 'cc_table' is assigned to but never used" looks like a new rule. (I have already found the one bloody space that triggered the "trailing whitespace" failure, which also appears to be a new rule.) The sqlalchemy_gerrit/3577 failure looks like a transient.

F841 has been in for awhile. I'm working on F821 in https://gerrit.sqlalchemy.org/#/c/sqlalchemy/sqlalchemy/+/1649/ but F841 was in since 190e013

[gordthompson]

Yup, you're right. We had to change cc_table.create() to metadata.create_all() and that got rid of the usage. Anyway, we're passing now.

[sqla-tester]

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+1

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/1623 has been merged. Congratulations! :)

[sqla-tester]

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+1

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/1672 has been merged. Congratulations! :)