Re: [HACKERS] [PATCH] WIP Add ALWAYS DEFERRED option for constraints

Nico Williams <nico@cryptonector.com> writes:

> [Re-send; first attempt appears to have hit /dev/null somewhere.  My
> apologies if you get two copies.]
>
> I've finally gotten around to rebasing this patch and making the change
> that was requested, which was: merge the now-would-be-three deferral-
> related bool columns in various pg_catalog tables into one char column.
>
> Instead of (deferrable, initdeferred, alwaysdeferred), now there is just
> (deferral).

This design seems correct to me.  I have a couple questions inline, and
some nits to go with them.

> All tests (make check) pass.

Thank you for adding tests!

> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
> index 3ed9021..e82e39b 100644
> --- a/doc/src/sgml/catalogs.sgml
> +++ b/doc/src/sgml/catalogs.sgml
> @@ -2239,17 +2239,15 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
>       </row>
>
>       <row>
> -      <entry><structfield>condeferrable</structfield></entry>
> -      <entry><type>bool</type></entry>
> -      <entry></entry>
> -      <entry>Is the constraint deferrable?</entry>
> -     </row>
> -
> -     <row>
> -      <entry><structfield>condeferred</structfield></entry>
> -      <entry><type>bool</type></entry>
> +      <entry><structfield>condeferral</structfield></entry>
> +      <entry><type>char</type></entry>
>        <entry></entry>
> -      <entry>Is the constraint deferred by default?</entry>
> +      <entry>Constraint deferral option:
> +            <literal>a</literal> = always deferred,
> +            <literal>d</literal> = deferrable,
> +            <literal>d</literal> = deferrable initially deferred,

From the rest of the code, I think this is supposed to be 'i', not 'd'.

> +            <literal>n</literal> = not deferrable
> +          </entry>
>       </row>
>
>       <row>

> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
> index 8b276bc..795a7a9 100644
> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
> @@ -1070,6 +1070,7 @@ index_create(Relation heapRelation,
>
>                  recordDependencyOn(&myself, &referenced, deptype);
>              }
> +            Assert((flags & INDEX_CREATE_ADD_CONSTRAINT) == 0);

What does this ensure, and why is it in this part of the code?  We're in
the `else` branch here - isn't this always the case (i.e., Assert can
never fire), since `flags` isn't manipulated in this block?

>          }
>
>          /* Store dependency on parent index, if any */

> diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
> index f4e69f4..bde6199 100644
> --- a/src/backend/catalog/information_schema.sql
> +++ b/src/backend/catalog/information_schema.sql
> @@ -891,10 +891,14 @@ CREATE VIEW domain_constraints AS
>             CAST(current_database() AS sql_identifier) AS domain_catalog,
>             CAST(n.nspname AS sql_identifier) AS domain_schema,
>             CAST(t.typname AS sql_identifier) AS domain_name,
> -           CAST(CASE WHEN condeferrable THEN 'YES' ELSE 'NO' END
> +           CAST(CASE WHEN condeferral = 'n' THEN 'NO' ELSE 'YES' END
>               AS yes_or_no) AS is_deferrable,
> -           CAST(CASE WHEN condeferred THEN 'YES' ELSE 'NO' END
> +           CAST(CASE WHEN condeferral = 'i' OR condeferral = 'a' THEN 'YES' ELSE 'NO' END
>               AS yes_or_no) AS initially_deferred
> +       /*
> +        * XXX Can we add is_always_deferred here?  Are there
> +        * standards considerations?
> +        */

I'm not familiar enough to speak to this.  Hopefully someone else can.
Absent other input, I think we should add is_always_deferred.  (And if
we were building this today, probably just expose a single character
instead of three booleans.)

>      FROM pg_namespace rs, pg_namespace n, pg_constraint con, pg_type t
>      WHERE rs.oid = con.connamespace
>            AND n.oid = t.typnamespace

> diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
> index 57519fe..41dc6a4 100644
> --- a/src/backend/commands/trigger.c
> +++ b/src/backend/commands/trigger.c
> @@ -3632,6 +3627,7 @@ typedef struct AfterTriggerSharedData
>      TriggerEvent ats_event;        /* event type indicator, see trigger.h */
>      Oid            ats_tgoid;        /* the trigger's ID */
>      Oid            ats_relid;        /* the relation it's on */
> +    bool            ats_alwaysdeferred;    /* whether this can be deferred */

"Whether this must be deferred"?  Also, I'm not sure what this is for -
it doesn't seem to be used anywhere.

>      CommandId    ats_firing_id;    /* ID for firing cycle */
>      struct AfterTriggersTableData *ats_table;    /* transition table access */
>  } AfterTriggerSharedData;

> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index 90dfac2..dab721a 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -184,8 +185,8 @@ static void SplitColQualList(List *qualList,
>                               List **constraintList, CollateClause **collClause,
>                               core_yyscan_t yyscanner);
>  static void processCASbits(int cas_bits, int location, const char *constrType,
> -               bool *deferrable, bool *initdeferred, bool *not_valid,
> -               bool *no_inherit, core_yyscan_t yyscanner);
> +               char *deferral,
> +               bool *not_valid, bool *no_inherit, core_yyscan_t yyscanner);

Can you fix the wrapping on this?

>  static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>
>  %}
> @@ -5538,17 +5568,24 @@ ConstraintAttributeSpec:
>                      int        newspec = $1 | $2;
>
>                      /* special message for this case */
> -                    if ((newspec & (CAS_NOT_DEFERRABLE | CAS_INITIALLY_DEFERRED)) == (CAS_NOT_DEFERRABLE |
CAS_INITIALLY_DEFERRED))
> +                    if ((newspec & CAS_NOT_DEFERRABLE) &&
> +                        (newspec & (CAS_INITIALLY_DEFERRED | CAS_ALWAYS_DEFERRED)))
>                          ereport(ERROR,
>                                  (errcode(ERRCODE_SYNTAX_ERROR),
>                                   errmsg("constraint declared INITIALLY DEFERRED must be DEFERRABLE"),
>                                   parser_errposition(@2)));
>                      /* generic message for other conflicts */
> +                    if ((newspec & CAS_ALWAYS_DEFERRED) &&
> +                        (newspec & (CAS_INITIALLY_IMMEDIATE)))
> +                        ereport(ERROR,
> +                                (errcode(ERRCODE_SYNTAX_ERROR),
> +                                 errmsg("conflicting constraint properties 1"),
> +                                 parser_errposition(@2)));
>                      if ((newspec & (CAS_NOT_DEFERRABLE | CAS_DEFERRABLE)) == (CAS_NOT_DEFERRABLE | CAS_DEFERRABLE)
||
>                          (newspec & (CAS_INITIALLY_IMMEDIATE | CAS_INITIALLY_DEFERRED)) == (CAS_INITIALLY_IMMEDIATE |
CAS_INITIALLY_DEFERRED))
>                          ereport(ERROR,
>                                  (errcode(ERRCODE_SYNTAX_ERROR),
> -                                 errmsg("conflicting constraint properties"),
> +                                 errmsg("conflicting constraint properties 2"),

I'd prefer you just repeat the message (or make them more situationally
descriptive), rather than appending a number.  (Repeating error messages
is in keeping with the style here.)

>                                   parser_errposition(@2)));
>                      $$ = newspec;
>                  }
> @@ -16197,34 +16234,41 @@ SplitColQualList(List *qualList,
>   */
>  static void
>  processCASbits(int cas_bits, int location, const char *constrType,
> -               bool *deferrable, bool *initdeferred, bool *not_valid,
> -               bool *no_inherit, core_yyscan_t yyscanner)
> +               char *deferral,
> +               bool *not_valid, bool *no_inherit, core_yyscan_t yyscanner)

Line wrapping?

>  {
>      /* defaults */
> -    if (deferrable)
> -        *deferrable = false;
> -    if (initdeferred)
> -        *initdeferred = false;
> +    if (deferral)
> +        *deferral = 'n';
>      if (not_valid)
>          *not_valid = false;
>
> -    if (cas_bits & (CAS_DEFERRABLE | CAS_INITIALLY_DEFERRED))
> +    if (cas_bits & CAS_ALWAYS_DEFERRED)
>      {
> -        if (deferrable)
> -            *deferrable = true;
> +        if (deferral)
> +            *deferral = 'a';
>          else
>              ereport(ERROR,
>                      (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                       /* translator: %s is CHECK, UNIQUE, or similar */
> -                     errmsg("%s constraints cannot be marked DEFERRABLE",
> +                     errmsg("%s constraints cannot be marked ALWAYS DEFERRED",
>                              constrType),
>                       parser_errposition(location)));
> -    }
> -
> -    if (cas_bits & CAS_INITIALLY_DEFERRED)
> +    } else if (cas_bits & CAS_INITIALLY_DEFERRED)

Style on this file doesn't cuddle `else`.  (i.e., `else if (cond)` gets its own
line without any braces on it.)

> +    {
> +        if (deferral)
> +            *deferral = 'i';
> +        else
> +            ereport(ERROR,
> +                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                     /* translator: %s is CHECK, UNIQUE, or similar */
> +                     errmsg("%s constraints cannot be marked INITIALLY DEFERRED",
> +                            constrType),
> +                     parser_errposition(location)));
> +    } else if (cas_bits & CAS_DEFERRABLE)
>      {
> -        if (initdeferred)
> -            *initdeferred = true;
> +        if (deferral)
> +            *deferral = 'd';
>          else
>              ereport(ERROR,
>                      (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),

Thanks,
--Robbie